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

[qasdev <qasdev00@gmail.com>]

On Thu, Jan 09, 2025 at 05:02:02PM +0800, Chao Yu wrote:
> 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,

Hi Chao,

Thank you for the suggestion. I agree that quoting my comment description and tags into the patch would provide helpful context.

Please feel free to include them as appropriate. Let me know if you need anything else from me.

Best regards,
Qasim

> 
> > > > 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)
> > > 
>