linux-kernel - Re: [PATCH v3] fs/ntfs3: add a boundary check for EA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <Yyhaw8iBPulIO0Pp@kadam>
Date:   Mon, 19 Sep 2022 15:04:19 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     eadavis@...a.com
Cc:     almaz.alexandrovich@...agon-software.com, kbuild-all@...ts.01.org,
        linux-kernel@...r.kernel.org, lkp@...el.com, llvm@...ts.linux.dev,
        nathan@...nel.org, ndesaulniers@...gle.com, ntfs3@...ts.linux.dev,
        syzbot+c4d950787fd5553287b7@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com, trix@...hat.com
Subject: Re: [PATCH v3] fs/ntfs3: add a boundary check for EA_FULL

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@...a.com wrote:
> From: Edward Adam Davis <eadavis@...a.com>
> 
> 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@...kaller.appspotmail.com
> Suggested-by: Dan Carpenter <dan.carpenter@...cle.com>
> Signed-off-by: Edward Adam Davis <eadavis@...a.com>
> ---
> 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;
 	}
 }