[PATCH v2 0/2] smb: client: reject a too-small tree connect byte count
Bryam Vargas via B4 Relay <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.linux.kernel.cifs,gmane.network.samba.internals |
|---|---|
| Message-ID | <[email protected]> |
v1 was a single patch. The kernel test robot then reported three -Wconstant-conversion diagnostics in the write path, which Namjae asked about; they are v1's doing. enum smb_eio_trace is __mode(byte) and its list was already at 128 entries, so v1's new entry pushed smb_eio_trace_write_too_far to index 128, which the type cannot hold. clang gives the enum a signed underlying type and turns that into an error wherever CONFIG_WERROR is set -- x86_64 defconfig sets it -- so v1 alone does not build there. gcc gives the same enum an unsigned underlying type and says nothing, which is why the tree had not run into this. 1/2 gives the enum room, 2/2 is v1 unchanged. 1/2 has to land first: 2/2 is what adds the 129th entry, so on its own it does not build under clang. Stable trees older than v6.19 have neither smb_EIO2() nor the trace enum, so the backport there is 2/2's guard with a plain rc = -EIO. Build A/B at v7.2, W=1, CONFIG_CIFS=m, with gcc 14.2 and clang 19.1.7: source state gcc clang v7.2, unpatched clean clean 2/2 only clean 3 errors, build fails 1/2 + 2/2 clean clean I dropped the attribute rather than widening it because on x86_64 the record does not grow either way. The field precedes an unsigned long at offset 8 of struct trace_event_raw_smb3_eio, so sizeof() is 32 with __mode(byte), with __mode(HI), and with a plain enum. A plain enum also avoids a spelling the tree does not use: all 68 __mode() uses in it are __mode(byte). smb_eio_trace was the only one of those 68 with no room left. rxrpc_abort_reason has 127 entries. Two corrections to what v1's changelog said, both prose and not code. The initialised prefix is 256 bytes rather than the 67 I wrote: header_assemble() memsets 256 right after cifs_buf_get(), so the byte area only starts past it from WordCount 111. And the impact was understated -- once bytes_left wraps, the bound handed to cifs_strndup_from_utf16() reaches 65535 against a ~16 KB cifs_req_poolp object, so this is a slab out-of-bounds read and not only an uninitialised one. checkSMB() is also not what makes the guard sufficient, which is why 2/2 adds no WordCount check: the parse runs on the request buffer, into which SendReceive() copies smbCalcSize() bytes, so an honest bytes_left is what keeps the walk inside the copy. Reproducer is a fake SMB1 server that answers negprot, completes SESSION_SETUP_ANDX and replies to TREE_CONNECT_ANDX with WordCount 128 and ByteCount 0; KMSAN and KASAN logs on request. Prior version: https://lore.kernel.org/all/[email protected]/ --- Bryam Vargas (2): smb: client: let enum smb_eio_trace grow past 128 entries smb: client: reject a tree connect response whose byte count is too small fs/smb/client/cifssmb.c | 6 ++++++ fs/smb/client/trace.h | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) --- base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f change-id: 20260820-b4-disp-58f78a28-0dbf722521ec Best regards, -- Bryam Vargas <[email protected]>