Re: [PATCH] 9P FS: Fix wild-memory-access write in v9fs_get_acl

[Ivan Orlov]

On 3/11/23 09:16, Siddh Raman Pant wrote:
> On Sat, 11 Mar 2023 01:56:19 +0530, Ivan Orlov wrote:
> > KASAN reported the following issue:
> > [...]
> >
> > Calling '__v9fs_get_acl' method in 'v9fs_get_acl' creates the
> > following chain of function calls:
> >
> > __v9fs_get_acl
> >        v9fs_fid_get_acl
> >                v9fs_fid_xattr_get
> >                        p9_client_xattrwalk
> >
> > Function p9_client_xattrwalk accepts a pointer to u64-typed
> > variable attr_size and puts some u64 value into it. However,
> > after the executing the p9_client_xattrwalk, in some circumstances
> > we assign the value of u64-typed variable 'attr_size' to the
> > variable 'retval', which we will return. However, the type of
> > 'retval' is ssize_t, and if the value of attr_size is larger
> > than SSIZE_MAX, we will face the signed type overflow. If the
> > overflow occurs, the result of v9fs_fid_xattr_get may be
> > negative, but not classified as an error. When we try to allocate
> > an acl with 'broken' size we receive an error, but don't process
> > it. When we try to free this acl, we face the 'wild-memory-access'
> > error (because it wasn't allocated).
> >
> > This patch will modify the condition in the 'v9fs_fid_xattr_get'
> > function, so it will return an error if the 'attr_size' is larger
> > than SSIZE_MAX.
> >
> > Reported-by: syzbot+cb1d16facb3cc90de5fb@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Link: https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659
> > Signed-off-by: Ivan Orlov ivan.orlov0322@xxxxxxxxx>
>
> You should also test with Syzkaller if it gave a reproducer.
> Check the following docs to know about it:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
>
> > ---
> >   fs/9p/xattr.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
> > index 50f7f3f6b55e..d6f7450107a8 100644
> > --- a/fs/9p/xattr.c
> > +++ b/fs/9p/xattr.c
> > @@ -35,7 +35,7 @@ ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name,
> >   		return retval;
> >   	}
> >   	if (attr_size > buffer_size) {
> > -		if (!buffer_size) /* request to get the attr_size */
> > +		if (!buffer_size && attr_size <= (u64) SSIZE_MAX) /* request to get the attr_size */
> >   			retval = attr_size;
> >   		else
> >   			retval = -ERANGE;
>
> You should use EOVERFLOW for overflow error. Make a new conditional
> instead of using AND. Also, the explicit u64 cast for SSIZE_MAX can
> be dropped for better readability.
>
> Thanks,
> Siddh
>
> > --
> > 2.34.1
> >
> > _______________________________________________
> > Linux-kernel-mentees mailing list
> > Linux-kernel-mentees@xxxxxxxxxxxxxxxxxxxxxxxxx
> > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Thank you for the review! I will modify the patch according to your 
comments, resend it as v2 version and test it via syzkaller.