[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]>
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.