Date:   Sat, 11 Mar 2023 10:46:54 +0530
From:   Siddh Raman Pant <code@siddh.me>
To:     "Ivan Orlov" <ivan.orlov0322@gmail.com>
Cc:     "ericvh" <ericvh@gmail.com>, "lucho" <lucho@ionkov.net>,
        "asmadeus" <asmadeus@codewreck.org>,
        "linux_oss" <linux_oss@crudebyte.com>,
        "linux-kernel" <linux-kernel@vger.kernel.org>,
        "syzbot+cb1d16facb3cc90de5fb" 
        <syzbot+cb1d16facb3cc90de5fb@syzkaller.appspotmail.com>,
        "v9fs-developer" <v9fs-developer@lists.sourceforge.net>,
        "linux-kernel-mentees" 
        <linux-kernel-mentees@lists.linuxfoundation.org>
Message-ID: <186cf19b619.4777c80c154603.5258165448157616593@siddh.me>
In-Reply-To: <20230310202619.433269-1-ivan.orlov0322@gmail.com>
References: <20230310202619.433269-1-ivan.orlov0322@gmail.com>
Subject: Re: [PATCH] 9P FS: Fix wild-memory-access write in v9fs_get_acl
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Importance: Medium
User-Agent: Zoho Mail
Precedence: bulk

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@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659
> Signed-off-by: Ivan Orlov ivan.orlov0322@gmail.com>

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@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees