1. Patchew
  2. linux
  3. View series

[PATCH v1] udf: return EIO when ftrunctate access beyond end of device

Zhao Mengmeng posted 1 patch 2 months, 2 weeks ago 
fs/udf/inode.c | 10 ++++++++++
1 file changed, 10 insertions(+)
[PATCH v1] udf: return EIO when ftrunctate access beyond end of device
Posted by Zhao Mengmeng 2 months, 2 weeks ago 
syzbot reports a udf slab-out-of-bounds as blow:

loop0: rw=0, sector=117, nr_sectors = 1 limit=0
syz-executor135: attempt to access beyond end of device
loop0: rw=0, sector=117, nr_sectors = 1 limit=0
==================================================================
BUG: KASAN: slab-out-of-bounds in udf_get_filelongad+0x167/0x1b0 fs/udf/directory.c:526
Read of size 4 at addr ffff888012113f30 by task syz-executor135/5106

CPU: 0 UID: 0 PID: 5106 Comm: syz-executor135 Not tainted 6.11.0-rc6-syzkaller-00019-g67784a74e258 #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:93 [inline]
 dump_stack_lvl+0x241/0x360 lib/dump_stack.c:119
 print_address_description mm/kasan/report.c:377 [inline]
 print_report+0x169/0x550 mm/kasan/report.c:488
 kasan_report+0x143/0x180 mm/kasan/report.c:601
 udf_get_filelongad+0x167/0x1b0 fs/udf/directory.c:526
 udf_current_aext+0x435/0x9e0 fs/udf/inode.c:2235
 udf_next_aext+0x8c/0x4a0 fs/udf/inode.c:2171
 udf_extend_file fs/udf/inode.c:677 [inline]
 udf_setsize+0xa8a/0x1280 fs/udf/inode.c:1265
 udf_setattr+0x3c7/0x5d0 fs/udf/file.c:236
 notify_change+0xbca/0xe90 fs/attr.c:503
 do_truncate fs/open.c:65 [inline]
 do_ftruncate+0x46b/0x590 fs/open.c:181
 do_sys_ftruncate fs/open.c:199 [inline]
 __do_sys_ftruncate fs/open.c:207 [inline]
 __se_sys_ftruncate fs/open.c:205 [inline]
 __x64_sys_ftruncate+0x95/0xf0 fs/open.c:205
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f13639ac249
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 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 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fff0302d508 EFLAGS: 00000246 ORIG_RAX: 000000000000004d
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f13639ac249
RDX: 00007f13639ac249 RSI: 0000008002007ffb RDI: 0000000000000005
RBP: 00000000000013f1 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff0302d550
R13: 00007fff0302d630 R14: 431bde82d7b634db R15: 00007f13639f501d

The root cause is:
  udf_extend_file
    ->inode_bmap       --> etype == -1 and epos.bh == NULL
     -> udf_next_aext  --> return -1 because reading block failed
       -> sb_read --> return NULL because access beyond end of device

Under this, etype == -1, epos.bh == NULL, epos.offset is 24, which is
less than sizeof(struct extentedFileEntry), aka 216. As a result,
it skipped the epos.bh check and goes into udf_next_aext(). Since the
epos.offset is illegal, udf_get_filelongad's first argument ptr,
	ptr = iinfo->i_data + epos->offset -
		udf_file_entry_alloc_offset(inode) +
		iinfo->i_lenEAttr;
points to some buffer before iinfo->i_data, which triggeres KASAN's
warnning.

The fix is to add addition check on etype, epos.bh and epos.offset,
when ftruncate accesses beyound end of device, just return EIO and failed.

Reported-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7a4842f0b1801230a989
Tested-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
---
 fs/udf/inode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 4726a4d014b6..66f73f728dae 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -660,6 +660,16 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
 	udf_discard_prealloc(inode);
 
 	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
+
+	/*
+	 * when ftruncate attempt to access beyond end of device, sb_read will
+	 * fail with epos.bh be null and return etype be -1, just return EIO.
+	 */
+	if (etype == -1 && !epos.bh && epos.offset == sizeof(struct allocExtDesc)) {
+		err = -EIO;
+		goto out;
+	}
+
 	within_last_ext = (etype != -1);
 	/* We don't expect extents past EOF... */
 	WARN_ON_ONCE(within_last_ext &&
-- 
2.43.0
Re: [PATCH v1] udf: return EIO when ftrunctate access beyond end of device
Posted by Jan Kara 2 months, 2 weeks ago 
On Thu 12-09-24 14:16:52, Zhao Mengmeng wrote:
> syzbot reports a udf slab-out-of-bounds as blow:
> 
> loop0: rw=0, sector=117, nr_sectors = 1 limit=0
> syz-executor135: attempt to access beyond end of device
> loop0: rw=0, sector=117, nr_sectors = 1 limit=0
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in udf_get_filelongad+0x167/0x1b0 fs/udf/directory.c:526
> Read of size 4 at addr ffff888012113f30 by task syz-executor135/5106
> 
> CPU: 0 UID: 0 PID: 5106 Comm: syz-executor135 Not tainted 6.11.0-rc6-syzkaller-00019-g67784a74e258 #0
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:93 [inline]
>  dump_stack_lvl+0x241/0x360 lib/dump_stack.c:119
>  print_address_description mm/kasan/report.c:377 [inline]
>  print_report+0x169/0x550 mm/kasan/report.c:488
>  kasan_report+0x143/0x180 mm/kasan/report.c:601
>  udf_get_filelongad+0x167/0x1b0 fs/udf/directory.c:526
>  udf_current_aext+0x435/0x9e0 fs/udf/inode.c:2235
>  udf_next_aext+0x8c/0x4a0 fs/udf/inode.c:2171
>  udf_extend_file fs/udf/inode.c:677 [inline]
>  udf_setsize+0xa8a/0x1280 fs/udf/inode.c:1265
>  udf_setattr+0x3c7/0x5d0 fs/udf/file.c:236
>  notify_change+0xbca/0xe90 fs/attr.c:503
>  do_truncate fs/open.c:65 [inline]
>  do_ftruncate+0x46b/0x590 fs/open.c:181
>  do_sys_ftruncate fs/open.c:199 [inline]
>  __do_sys_ftruncate fs/open.c:207 [inline]
>  __se_sys_ftruncate fs/open.c:205 [inline]
>  __x64_sys_ftruncate+0x95/0xf0 fs/open.c:205
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f13639ac249
> Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 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 b8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007fff0302d508 EFLAGS: 00000246 ORIG_RAX: 000000000000004d
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f13639ac249
> RDX: 00007f13639ac249 RSI: 0000008002007ffb RDI: 0000000000000005
> RBP: 00000000000013f1 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff0302d550
> R13: 00007fff0302d630 R14: 431bde82d7b634db R15: 00007f13639f501d
> 
> The root cause is:
>   udf_extend_file
>     ->inode_bmap       --> etype == -1 and epos.bh == NULL
>      -> udf_next_aext  --> return -1 because reading block failed
>        -> sb_read --> return NULL because access beyond end of device
> 
> Under this, etype == -1, epos.bh == NULL, epos.offset is 24, which is
> less than sizeof(struct extentedFileEntry), aka 216. As a result,
> it skipped the epos.bh check and goes into udf_next_aext(). Since the
> epos.offset is illegal, udf_get_filelongad's first argument ptr,
> 	ptr = iinfo->i_data + epos->offset -
> 		udf_file_entry_alloc_offset(inode) +
> 		iinfo->i_lenEAttr;
> points to some buffer before iinfo->i_data, which triggeres KASAN's
> warnning.
> 
> The fix is to add addition check on etype, epos.bh and epos.offset,
> when ftruncate accesses beyound end of device, just return EIO and failed.
> 
> Reported-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7a4842f0b1801230a989
> Tested-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

Thanks for the analysis and the patch! I agree this will fix the problem
for the particular syzbot reproducer but I think a proper solution needs a
bit more. I think we need udf_current_aext() and udf_next_aext() to
differentiate between error and "hit EOF" - I think they can take pointer
to etype to store the extent type and just return 0 on success, <0 on
error. Then inode_bmap() can do the same and udf_extend_file() can properly
detect errors and bail out without resorting to checking for particular
offsets and assuming internal behavior of these functions. Will you be
interested in looking into this?

								Honza


> ---
>  fs/udf/inode.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 4726a4d014b6..66f73f728dae 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -660,6 +660,16 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>  	udf_discard_prealloc(inode);
>  
>  	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
> +
> +	/*
> +	 * when ftruncate attempt to access beyond end of device, sb_read will
> +	 * fail with epos.bh be null and return etype be -1, just return EIO.
> +	 */
> +	if (etype == -1 && !epos.bh && epos.offset == sizeof(struct allocExtDesc)) {
> +		err = -EIO;
> +		goto out;
> +	}
> +
>  	within_last_ext = (etype != -1);
>  	/* We don't expect extents past EOF... */
>  	WARN_ON_ONCE(within_last_ext &&
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
Re: [PATCH v1] udf: return EIO when ftrunctate access beyond end of device
Posted by Zhao Mengmeng 2 months, 2 weeks ago 
On 2024/9/12 19:12, Jan Kara wrote:
> On Thu 12-09-24 14:16:52, Zhao Mengmeng wrote:
>> syzbot reports a udf slab-out-of-bounds as blow:
>>
>> loop0: rw=0, sector=117, nr_sectors = 1 limit=0
>> syz-executor135: attempt to access beyond end of device
>> loop0: rw=0, sector=117, nr_sectors = 1 limit=0
>> ==================================================================
>> BUG: KASAN: slab-out-of-bounds in udf_get_filelongad+0x167/0x1b0 fs/udf/directory.c:526
>> Read of size 4 at addr ffff888012113f30 by task syz-executor135/5106
>>
>> CPU: 0 UID: 0 PID: 5106 Comm: syz-executor135 Not tainted 6.11.0-rc6-syzkaller-00019-g67784a74e258 #0
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
>> Call Trace:
>>  <TASK>
>>  __dump_stack lib/dump_stack.c:93 [inline]
>>  dump_stack_lvl+0x241/0x360 lib/dump_stack.c:119
>>  print_address_description mm/kasan/report.c:377 [inline]
>>  print_report+0x169/0x550 mm/kasan/report.c:488
>>  kasan_report+0x143/0x180 mm/kasan/report.c:601
>>  udf_get_filelongad+0x167/0x1b0 fs/udf/directory.c:526
>>  udf_current_aext+0x435/0x9e0 fs/udf/inode.c:2235
>>  udf_next_aext+0x8c/0x4a0 fs/udf/inode.c:2171
>>  udf_extend_file fs/udf/inode.c:677 [inline]
>>  udf_setsize+0xa8a/0x1280 fs/udf/inode.c:1265
>>  udf_setattr+0x3c7/0x5d0 fs/udf/file.c:236
>>  notify_change+0xbca/0xe90 fs/attr.c:503
>>  do_truncate fs/open.c:65 [inline]
>>  do_ftruncate+0x46b/0x590 fs/open.c:181
>>  do_sys_ftruncate fs/open.c:199 [inline]
>>  __do_sys_ftruncate fs/open.c:207 [inline]
>>  __se_sys_ftruncate fs/open.c:205 [inline]
>>  __x64_sys_ftruncate+0x95/0xf0 fs/open.c:205
>>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>> RIP: 0033:0x7f13639ac249
>> Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 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 b8 ff ff ff f7 d8 64 89 01 48
>> RSP: 002b:00007fff0302d508 EFLAGS: 00000246 ORIG_RAX: 000000000000004d
>> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f13639ac249
>> RDX: 00007f13639ac249 RSI: 0000008002007ffb RDI: 0000000000000005
>> RBP: 00000000000013f1 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff0302d550
>> R13: 00007fff0302d630 R14: 431bde82d7b634db R15: 00007f13639f501d
>>
>> The root cause is:
>>   udf_extend_file
>>     ->inode_bmap       --> etype == -1 and epos.bh == NULL
>>      -> udf_next_aext  --> return -1 because reading block failed
>>        -> sb_read --> return NULL because access beyond end of device
>>
>> Under this, etype == -1, epos.bh == NULL, epos.offset is 24, which is
>> less than sizeof(struct extentedFileEntry), aka 216. As a result,
>> it skipped the epos.bh check and goes into udf_next_aext(). Since the
>> epos.offset is illegal, udf_get_filelongad's first argument ptr,
>> 	ptr = iinfo->i_data + epos->offset -
>> 		udf_file_entry_alloc_offset(inode) +
>> 		iinfo->i_lenEAttr;
>> points to some buffer before iinfo->i_data, which triggeres KASAN's
>> warnning.
>>
>> The fix is to add addition check on etype, epos.bh and epos.offset,
>> when ftruncate accesses beyound end of device, just return EIO and failed.
>>
>> Reported-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=7a4842f0b1801230a989
>> Tested-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> Thanks for the analysis and the patch! I agree this will fix the problem
> for the particular syzbot reproducer but I think a proper solution needs a
> bit more. I think we need udf_current_aext() and udf_next_aext() to
> differentiate between error and "hit EOF" - I think they can take pointer
> to etype to store the extent type and just return 0 on success, <0 on
> error. Then inode_bmap() can do the same and udf_extend_file() can properly
> detect errors and bail out without resorting to checking for particular
> offsets and assuming internal behavior of these functions. Will you be
> interested in looking into this?
>
> 								Honza
>
>
>> ---
>>  fs/udf/inode.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
>> index 4726a4d014b6..66f73f728dae 100644
>> --- a/fs/udf/inode.c
>> +++ b/fs/udf/inode.c
>> @@ -660,6 +660,16 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>>  	udf_discard_prealloc(inode);
>>  
>>  	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
>> +
>> +	/*
>> +	 * when ftruncate attempt to access beyond end of device, sb_read will
>> +	 * fail with epos.bh be null and return etype be -1, just return EIO.
>> +	 */
>> +	if (etype == -1 && !epos.bh && epos.offset == sizeof(struct allocExtDesc)) {
>> +		err = -EIO;
>> +		goto out;
>> +	}
>> +
>>  	within_last_ext = (etype != -1);
>>  	/* We don't expect extents past EOF... */
>>  	WARN_ON_ONCE(within_last_ext &&
>> -- 
>> 2.43.0
>>
Thanks for the review!  You suggestion is great and I am glad to have a try.
ps: I am new on udf module, so this may takes some time, wishes finishing it ASAP.:)

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.