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

[Chao Yu]

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@xxxxxxxxxx/
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxx>
> > > Closes: https://syzkaller.appspot.com/bug?extid=f5e74075e096e757bdbf
> > > Tested-by: syzbot <syzbot+f5e74075e096e757bdbf@xxxxxxxxxxxxxxxxxxxxxxxxx>
> > > Tested-by: Qasim Ijaz <qasdev00@xxxxxxxxx>

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