Hello,
I am sending this patch for inclusion in the stable tree, as it fixes a critical stack-out-of-bounds bug in the cifs module related to the `smb2_set_next_command()` function.
Problem Summary: A problem was observed in the `statfs` system call for cifs, where it failed with a "Resource temporarily unavailable" message. Further investigation with KASAN revealed a stack-out-of-bounds error. The root cause was a miscalculation of the size of the `smb2_query_info_req` structure in the `SMB2_query_info_init()` function.
This situation arose due to a dependency on a prior commit (`eb3e28c1e89b`) that replaced a 1-element array with a flexible array member in the `smb2_query_info_req` structure. This commit was not backported to the 5.10.y and 5.15.y stable branch, leading to an incorrect size calculation after the backport of commit `33eae65c6f49`.
Fix Details: The patch corrects the size calculation to ensure the correct length is used when initializing the `smb2_query_info_req` structure. It has been tested and confirmed to resolve the issue without introducing any regressions.
Maybe the prior commit eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays") should be backported to solve this problem directly. The patch does not seem to conflict.
Best regards, ZhaoLong Wang
ZhaoLong Wang (1): cifs: Fix stack-out-of-bounds in smb2_set_next_command()
fs/cifs/smb2pdu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
After backporting the mainline commit 33eae65c6f49 ("smb: client: fix OOB in SMB2_query_info_init()") to the linux-5.10.y stable branch, an issue arose where the cifs statfs system call failed, resulting in:
$ df /mnt df: /mnt: Resource temporarily unavailable
KASAN also reported a stack-out-of-bounds error as follows:
================================================================== BUG: KASAN: stack-out-of-bounds in smb2_set_next_command+0x247/0x280 [cifs] Write of size 8 at addr ffff8881073ef830 by task df/533
CPU: 4 PID: 533 Comm: df Not tainted 5.10.0+ #17 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2. fc37 04/01/2014 Call Trace: dump_stack+0xb3/0xf1 print_address_description.constprop.0+0x1e/0x280 __kasan_report.cold+0x6c/0x84 kasan_report+0x3a/0x50 smb2_set_next_command+0x247/0x280 [cifs] smb2_query_info_compound+0x3e9/0x5d0 [cifs] smb2_queryfs+0xb9/0x180 [cifs] smb311_queryfs+0x218/0x230 [cifs] cifs_statfs+0x161/0x340 [cifs] statfs_by_dentry+0xa8/0x100 vfs_statfs+0x2f/0x180 user_statfs+0x96/0x100 __se_sys_statfs+0x6a/0xc0 do_syscall_64+0x33/0x40 entry_SYSCALL_64_after_hwframe+0x62/0xc7 RIP: 0033:0x7ff427ad93b7 Code: ff ff ff ff c3 66 0f 1f 44 00 00 48 8b 05 59 ba 0d 00 64 c7 00 16 00 00 00 b8 ff ff ff ff c3 0f 1f 40 00 b8 89 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 01 c3 48 8b 15 31 ba 0d 00 f7 d8 64 89 8 RSP: 002b:00007ffd8371e158 EFLAGS: 00000246 ORIG_RAX: 0000000000000089 RAX: ffffffffffffffda RBX: 00007ffd8371e200 RCX: 00007ff427ad93b7 RDX: 0000000000000003 RSI: 00007ffd8371e160 RDI: 00007ffd8371ee4b RBP: 00007ffd8371e160 R08: 00007ffd8371e283 R09: 0000000000000032 R10: 00007ff4279ed368 R11: 0000000000000246 R12: 00007ffd8371e200 R13: 000056296a125dd0 R14: 0000000000000001 R15: 0000000000000000
The buggy address belongs to the page: page:00000000862dac80 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1073ef flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) raw: 0017ffffc0000000 0000000000000000 dead000000000122 0000000000000000 raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000 page dumped because: kasan: bad access detected
addr ffff8881073ef830 is located in stack of task df/533 at offset 112 in frame: smb2_query_info_compound+0x0/0x5d0 [cifs]
this frame has 9 objects: [48, 49) 'oplock' [64, 76) 'resp_buftype' [96, 112) 'qi_iov' [128, 144) 'close_iov' [160, 208) 'rsp_iov' [240, 296) 'oparms' [336, 456) 'rqst' [496, 624) 'open_iov' [656, 736) 'fid'
Memory state around the buggy address: ffff8881073ef700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff8881073ef780: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 f1 f1 01 f2
ffff8881073ef800: 00 04 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 00 00
^ ffff8881073ef880: 00 00 f2 f2 f2 f2 00 00 00 00 00 00 00 f2 f2 f2 ffff8881073ef900: f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ================================================================== Disabling lock debugging due to kernel taint
This issue was caused because the stable branch did not include the prerequisite patch eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays"). The patch replaces the trailing 1-element array with a flexible array in the smb2_query_info_req structure and modifies the length calculation expression from `sizeof(smb2_query_info_req) - 1` to `sizeof(smb2_query_info_req)`. Consequently, backporting only commit 33eae65c6f49 led to an incorrect length calculation for the smb2_query_info_req structure within SMB2_query_info_init().
cifs_statfs smb2_queryfs smb2_query_info_compound struct kvec qi_iov[1]; rqst[1].rq_iov = qi_iov; rqst[1].rq_nvec = 1; SMB2_query_info_init # The length of len is incorrect because the value of sizeof(req) # is not decreased by 1. check_add_overflow(input_len, sizeof(*req), &len) smb2_set_next_command(tcon, &rqst[1]); # 1 byte greater than the actual length len = smb_rqst_len(server, rqst); # 'len' is not 8-byte aligned, then paddingg. # Access to .rq_iov[1] results in out-of-bounds array rqst->rq_iov[rqst->rq_nvec].iov_base = smb2_padding; compound_send_recv
cifs_demultiplex_thread cifs_read_from_socket cifs_readv_from_socket # The request may be invalid and no expected response. length = sock_recvmsg
This patch corrects the length calculation for the smb2_query_info_req structure in SMB2_query_info_init() to address this problem.
Signed-off-by: ZhaoLong Wang wangzhaolong1@huawei.com --- fs/cifs/smb2pdu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 76679dc4e632..8532cc416188 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -3354,7 +3354,7 @@ SMB2_query_info_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server, size_t len; int rc;
- if (unlikely(check_add_overflow(input_len, sizeof(*req), &len) || + if (unlikely(check_add_overflow(input_len, sizeof(*req) - 1, &len) || len > CIFSMaxBufSize)) return -EINVAL;
Hi,
On Wed, Feb 07, 2024 at 07:52:50PM +0800, ZhaoLong Wang wrote:
Hello,
I am sending this patch for inclusion in the stable tree, as it fixes a critical stack-out-of-bounds bug in the cifs module related to the `smb2_set_next_command()` function.
Problem Summary: A problem was observed in the `statfs` system call for cifs, where it failed with a "Resource temporarily unavailable" message. Further investigation with KASAN revealed a stack-out-of-bounds error. The root cause was a miscalculation of the size of the `smb2_query_info_req` structure in the `SMB2_query_info_init()` function.
This situation arose due to a dependency on a prior commit (`eb3e28c1e89b`) that replaced a 1-element array with a flexible array member in the `smb2_query_info_req` structure. This commit was not backported to the 5.10.y and 5.15.y stable branch, leading to an incorrect size calculation after the backport of commit `33eae65c6f49`.
Fix Details: The patch corrects the size calculation to ensure the correct length is used when initializing the `smb2_query_info_req` structure. It has been tested and confirmed to resolve the issue without introducing any regressions.
Maybe the prior commit eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays") should be backported to solve this problem directly. The patch does not seem to conflict.
It looks there are several people working on the very same problem addint patches right now on top.
See as well https://lore.kernel.org/stable/c4c2f990-20cf-4126-95bd-d14c58e85042@oracle.c...
But this is already worked on and the proper solution is to only the eb3e28c1e89b backport included?
See as well https://lore.kernel.org/regressions/Zb5eL-AKcZpmvYSl@eldamar.lan/ and following.
And this needs to be done consistently for the 5.10.y and 5.15.y series.
Regards, Salvatore
On Wed, Feb 07, 2024 at 04:11:24PM +0100, Salvatore Bonaccorso wrote:
Hi,
On Wed, Feb 07, 2024 at 07:52:50PM +0800, ZhaoLong Wang wrote:
Hello,
I am sending this patch for inclusion in the stable tree, as it fixes a critical stack-out-of-bounds bug in the cifs module related to the `smb2_set_next_command()` function.
Problem Summary: A problem was observed in the `statfs` system call for cifs, where it failed with a "Resource temporarily unavailable" message. Further investigation with KASAN revealed a stack-out-of-bounds error. The root cause was a miscalculation of the size of the `smb2_query_info_req` structure in the `SMB2_query_info_init()` function.
This situation arose due to a dependency on a prior commit (`eb3e28c1e89b`) that replaced a 1-element array with a flexible array member in the `smb2_query_info_req` structure. This commit was not backported to the 5.10.y and 5.15.y stable branch, leading to an incorrect size calculation after the backport of commit `33eae65c6f49`.
Fix Details: The patch corrects the size calculation to ensure the correct length is used when initializing the `smb2_query_info_req` structure. It has been tested and confirmed to resolve the issue without introducing any regressions.
Maybe the prior commit eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays") should be backported to solve this problem directly. The patch does not seem to conflict.
It looks there are several people working on the very same problem addint patches right now on top.
See as well https://lore.kernel.org/stable/c4c2f990-20cf-4126-95bd-d14c58e85042@oracle.c...
But this is already worked on and the proper solution is to only the eb3e28c1e89b backport included?
See as well https://lore.kernel.org/regressions/Zb5eL-AKcZpmvYSl@eldamar.lan/ and following.
And this needs to be done consistently for the 5.10.y and 5.15.y series.
And I'm totally confused here.
Can someone send me, on top of the patches that are in the current queue (I'll push out a -rc series soon), for what needs to be done here? Or, should I just start reverting things?
lost,
greg k-h
linux-stable-mirror@lists.linaro.org