* [PATCH 5.10 1/1] cifs: Fix stack-out-of-bounds in smb2_set_next_command()
2024-02-07 11:52 [PATCH 5.10 0/1] cifs: Fix stack-out-of-bounds in smb2_set_next_command() ZhaoLong Wang
@ 2024-02-07 11:52 ` ZhaoLong Wang
2024-02-07 15:11 ` [PATCH 5.10 0/1] " Salvatore Bonaccorso
1 sibling, 0 replies; 4+ messages in thread
From: ZhaoLong Wang @ 2024-02-07 11:52 UTC (permalink / raw)
To: stable; +Cc: gregkh, sfrench
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;
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 5.10 0/1] cifs: Fix stack-out-of-bounds in smb2_set_next_command()
2024-02-07 11:52 [PATCH 5.10 0/1] cifs: Fix stack-out-of-bounds in smb2_set_next_command() ZhaoLong Wang
2024-02-07 11:52 ` [PATCH 5.10 1/1] " ZhaoLong Wang
@ 2024-02-07 15:11 ` Salvatore Bonaccorso
2024-02-20 20:26 ` Greg KH
1 sibling, 1 reply; 4+ messages in thread
From: Salvatore Bonaccorso @ 2024-02-07 15:11 UTC (permalink / raw)
To: ZhaoLong Wang
Cc: stable, gregkh, sfrench, kovalev, Mohamed Abuelfotoh, Hazem,
Darren Kenny, Harshit Mogalapalli
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.com/
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
^ permalink raw reply [flat|nested] 4+ messages in thread