Skip to contentSkip to conversation

9P FS: Fix wild-memory-access write in v9fs_get_acl

PATCH Superseded Ivan Orlov 4 participants 7 messages Mar 10, 2023 – Mar 12, 2023 9P file system

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Ivan Orlov

author
#0 lore raw

PATCH 1/1 9P FS: Fix wild-memory-access write in v9fs_get_acl

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(-)
fs/9p/xattr.c +1 -1
index 50f7f3f6b55e..d6f7450107a8 100644
@@ -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;
Signature · 6 lines
2.34.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
#1 lore raw
Quoted 24 lines · Ivan Orlov

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

Quoted 7 lines

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

Quoted 9 lines
---
 fs/9p/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/9p/xattr.c +0 -0
index 50f7f3f6b55e..d6f7450107a8 100644

fs/9p/xattr.c ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name, · line 41

Quoted 8 lines
fs/9p/xattr.c +1 -1
@@ -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

Quoted 7 lines
Signature · 6 lines
2.34.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
author
#2 lore raw
Quoted 59 lines · Siddh Raman Pant
Quoted 24 lines · Ivan Orlov

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

Quoted 7 lines

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

Quoted 9 lines
---
  fs/9p/xattr.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
fs/9p/xattr.c +0 -0
index 50f7f3f6b55e..d6f7450107a8 100644

fs/9p/xattr.c ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name, · line 41

Quoted 8 lines
fs/9p/xattr.c +1 -1
@@ -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.

Quoted 10 lines

Thanks,
Siddh

Quoted 7 lines
Signature · 6 lines
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.
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

#3 lore raw
In reply to Ivan Orlov
Quoted 78 lines · Ivan Orlov

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
<https://syzkaller.appspot.com/bug?id=fbbef66d9e4d096242f3617de5d14d12705b4659Signed-off-by>:
Ivan Orlov <[email protected]>

---
 fs/9p/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/9p/xattr.c +1 -1
index 50f7f3f6b55e..d6f7450107a8 100644
@@ -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) /*
Quoted 2 lines

request to get the attr_size */

I'm not sure what are the rules around here but I prefer to use brackets more generously.

Quoted 11 lines
                        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

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

author
#4 lore raw
Quoted 2 lines

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.
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

#5 lore raw
Quoted 8 lines · Ivan Orlov
Quoted 3 lines

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.

Quoted 4 lines

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

#6 lore raw
In reply to Ivan Orlov
Quoted 66 lines · Ivan Orlov

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(-)
fs/9p/xattr.c +0 -0
index 50f7f3f6b55e..d6f7450107a8 100644

fs/9p/xattr.c ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name, · line 38

Quoted 5 lines
fs/9p/xattr.c +1 -1
@@ -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.

Quoted 11 lines
fs/9p/xattr.c +0 -1
                        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

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees