[kbuild-all] 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)sina.com wrote: 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. 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; } }