Re: [PATCH] hfs: Fix OOB Write in hfs_asc2mac

["zhangpeng (AS)" <zhangpeng362@huawei.com>]

On 2022/11/30 3:08, Viacheslav Dubeyko wrote:
>> On Nov 28, 2022, at 6:23 PM, zhangpeng (AS) <zhangpeng362@huawei.com> wrote:
>>
>> On 2022/11/29 3:29, Viacheslav Dubeyko wrote:
>>>> On Nov 25, 2022, at 8:36 PM, Peng Zhang <zhangpeng362@huawei.com> wrote:
>>>>
>>>> From: ZhangPeng <zhangpeng362@huawei.com>
>>>>
>>>> Syzbot reported a OOB Write bug:
>>>>
>>>> loop0: detected capacity change from 0 to 64
>>>> ==================================================================
>>>> BUG: KASAN: slab-out-of-bounds in hfs_asc2mac+0x467/0x9a0
>>>> fs/hfs/trans.c:133
>>>> Write of size 1 at addr ffff88801848314e by task syz-executor391/3632
>>>>
>>>> Call Trace:
>>>> <TASK>
>>>> __dump_stack lib/dump_stack.c:88 [inline]
>>>> dump_stack_lvl+0x1b1/0x28e lib/dump_stack.c:106
>>>> print_address_description+0x74/0x340 mm/kasan/report.c:284
>>>> print_report+0x107/0x1f0 mm/kasan/report.c:395
>>>> kasan_report+0xcd/0x100 mm/kasan/report.c:495
>>>> hfs_asc2mac+0x467/0x9a0 fs/hfs/trans.c:133
>>>> hfs_cat_build_key+0x92/0x170 fs/hfs/catalog.c:28
>>>> hfs_lookup+0x1ab/0x2c0 fs/hfs/dir.c:31
>>>> lookup_open fs/namei.c:3391 [inline]
>>>> open_last_lookups fs/namei.c:3481 [inline]
>>>> path_openat+0x10e6/0x2df0 fs/namei.c:3710
>>>> do_filp_open+0x264/0x4f0 fs/namei.c:3740
>>>>
>>>> If in->len is much larger than HFS_NAMELEN(31) which is the maximum
>>>> length of an HFS filename, a OOB Write could occur in hfs_asc2mac(). In
>>>> that case, when the dst reaches the boundary, the srclen is still
>>>> greater than 0, which causes a OOB Write.
>>>> Fix this by adding a Check on dstlen before Writing to dst address.
>>>>
>>>> Fixes: 328b92278650 ("[PATCH] hfs: NLS support")
>>>> Reported-by: syzbot+dc3b1cf9111ab5fe98e7@syzkaller.appspotmail.com
>>>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>>>> ---
>>>> fs/hfs/trans.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/hfs/trans.c b/fs/hfs/trans.c
>>>> index 39f5e343bf4d..886158db07b3 100644
>>>> --- a/fs/hfs/trans.c
>>>> +++ b/fs/hfs/trans.c
>>>> @@ -130,6 +130,8 @@ void hfs_asc2mac(struct super_block *sb, struct hfs_name *out, const struct qstr
>>>> 				dst += size;
>>>> 				dstlen -= size;
>>>> 			} else {
>>>> +				if (dstlen == 0)
>>>> +					goto out;
>>> Maybe, it makes sense to use dstlen instead of srclen in while()?
>>>
>>> We have now:
>>>
>>> while (srclen > 0) {
>>>     <skipped>
>>> } else {
>>>     <skipped>
>>> }
>>>
>>> We can use instead:
>>>
>>> while (dstlen > 0) {
>>>     <skipped>
>>> } else {
>>>     <skipped>
>>> }
>>>
>>> Will it fix the issue?
>>>
>>> Thanks,
>>> Slava.
>> Thank you for your help.
>>
>> After testing, it fix the issue.
>> Would it be better to add dstlen > 0 instead of replacing srclen > 0 with dstlen > 0?
>> Because there may be dstlen > 0 and srclen <= 0.
>>
>> we can use:
>>
>> while (srclen > 0 && dstlen > 0) {
>>    <skipped>
>> } else {
>>    <skipped>
>> }
>>
> Looks good to me.

Can I put you down as a Reviewed-by or Suggested-by?

Thanks,
Zhang Peng

> Thanks,
> Slava.
>
>> Thanks,
>> Zhang Peng
>>
>>>> 				*dst++ = ch > 0xff ? '?' : ch;
>>>> 				dstlen--;
>>>> 			}
>>>> -- 
>>>> 2.25.1