Re: [PATCH v3] fs/ntfs3: add a boundary check for EA_FULL

[Dan Carpenter]

This patch is useful and shows what the bug is however the fix needs
some additional work.

On Mon, Sep 19, 2022 at 07:19:54PM +0800, eadavis@xxxxxxxx wrote:
> From: Edward Adam Davis <eadavis@xxxxxxxx>
> 
> the root cause is: 
> The remaining space after the offset is less than the space needed to 
> accommodate the next EA_FULL struct.

This commit message is not good and does not explain what how the
problem appears to the user.  Don't start the commit message in the
middle of a sentence.

> 
> Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7  
> Reported-by: syzbot+c4d950787fd5553287b7@xxxxxxxxxxxxxxxxxxxxxxxxx
> Suggested-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Edward Adam Davis <eadavis@xxxxxxxx>
> ---
> Changes in v3:
>   Add Suggested-by: and fix the syntax err.

This is not what I suggested... There several problems with this patch.

1) If we call find_ea() if "bytes" set to a number in 1-7 range then
   then the unpacked_ea_size() is an out of bounds read.
2) The *off + unpacked_ea_size() can have an integer overflow.
3) The math in "next_len + ea->name_len + le16_to_cpu(ea->elength)" is
   not correct.  It should use unpacked_ea_size() instead.  (The math
   is different, this is a bug an not a style complaint).
4) My one style complaint is that "8" in "next_off + 8" is a magic
   number.

So maybe we could separate out the issue with the buffer overflow from
the integer overflow.

Patch 1:  Fix buffer overflow:

Add "if (bytes - *off < sizeof(*ea))" before the call to
"ea = Add2Ptr(ea_all, *off);".  Then remove the "!bytes" test.  That
test is wrong and useless.  Also remove the if (next_off >= bytes) test.
That test is also insufficient and no longer required.

Patch 2: Fix the integer overflow bug.

-		next_off = *off + unpacked_ea_size(ea);
+		next_off = size_add(*off, unpacked_ea_size(ea));

We also need to change the types of next_off and bytes to size_t for the
size_add() to be useful.  I forgot about that last time I sent this.

After both patches are applied the code will look like below.

regards,
dan carpenter


diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 7de8718c68a9..bf53ed96b03f 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,24 @@ static inline size_t packed_ea_size(const struct EA_FULL *ea)
  *
  * Assume there is at least one xattr in the list.
  */
-static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
+static inline bool find_ea(const struct EA_FULL *ea_all, size_t bytes,
 			   const char *name, u8 name_len, u32 *off)
 {
+	const struct EA_FULL *ea;
+	size_t next_off;
+
 	*off = 0;
 
-	if (!ea_all || !bytes)
+	if (!ea_all)
 		return false;
 
+
 	for (;;) {
-		const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
-		u32 next_off = *off + unpacked_ea_size(ea);
+		if (bytes - *off < sizeof(*ea))
+			return false;
+
+		ea = Add2Ptr(ea_all, *off);
+		next_off = size_add(*off, unpacked_ea_size(ea));
 
 		if (next_off > bytes)
 			return false;
@@ -61,8 +68,6 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
 			return true;
 
 		*off = next_off;
-		if (next_off >= bytes)
-			return false;
 	}
 }