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; } }
