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, [email protected] wrote:
> From: Edward Adam Davis <[email protected]>
>
> 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: [email protected]
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Edward Adam Davis <[email protected]>
> ---
> 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;
}
}