KASAN reported the following issue:
[ 36.825817][ T5923] BUG: KASAN: wild-memory-access in v9fs_get_acl+0x1a4/0x390
[ 36.827479][ T5923] Write of size 4 at addr 9fffeb37f97f1c00 by task syz-executor798/5923
[ 36.829303][ T5923]
[ 36.829846][ T5923] CPU: 0 PID: 5923 Comm: syz-executor798 Not tainted 6.2.0-syzkaller-18302-g596b6b709632 #0
[ 36.832110][ T5923] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/21/2023
[ 36.834464][ T5923] Call trace:
[ 36.835196][ T5923] dump_backtrace+0x1c8/0x1f4
[ 36.836229][ T5923] show_stack+0x2c/0x3c
[ 36.837100][ T5923] dump_stack_lvl+0xd0/0x124
[ 36.838103][ T5923] print_report+0xe4/0x4c0
[ 36.839068][ T5923] kasan_report+0xd4/0x130
[ 36.840052][ T5923] kasan_check_range+0x264/0x2a4
[ 36.841199][ T5923] __kasan_check_write+0x2c/0x3c
[ 36.842216][ T5923] v9fs_get_acl+0x1a4/0x390
[ 36.843232][ T5923] v9fs_mount+0x77c/0xa5c
[ 36.844163][ T5923] legacy_get_tree+0xd4/0x16c
[ 36.845173][ T5923] vfs_get_tree+0x90/0x274
[ 36.846137][ T5923] do_new_mount+0x25c/0x8c8
[ 36.847066][ T5923] path_mount+0x590/0xe58
[ 36.848147][ T5923] __arm64_sys_mount+0x45c/0x594
[ 36.849273][ T5923] invoke_syscall+0x98/0x2c0
[ 36.850421][ T5923] el0_svc_common+0x138/0x258
[ 36.851397][ T5923] do_el0_svc+0x64/0x198
[ 36.852398][ T5923] el0_svc+0x58/0x168
[ 36.853224][ T5923] el0t_64_sync_handler+0x84/0xf0
[ 36.854293][ T5923] el0t_64_sync+0x190/0x194
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: [email protected]
Link: https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659
Signed-off-by: Ivan Orlov <[email protected]>
---
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;
--
2.34.1
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: [email protected]
> Link: https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659
> Signed-off-by: Ivan Orlov [email protected]>
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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
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: [email protected]
>> Link: https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659
>> Signed-off-by: Ivan Orlov [email protected]>
>
> 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
>> [email protected]
>> 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.
On Fri, Mar 10, 2023 at 11:28 PM Ivan Orlov <[email protected]> wrote:
>
> KASAN reported the following issue:
> [ 36.825817][ T5923] BUG: KASAN: wild-memory-access in v9fs_get_acl+0x1a4/0x390
> [ 36.827479][ T5923] Write of size 4 at addr 9fffeb37f97f1c00 by task syz-executor798/5923
> [ 36.829303][ T5923]
> [ 36.829846][ T5923] CPU: 0 PID: 5923 Comm: syz-executor798 Not tainted 6.2.0-syzkaller-18302-g596b6b709632 #0
> [ 36.832110][ T5923] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/21/2023
> [ 36.834464][ T5923] Call trace:
> [ 36.835196][ T5923] dump_backtrace+0x1c8/0x1f4
> [ 36.836229][ T5923] show_stack+0x2c/0x3c
> [ 36.837100][ T5923] dump_stack_lvl+0xd0/0x124
> [ 36.838103][ T5923] print_report+0xe4/0x4c0
> [ 36.839068][ T5923] kasan_report+0xd4/0x130
> [ 36.840052][ T5923] kasan_check_range+0x264/0x2a4
> [ 36.841199][ T5923] __kasan_check_write+0x2c/0x3c
> [ 36.842216][ T5923] v9fs_get_acl+0x1a4/0x390
> [ 36.843232][ T5923] v9fs_mount+0x77c/0xa5c
> [ 36.844163][ T5923] legacy_get_tree+0xd4/0x16c
> [ 36.845173][ T5923] vfs_get_tree+0x90/0x274
> [ 36.846137][ T5923] do_new_mount+0x25c/0x8c8
> [ 36.847066][ T5923] path_mount+0x590/0xe58
> [ 36.848147][ T5923] __arm64_sys_mount+0x45c/0x594
> [ 36.849273][ T5923] invoke_syscall+0x98/0x2c0
> [ 36.850421][ T5923] el0_svc_common+0x138/0x258
> [ 36.851397][ T5923] do_el0_svc+0x64/0x198
> [ 36.852398][ T5923] el0_svc+0x58/0x168
> [ 36.853224][ T5923] el0t_64_sync_handler+0x84/0xf0
> [ 36.854293][ T5923] el0t_64_sync+0x190/0x194
>
> 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: [email protected]
> Link: https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659
> Signed-off-by: Ivan Orlov <[email protected]>
> ---
> 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 */
I'm not sure what are the rules around here but I prefer to use
brackets more generously.
>
> retval = attr_size;
> else
> retval = -ERANGE;
> --
> 2.34.1
>
> _______________________________________________
> Linux-kernel-mentees mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
> I'm not sure what are the rules around here but I prefer to use brackets
> more generously.
I think in this particular case they can just make the code less
readable (considering they will not change the behavior in any way).
However, this is eternal discussion and I saw examples of both in the
kernel sources.
On Sat Mar 11, 2023 at 6:35 PM CET, Ivan Orlov wrote:
>
> > I'm not sure what are the rules around here but I prefer to use brackets
> > more generously.
>
> I think in this particular case they can just make the code less
> readable (considering they will not change the behavior in any way).
> However, this is eternal discussion and I saw examples of both in the
> kernel sources.
Maybe this can be a good reading [1], I think it is more a kernel rule?
[1] https://www.kernel.org/doc/html/latest/process/coding-style.html
Cheers
Vincent.
> _______________________________________________
> Linux-kernel-mentees mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees