[syzbot] KASAN: slab-out-of-bounds Read in ntfs_get_ea

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: e47eb90a0a9a Add linux-next specific files for 20220901
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13a9f455080000
kernel config: https://syzkaller.appspot.com/x/.config?x=7933882276523081
dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1550d5e5080000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=173664d7080000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com

ntfs3: loop0: Different NTFS' sector size (4096) and media sector size (512)
ntfs3: loop0: Mark volume as dirty due to NTFS errors
ntfs3: loop0: Failed to load $Extend.
==================================================================
BUG: KASAN: slab-out-of-bounds in unpacked_ea_size fs/ntfs3/xattr.c:26 [inline]
BUG: KASAN: slab-out-of-bounds in unpacked_ea_size fs/ntfs3/xattr.c:23 [inline]
BUG: KASAN: slab-out-of-bounds in find_ea fs/ntfs3/xattr.c:54 [inline]
BUG: KASAN: slab-out-of-bounds in find_ea fs/ntfs3/xattr.c:44 [inline]
BUG: KASAN: slab-out-of-bounds in ntfs_get_ea+0x5ca/0x620 fs/ntfs3/xattr.c:230
Read of size 1 at addr ffff88801b66abbd by task syz-executor378/3604

CPU: 0 PID: 3604 Comm: syz-executor378 Not tainted 6.0.0-rc3-next-20220901-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/26/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:317 [inline]
print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
unpacked_ea_size fs/ntfs3/xattr.c:26 [inline]
unpacked_ea_size fs/ntfs3/xattr.c:23 [inline]
find_ea fs/ntfs3/xattr.c:54 [inline]
find_ea fs/ntfs3/xattr.c:44 [inline]
ntfs_get_ea+0x5ca/0x620 fs/ntfs3/xattr.c:230
ntfs_get_wsl_perm+0x117/0x360 fs/ntfs3/xattr.c:973
ntfs_read_mft fs/ntfs3/inode.c:355 [inline]
ntfs_iget5+0xe65/0x3280 fs/ntfs3/inode.c:501
dir_search_u+0x36a/0x3f0 fs/ntfs3/dir.c:264
ntfs_lookup+0x174/0x1e0 fs/ntfs3/namei.c:83
__lookup_slow+0x24c/0x460 fs/namei.c:1685
lookup_slow fs/namei.c:1702 [inline]
walk_component+0x33f/0x5a0 fs/namei.c:1993
lookup_last fs/namei.c:2450 [inline]
path_lookupat+0x1ba/0x840 fs/namei.c:2474
filename_lookup+0x1ce/0x590 fs/namei.c:2503
vfs_statx+0x148/0x390 fs/stat.c:228
do_statx+0xd9/0x160 fs/stat.c:629
__do_sys_statx fs/stat.c:656 [inline]
__se_sys_statx fs/stat.c:647 [inline]
__x64_sys_statx+0x157/0x1b0 fs/stat.c:647
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f0f3fb82519
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffcc68ced18 EFLAGS: 00000246 ORIG_RAX: 000000000000014c
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f0f3fb82519
RDX: 0000000000000000 RSI: 0000000020003c40 RDI: 0000000000000005
RBP: 00007f0f3fb41d10 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000080 R11: 0000000000000246 R12: 00007f0f3fb41da0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>

Allocated by task 3604:
kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38
kasan_set_track mm/kasan/common.c:45 [inline]
set_alloc_info mm/kasan/common.c:437 [inline]
____kasan_kmalloc mm/kasan/common.c:516 [inline]
____kasan_kmalloc mm/kasan/common.c:475 [inline]
__kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:525
kasan_kmalloc include/linux/kasan.h:234 [inline]
__do_kmalloc_node mm/slab_common.c:930 [inline]
__kmalloc+0x54/0xc0 mm/slab_common.c:943
kmalloc include/linux/slab.h:564 [inline]
ntfs_read_ea+0x3e4/0x850 fs/ntfs3/xattr.c:110
ntfs_get_ea+0x1ad/0x620 fs/ntfs3/xattr.c:222
ntfs_get_wsl_perm+0x117/0x360 fs/ntfs3/xattr.c:973
ntfs_read_mft fs/ntfs3/inode.c:355 [inline]
ntfs_iget5+0xe65/0x3280 fs/ntfs3/inode.c:501
dir_search_u+0x36a/0x3f0 fs/ntfs3/dir.c:264
ntfs_lookup+0x174/0x1e0 fs/ntfs3/namei.c:83
__lookup_slow+0x24c/0x460 fs/namei.c:1685
lookup_slow fs/namei.c:1702 [inline]
walk_component+0x33f/0x5a0 fs/namei.c:1993
lookup_last fs/namei.c:2450 [inline]
path_lookupat+0x1ba/0x840 fs/namei.c:2474
filename_lookup+0x1ce/0x590 fs/namei.c:2503
vfs_statx+0x148/0x390 fs/stat.c:228
do_statx+0xd9/0x160 fs/stat.c:629
__do_sys_statx fs/stat.c:656 [inline]
__se_sys_statx fs/stat.c:647 [inline]
__x64_sys_statx+0x157/0x1b0 fs/stat.c:647
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

The buggy address belongs to the object at ffff88801b66ab80
which belongs to the cache kmalloc-64 of size 64
The buggy address is located 61 bytes inside of
64-byte region [ffff88801b66ab80, ffff88801b66abc0)

The buggy address belongs to the physical page:
page:ffffea00006d9a80 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1b66a
flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
raw: 00fff00000000200 ffffea0001fc0040 dead000000000002 ffff888011841640
raw: 0000000000000000 0000000080200020 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 2, tgid 2 (kthreadd), ts 6668566803, free_ts 0
prep_new_page mm/page_alloc.c:2534 [inline]
get_page_from_freelist+0x109b/0x2ce0 mm/page_alloc.c:4284
__alloc_pages+0x1c7/0x510 mm/page_alloc.c:5542
alloc_pages+0x1a6/0x270 mm/mempolicy.c:2280
alloc_slab_page mm/slub.c:1721 [inline]
allocate_slab+0x228/0x370 mm/slub.c:1866
new_slab mm/slub.c:1919 [inline]
___slab_alloc+0xad0/0x1440 mm/slub.c:3100
__slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3198
slab_alloc_node mm/slub.c:3283 [inline]
__kmem_cache_alloc_node+0x18a/0x3d0 mm/slub.c:3356
__do_kmalloc_node mm/slab_common.c:929 [inline]
__kmalloc+0x44/0xc0 mm/slab_common.c:943
kmalloc include/linux/slab.h:564 [inline]
kzalloc include/linux/slab.h:695 [inline]
lsm_task_alloc security/security.c:615 [inline]
security_task_alloc+0x10b/0x250 security/security.c:1655
copy_process+0x23de/0x7120 kernel/fork.c:2240
kernel_clone+0xe7/0xab0 kernel/fork.c:2678
kernel_thread+0xb5/0xf0 kernel/fork.c:2738
create_kthread kernel/kthread.c:399 [inline]
kthreadd+0x4ea/0x750 kernel/kthread.c:746
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
page_owner free stack trace missing

Memory state around the buggy address:
ffff88801b66aa80: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff88801b66ab00: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>ffff88801b66ab80: 00 00 00 00 00 00 00 04 fc fc fc fc fc fc fc fc
^
ffff88801b66ac00: fa fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff88801b66ac80: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

the root case is:
The remaining space after the offset is less than the space needed to
accommodate the next EA_FULL struct.

Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@sina.com>
---
fs/ntfs3/xattr.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 7de8718c68a9..c90cc453390d 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -52,6 +52,7 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
for (;;) {
const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
u32 next_off = *off + unpacked_ea_size(ea);
+ u32 next_len = 0;

if (next_off > bytes)
return false;
@@ -63,6 +64,13 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
*off = next_off;
if (next_off >= bytes)
return false;
+
+ next_len = next_off + 8;
+ if (next_len >= bytes ||
+ ((!ea->size) &&
+ (next_len + ea->name_len +
+ le16_to_cpu(ea->elength) >= bytes))
+ return false;
}
}

--
2.37.2

[kernel test robot]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.0-rc5 next-20220909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/eadavis-sina-com/fs-netfs3-add-a-boundary-check-for-EA_FULL/20220912-114458
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 80e78fcce86de0288793a0ef0f6acf37656ee4cf
config: sparc-randconfig-r024-20220911
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2d169c3ed66886da971be5f3c6c24137ee7877d6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review eadavis-sina-com/fs-netfs3-add-a-boundary-check-for-EA_FULL/20220912-114458
git checkout 2d169c3ed66886da971be5f3c6c24137ee7877d6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash fs/ntfs3/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <l...@intel.com>

All error/warnings (new ones prefixed by >>):

fs/ntfs3/xattr.c: In function 'find_ea':
>> fs/ntfs3/xattr.c:1108:19: error: unterminated argument list invoking macro "if"
1108 | // clang-format on
| ^
>> fs/ntfs3/xattr.c:1109: error: expected '(' at end of input
fs/ntfs3/xattr.c:69:17: note: '-Wmisleading-indentation' is disabled from this point onwards, since column-tracking was disabled due to the size of the code/headers
69 | if (next_len >= bytes ||
| ^~
fs/ntfs3/xattr.c:69:17: note: adding '-flarge-source-files' will allow for more column-tracking support, at the expense of compilation time and memory
>> fs/ntfs3/xattr.c:69:17: error: expected declaration or statement at end of input
>> fs/ntfs3/xattr.c:55:21: warning: variable 'next_len' set but not used [-Wunused-but-set-variable]
55 | u32 next_len = 0;
| ^~~~~~~~
>> fs/ntfs3/xattr.c:69:17: error: expected declaration or statement at end of input
69 | if (next_len >= bytes ||
| ^~


vim +/if +1108 fs/ntfs3/xattr.c

be71b5cba2e648 Konstantin Komarov 2021-08-13 1103
be71b5cba2e648 Konstantin Komarov 2021-08-13 1104 const struct xattr_handler *ntfs_xattr_handlers[] = {
be71b5cba2e648 Konstantin Komarov 2021-08-13 1105 &ntfs_xattr_handler,
be71b5cba2e648 Konstantin Komarov 2021-08-13 1106 NULL,
be71b5cba2e648 Konstantin Komarov 2021-08-13 1107 };
be71b5cba2e648 Konstantin Komarov 2021-08-13 @1108 // clang-format on

--
0-DAY CI Kernel Test Service
https://01.org/lkp

[kernel test robot]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.0-rc5 next-20220909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/eadavis-sina-com/fs-netfs3-add-a-boundary-check-for-EA_FULL/20220912-114458
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 80e78fcce86de0288793a0ef0f6acf37656ee4cf

config: i386-randconfig-a014-20220912
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2d169c3ed66886da971be5f3c6c24137ee7877d6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review eadavis-sina-com/fs-netfs3-add-a-boundary-check-for-EA_FULL/20220912-114458
git checkout 2d169c3ed66886da971be5f3c6c24137ee7877d6
# save the config file
mkdir build_dir && cp config build_dir/.config

COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <l...@intel.com>

All errors (new ones prefixed by >>):

>> fs/ntfs3/xattr.c:73:4: error: expected ')'
return false;
^
fs/ntfs3/xattr.c:69:6: note: to match this '('
if (next_len >= bytes ||
^
1 error generated.


vim +73 fs/ntfs3/xattr.c

38
39 /*
40 * find_ea
41 *
42 * Assume there is at least one xattr in the list.
43 */
44 static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
45 const char *name, u8 name_len, u32 *off)
46 {
47 *off = 0;
48
49 if (!ea_all || !bytes)
50 return false;
51
52 for (;;) {
53 const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
54 u32 next_off = *off + unpacked_ea_size(ea);

55 u32 next_len = 0;

56
57 if (next_off > bytes)
58 return false;
59
60 if (ea->name_len == name_len &&
61 !memcmp(ea->name, name, name_len))
62 return true;
63
64 *off = next_off;
65 if (next_off >= bytes)
66 return false;
67
68 next_len = next_off + 8;

69 if (next_len >= bytes ||

70 ((!ea->size) &&
71 (next_len + ea->name_len +
72 le16_to_cpu(ea->elength) >= bytes))
> 73 return false;
74 }
75 }
76

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

the root cause is:

The remaining space after the offset is less than the space needed to
accommodate the next EA_FULL struct.

Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@sina.com>
---
fs/ntfs3/xattr.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 7de8718c68a9..c90cc453390d 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c

@@ -52,6 +52,7 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
for (;;) {

const struct EA_FULL *ea = Add2Ptr(ea_all, *off);

u32 next_off = *off + unpacked_ea_size(ea);

+ u32 next_len = 0;

if (next_off > bytes)
return false;
@@ -63,6 +64,13 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
*off = next_off;
if (next_off >= bytes)
return false;
+
+ next_len = next_off + 8;
+ if (next_len >= bytes ||
+ ((!ea->size) &&

+ (next_len + ea->name_len +
+ le16_to_cpu(ea->elength) >= bytes)))

[Dan Carpenter]

On Mon, Sep 12, 2022 at 02:54:31PM +0800, ead...@sina.com wrote:
> From: Edward Adam Davis <ead...@sina.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 needs to be checked on the first iteration as well before calling
unpacked_ea_size(ea).

if (bytes - *off < sizeof(*ea))
return false;

> Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
> Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <ead...@sina.com>
> ---
> fs/ntfs3/xattr.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 7de8718c68a9..c90cc453390d 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -52,6 +52,7 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
> for (;;) {
> const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
> u32 next_off = *off + unpacked_ea_size(ea);
> + u32 next_len = 0;
>
> if (next_off > bytes)
> return false;
> @@ -63,6 +64,13 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
> *off = next_off;
> if (next_off >= bytes)
> return false;
> +
> + next_len = next_off + 8;

8 is a magic number. Use sizeof(*ea).

> + if (next_len >= bytes ||
> + ((!ea->size) &&
> + (next_len + ea->name_len +
> + le16_to_cpu(ea->elength) >= bytes)))

This is open coding unpacked_ea_size() but slightly different/incorrect.
No need to check this anyway, because it gets checked at the start of
the iteration.

If we add the if (bytes - *off < sizeof(*ea)) check to the start of the
iteration that means we can delete the "if (next_off == bytes)" check
from the end.

The second issue with this code is that unpacked_ea_size() is a user
controlled u32. We're adding it to "*off" which is also a u32 so that
can have an integer overflow...

See diff below. (Untested).

regards,
dan carpenter

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 7de8718c68a9..c3dbe06fb784 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -44,14 +44,20 @@ static inline size_t packed_ea_size(const struct EA_FULL *ea)

static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,

const char *name, u8 name_len, u32 *off)

{
+ const struct EA_FULL *ea;
+ u32 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 +67,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;
}
}

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

I have provided a new patch:
https://lore.kernel.org/all/20220912065431.3...@sina.com
It is a collection of the current patch and the previous one.

Signed-off-by: Edward Adam Davis <ead...@sina.com>

Reported-by: kernel test robot <l...@intel.com>

Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com
---
fs/ntfs3/xattr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index c90cc453390d..4ec8f7e41b6d 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -69,7 +69,7 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
if (next_len >= bytes ||
((!ea->size) &&

(next_len + ea->name_len +

- le16_to_cpu(ea->elength) >= bytes))

+ le16_to_cpu(ea->elength) >= bytes)))

return false;
}
}
--
2.37.2

[Dan Carpenter]

On Mon, Sep 19, 2022 at 01:19:43PM +0800, ead...@sina.com wrote:
> From: Edward Adam Davis <ead...@sina.com>
>
> I have provided a new patch:
> https://lore.kernel.org/all/20220912065431.3...@sina.com
> It is a collection of the current patch and the previous one.
>
> Signed-off-by: Edward Adam Davis <ead...@sina.com>
> Reported-by: kernel test robot <l...@intel.com>
> Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com

No need to send this email. The correct thing was to send a v2 which
fixes the bug and you already did that. However your v2 patch was
missing a list of changes. "v2: don't break the build".

https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/

You will want to address the review comments I sent and send a v3 patch.

regards,
dan carpenter

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

the root cause is:
The remaining space after the offset is less than the space needed to
accommodate the next EA_FULL struct.

Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com

Suggested-by: Dan Carpenter <dan.ca...@oracle.com>

Signed-off-by: Edward Adam Davis <ead...@sina.com>

---
Changes in v3:
Add Suggested-by: and fix the syntax err.

fs/ntfs3/xattr.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 7de8718c68a9..c90cc453390d 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c

@@ -52,6 +52,7 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
for (;;) {
const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
u32 next_off = *off + unpacked_ea_size(ea);
+ u32 next_len = 0;

if (next_off > bytes)
return false;
@@ -63,6 +64,13 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
*off = next_off;
if (next_off >= bytes)
return false;
+
+ next_len = next_off + 8;

+ if (next_len >= bytes ||
+ ((!ea->size) &&

+ (next_len + ea->name_len +

+ le16_to_cpu(ea->elength) >= bytes)))

[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, ead...@sina.com wrote:
> From: Edward Adam Davis <ead...@sina.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+c4d950...@syzkaller.appspotmail.com
> Suggested-by: Dan Carpenter <dan.ca...@oracle.com>
> Signed-off-by: Edward Adam Davis <ead...@sina.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,

[ead...@sina.com]

From: eadaivs <ead...@sina.com>

1) If we call find_ea() if "bytes" less than 8 then the

unpacked_ea_size() is an out of bounds read.
2) The *off + unpacked_ea_size() can have an integer overflow.

Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com

Signed-off-by: eadaivs <ead...@sina.com>
---
Changes in v4:
Separate out the issue with the buffer overflow and the integer
overflow.

fs/ntfs3/xattr.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index afd0ddad826f..3cd1a24c7bcc 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,23 @@ 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;

+ u32 next_off;

*off = 0;

- if (!ea_all || !bytes)

+ if (!ea_all || bytes < sizeof(*ea))
return false;

for (;;) {
- const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
- u32 next_off = *off + unpacked_ea_size(ea);

+ *ea = Add2Ptr(ea_all, *off);
+
+ if (sizeof(*ea) + *off > bytes)
+ return false;
+
+ next_off = size_add(*off + unpacked_ea_size(ea));

if (next_off > bytes)
return false;

@@ -61,8 +67,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;
}
}

--
2.37.2

[ead...@sina.com]

From: eadaivs <ead...@sina.com>

1) If we call find_ea() if "bytes" less than 8 then the

unpacked_ea_size() is an out of bounds read.
2) The *off + unpacked_ea_size() can have an integer overflow.

Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
Reported-by: syzbot+c4d950...@syzkaller.appspotmail.com

Signed-off-by: eadaivs <ead...@sina.com>
---
Changes in v4:
Separate out the issue with the buffer overflow and the integer
overflow.

fs/ntfs3/xattr.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index afd0ddad826f..3cd1a24c7bcc 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,23 @@ 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;

+ u32 next_off;

*off = 0;

- if (!ea_all || !bytes)

+ if (!ea_all || bytes < sizeof(*ea))
return false;

for (;;) {
- const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
- u32 next_off = *off + unpacked_ea_size(ea);

+ *ea = Add2Ptr(ea_all, *off);
+
+ if (sizeof(*ea) + *off > bytes)

+ return false;
+

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

if (next_off > bytes)
return false;

@@ -61,8 +67,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;
}
}

--
2.37.2

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

On Tue, 06 Sep 2022 09:55:30 -0700

> syzbot found the following issue on:
>
> HEAD commit: e47eb90a0a9a Add linux-next specific files for 20220901
> git tree: linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=13a9f455080000
> kernel config: https://syzkaller.appspot.com/x/.config?x=7933882276523081
> dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1550d5e5080000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=173664d7080000

Check the issue is fix.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git e47eb90a0a9a

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/ntfs3/xattr.c:55:7: error: assignment of read-only location '*ea'


Tested on:

commit: e47eb90a Add linux-next specific files for 20220901
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=13898c04880000

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

On Tue, 06 Sep 2022 09:55:30 -0700

> syzbot found the following issue on:
>
> HEAD commit: e47eb90a0a9a Add linux-next specific files for 20220901
> git tree: linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=13a9f455080000
> kernel config: https://syzkaller.appspot.com/x/.config?x=7933882276523081
> dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1550d5e5080000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=173664d7080000

Fix *ea read-only err, and go on checking the issue is fix.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git e47eb90a0a9a

--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,23 @@ 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;
+ u32 next_off;
*off = 0;

- if (!ea_all || !bytes)
+ if (!ea_all || bytes < sizeof(*ea))
return false;

for (;;) {

const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
- u32 next_off = *off + unpacked_ea_size(ea);
+

+ if (sizeof(*ea) + *off > bytes)
+ return false;
+

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

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/ntfs3/xattr.c:60:14: error: too few arguments to function 'size_add'


Tested on:

commit: e47eb90a Add linux-next specific files for 20220901
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=16a32d40880000

[ead...@sina.com]

From: Edward Adam Davis <ead...@sina.com>

On Tue, 06 Sep 2022 09:55:30 -0700

> syzbot found the following issue on:
>
> HEAD commit: e47eb90a0a9a Add linux-next specific files for 20220901
> git tree: linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=13a9f455080000
> kernel config: https://syzkaller.appspot.com/x/.config?x=7933882276523081
> dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1550d5e5080000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=173664d7080000

Fix *ea read-only err double check, and go on checking the issue is fix.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git e47eb90a0a9a

--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,23 @@ 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 *eb;

+ u32 next_off;
*off = 0;

- if (!ea_all || !bytes)

+ if (!ea_all || bytes < sizeof(*eb))

return false;

for (;;) {
const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
- u32 next_off = *off + unpacked_ea_size(ea);
+
+ if (sizeof(*ea) + *off > bytes)
+ return false;
+

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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+c4d950...@syzkaller.appspotmail.com

Tested on:

commit: e47eb90a Add linux-next specific files for 20220901
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
console output: https://syzkaller.appspot.com/x/log.txt?x=172b636c880000

kernel config: https://syzkaller.appspot.com/x/.config?x=7933882276523081
dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=167e7ddf080000

Note: testing is done by a robot and is best-effort only.

[ead...@sina.com]

From: eadaivs <ead...@sina.com>

Syzbot reports a slab out of bounds read in ntfs_get_ea,
According to splat, the final problem is in the function find_ea,

1) If parameter "bytes" less than the structure EA_ FULL Pointer Object Size,

then the unpacked_ea_size() is an out of bounds read.

2) The *off + unpacked_ea_size() can have an integer overflow.

Change the type of the parameter bytes to size_t,
add sanity check for ensuring that the parameter bytes within reason.
And instead of calculating the offset, use the function size add.

Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
Reported-and-tested-by: syzbot+c4d950...@syzkaller.appspotmail.com
Fixes: (be71b5cba2e6) fs/ntfs3: Add attrib operations
Signed-off-by: eadaivs <ead...@sina.com>
---
Changes in v5:
Refactor the code and pass syzbot test.

fs/ntfs3/xattr.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index afd0ddad826f..188a76c28233 100644

--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,23 @@ 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 *eb;

+ u32 next_off;
*off = 0;

- if (!ea_all || !bytes)

+ if (!ea_all || bytes < sizeof(*eb))

return false;

for (;;) {

const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
- u32 next_off = *off + unpacked_ea_size(ea);
+

[syzbot]

syzbot suspects this issue was fixed by commit:

commit 0e8235d28f3a0e9eda9f02ff67ee566d5f42b66b
Author: Konstantin Komarov <almaz.ale...@paragon-software.com>
Date: Mon Oct 10 10:15:33 2022 +0000

fs/ntfs3: Check fields while reading

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15cc6199480000
start commit: fef7fd48922d Merge tag 'scsi-fixes' of git://git.kernel.or..
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=7341d0f941b3356
dashboard link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=133d4479880000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13f05e71880000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: fs/ntfs3: Check fields while reading

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Aleksandr Nogikh]

#syz fix: fs/ntfs3: Check fields while reading

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000002c692505f3265254%40google.com.