Re: [PATCH] f2fs: Fix slab-out-of-bounds Read KASAN bug in f2fs_getxattr()

[Chao Yu <chao@kernel.org>]

On 1/9/25 00:23, qasdev wrote:
> On Wed, Jan 08, 2025 at 07:44:03PM +0800, Chao Yu wrote:
>> Hi Qasim,
>>
>> On 2025/1/8 07:03, qasdev wrote:
>>> In f2fs_getxattr(), the function lookup_all_xattrs() allocates a 12-byte
>>> (base_size) buffer for an inline extended attribute. However, when
>>> __find_inline_xattr() calls __find_xattr(), it uses the macro
>>> "list_for_each_xattr(entry, addr)", which starts by calling
>>> XATTR_FIRST_ENTRY(addr). This skips a 24-byte struct f2fs_xattr_header
>>> at the beginning of the buffer, causing an immediate out-of-bounds read
>>> in a 12-byte allocation. The subsequent !IS_XATTR_LAST_ENTRY(entry)
>>> check then dereferences memory outside the allocated region, triggering
>>> the slab-out-of bounds read.
>>>
>>> This patch prevents the out-of-bounds read by adding a check to bail
>>> out early if inline_size is too small and does not account for the
>>> header plus the 4-byte value that IS_XATTR_LAST_ENTRY reads.
>>
>> Thank you very much for analyzing this issue, the root cause you figured out
>> makes sense to me.
>>
>> Can you please check the patch in below link? It seems it can fix this issue
>> as well? IIUC.
>>
>> https://lore.kernel.org/linux-f2fs-devel/20241216134600.8308-1-chao@kernel.org/
>>
>> Thanks,
> 
> Hi Chao,
> 
> I tested the patch you linked on my machine and with syzbot, and both tests succeeded. The patch you linked works very well.

Hi Qasdev,

Thanks for the test!

>   
> Here is the link to the results of the testing of both patches: https://syzkaller.appspot.com/bug?extid=f5e74075e096e757bdbf
> 
> Would it be possible to include me in the Tested-by header and any other contribution acknowledgments you feel appropriate?
 > > Thanks!
> 
> Best regards,
> Qasim
> 
>>
>>>
>>> Reported-by: syzbot <syzbot+f5e74075e096e757bdbf@syzkaller.appspotmail.com>
>>> Closes: https://syzkaller.appspot.com/bug?extid=f5e74075e096e757bdbf
>>> Tested-by: syzbot <syzbot+f5e74075e096e757bdbf@syzkaller.appspotmail.com>
>>> Tested-by: Qasim Ijaz <qasdev00@gmail.com>

IMO, it will be better to quoted your comment description and all above 
tags into the patch, what do you think?

Thanks,

>>> Fixes: 388a2a0640e1 ("f2fs: remove redundant sanity check in sanity_check_inode()")
>>> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
>>> ---
>>>    fs/f2fs/xattr.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>> index 3f3874943679..cf82646bca0e 100644
>>> --- a/fs/f2fs/xattr.c
>>> +++ b/fs/f2fs/xattr.c
>>> @@ -329,6 +329,9 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
>>>    	if (!xnid && !inline_size)
>>>    		return -ENODATA;
>>> +	if (inline_size < sizeof(struct f2fs_xattr_header) + sizeof(__u32))
>>> +		return -ENODATA;
>>> +
>>>    	*base_size = XATTR_SIZE(inode) + XATTR_PADDING_SIZE;
>>>    	txattr_addr = xattr_alloc(F2FS_I_SB(inode), *base_size, is_inline);
>>>    	if (!txattr_addr)
>>