[PATCH 0/3] smb: client: validate SMB1 write and query response lengths

[PATCH 0/3] smb: client: validate SMB1 write and query response lengths

[Diego Oliva]

Three fixes for SMB1 reply parsers that dereference a response struct
without first establishing that the response was long enough to hold
it. They are the same class of defect as the recent "smb: client:
reject short READ responses in CIFSSMBRead()", generalised to the
write and legacy query paths.

checkSMB() only requires the RFC1002 length to agree with WordCount
and the byte count; it does not check either against what the command
in question requires. Every fixed-layout SMB1 response struct is
therefore unguarded unless its own parser checks: a reply carrying a
smaller WordCount than the command expects is accepted and handed to
the parser with rc == 0, and the smallest such reply is 35 bytes, a
header with WordCount and ByteCount both zero.

Patch 1 adds the missing minimum-length check to the three WRITE_RSP
parsers, and patch 3 does the same for SMBQueryInformation(). In each
case the size required is exactly the smallest reply a conforming
server can send for the word count the response is documented to
carry, so no legitimate server can be rejected:

  sizeof(WRITE_RSP)              wct = 6    33 + 2*6  + 2 = 47
  sizeof(QUERY_INFORMATION_RSP)  wct = 10   33 + 2*10 + 2 = 55

where 33 is sizeof(struct smb_hdr) and the trailing 2 is ByteCount.

Patch 2 is a different bug in one of the same parsers.
cifs_writev_callback() accepts a byte count larger than the length it
asked to write - the existing OS/2 workaround only masks off the high
16 bits - and hands it to netfs, which answers with a WARN() from
netfs_write_subrequest_terminated(). Before netfs clamps the value,
cifs_write_subrequest_terminated() has already used it to grow the
client's idea of the file size. Reject the reply instead.

All three use smb_EIO2(), introduced in v6.19, so they do not apply to
older stable trees as-is, and substituting plain -EIO is not enough on
its own: older trees shape struct smb_hdr and struct mid_q_entry
differently, so both the sizeof() bounds these patches test against and
the response length patch 1 reads out of the mid need reworking.

Diego Oliva (3):
  smb: client: reject short WRITE responses in the SMB1 write paths
  smb: client: reject over-long write counts in cifs_writev_callback()
  smb: client: reject short responses in SMBQueryInformation()

 fs/smb/client/cifssmb.c | 36 ++++++++++++++++++++++++++++++++++++
 fs/smb/client/trace.h   |  3 +++
 2 files changed, 39 insertions(+)


base-commit: 0a96d0d726cd380423ac38e2c28f538db2940a1d
-- 
2.39.5

[PATCH 1/3] smb: client: reject short WRITE responses in the SMB1 write paths

[Diego Oliva]

cifs_writev_callback(), CIFSSMBWrite() and CIFSSMBWrite2() all read
Count and CountHigh out of the WRITE_RSP returned by the server
without first checking that a whole WRITE_RSP was actually received.
The length of the response is available to each of them, in
mid->response_pdu_len, bytes_returned and rsp_iov.iov_len
respectively, but none of them constrains it to be at least
sizeof(WRITE_RSP) before those fields are dereferenced. In the
asynchronous case cifs_check_receive() has run first, but it only
verifies the signature and maps the SMB error; it performs no length
validation.

A malicious or compromised SMB1 server can therefore return a response
shorter than the WRITE_RSP header and still have it parsed. In
CIFSSMBWrite() the response buffer is the request buffer, since
smb_init() hands out a single allocation for both, so the count is
read back out of the request that was just sent; in the other two the
reply lives in the demultiplex thread's buffer, so it comes from
recycled slab memory. Either way the client reports a number of bytes
written that the server never sent. SMB1 is not negotiated by default;
reaching this code requires an explicit vers=1.0 mount.

Reject the response unless it is at least sizeof(WRITE_RSP) bytes
long. This cannot reject a conforming server: WRITE_RSP is documented
as wct = 6, so the smallest valid reply is
sizeof(struct smb_hdr) + 2 * 6 + 2, which is sizeof(WRITE_RSP).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: c28c89fc43e3 ("cifs: add cifs_async_writev")
Cc: <stable@vger.kernel.org> # 6.19.x
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 21 +++++++++++++++++++++
 fs/smb/client/trace.h   |  1 +
 2 files changed, 22 insertions(+)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f9aff0712794..b1525d491ce5 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -1880,6 +1880,12 @@ CIFSSMBWrite(const unsigned int xid, struct cifs_io_parms *io_parms,
 	cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
 	if (rc) {
 		cifs_dbg(FYI, "Send error in write = %d\n", rc);
+	} else if (bytes_returned < (int)sizeof(WRITE_RSP)) {
+		/* check that the received response can hold a whole WRITE_RSP */
+		cifs_dbg(FYI, "%s: server returned short header. got=%d expected=%zu\n",
+			 __func__, bytes_returned, sizeof(WRITE_RSP));
+		rc = smb_EIO2(smb_eio_trace_write_rsp_short,
+			      bytes_returned, sizeof(WRITE_RSP));
 	} else {
 		*nbytes = le16_to_cpu(pSMBr->CountHigh);
 		*nbytes = (*nbytes) << 16;
@@ -1927,6 +1933,15 @@ cifs_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		if (result != 0)
 			break;
 
+		if (mid->response_pdu_len < sizeof(WRITE_RSP)) {
+			/* check that the received response can hold a whole WRITE_RSP */
+			cifs_dbg(FYI, "%s: server returned short header. got=%u expected=%zu\n",
+				 __func__, mid->response_pdu_len, sizeof(WRITE_RSP));
+			result = smb_EIO2(smb_eio_trace_write_rsp_short,
+					  mid->response_pdu_len, sizeof(WRITE_RSP));
+			break;
+		}
+
 		written = le16_to_cpu(smb->CountHigh);
 		written <<= 16;
 		written += le16_to_cpu(smb->Count);
@@ -2150,6 +2165,12 @@ CIFSSMBWrite2(const unsigned int xid, struct cifs_io_parms *io_parms,
 	} else if (resp_buf_type == 0) {
 		/* presumably this can not happen, but best to be safe */
 		rc = smb_EIO1(smb_eio_trace_write_bad_buf_type, resp_buf_type);
+	} else if (rsp_iov.iov_len < sizeof(WRITE_RSP)) {
+		/* check that the received response can hold a whole WRITE_RSP */
+		cifs_dbg(FYI, "%s: server returned short header. got=%zu expected=%zu\n",
+			 __func__, rsp_iov.iov_len, sizeof(WRITE_RSP));
+		rc = smb_EIO2(smb_eio_trace_write_rsp_short,
+			      rsp_iov.iov_len, sizeof(WRITE_RSP));
 	} else {
 		WRITE_RSP *pSMBr = (WRITE_RSP *)rsp_iov.iov_base;
 		*nbytes = le16_to_cpu(pSMBr->CountHigh);
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index b442cccd1530..a0ad8068425e 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -150,6 +150,7 @@
 	EM(smb_eio_trace_write_bad_buf_type,		"write_bad_buf_type") \
 	EM(smb_eio_trace_write_mid_state_unknown,	"write_mid_state_unknown") \
 	EM(smb_eio_trace_write_rsp_malformed,		"write_rsp_malformed") \
+	EM(smb_eio_trace_write_rsp_short,		"write_rsp_short") \
 	E_(smb_eio_trace_write_too_far,			"write_too_far")
 
 #define smb3_rw_credits_traces \
-- 
2.39.5

[PATCH 2/3] smb: client: reject over-long write counts in cifs_writev_callback()

[Diego Oliva]

cifs_writev_callback() builds the number of bytes written from the
CountHigh and Count fields of the WRITE_RSP. Some servers are known to
set CountHigh incorrectly, so the value is masked with 0xFFFF when it
exceeds the requested length, but that only clears the high 16 bits: a
Count that is on its own larger than the requested length survives the
mask unchanged. written > wdata->subreq.len then still holds, the
"written < wdata->subreq.len" test is false, and the inflated count is
passed on as the result of the subrequest.

netfs_write_subrequest_terminated() rejects that with a WARN(). A
server answering a two-byte write with Count = 0xFFFF gives:

 ------------[ cut here ]------------
 Subreq excess write: R=7[1] 65535 > 2 - 0
 WARNING: fs/netfs/write_collect.c:508 at netfs_write_subrequest_terminated+0x425/0x720, CPU#0: cifsd/79
 CPU: 0 UID: 0 PID: 79 Comm: cifsd Not tainted 7.3.0-rc2-00131-g0a96d0d726cd #45 PREEMPT(lazy)
 RIP: 0010:netfs_write_subrequest_terminated+0x43a/0x720
 Call Trace:
  <TASK>
  cifs_writev_callback+0x4af/0x900
  cifs_demultiplex_thread+0xd35/0x2280
  kthread+0x315/0x410
  ret_from_fork+0x647/0x920
  ret_from_fork_asm+0x1a/0x30
  </TASK>
 ---[ end trace 0000000000000000 ]---

so such a server triggers a kernel warning on every write, and a panic
on a kernel built with panic_on_warn. Before netfs clamps the value,
cifs_write_subrequest_terminated() has already used it to compute
wrend, which feeds netfs_resize_file() and
cifs_update_i_blocks_for_write(), so the size the client believes the
file has also grows past what was actually written. SMB1 is not
negotiated by default; reaching this code requires an explicit
vers=1.0 mount.

The missing upper bound dates from the introduction of
cifs_writev_callback(). It became a WARN, and began inflating the
client's idea of the file size, only once the write result was handed
to netfs.

Reject the response instead. A server reporting more written than it
was asked to write is violating the protocol, and CIFSSMBRead()
already treats the symmetric case on the read side as an error.

Fixes: c28c89fc43e3 ("cifs: add cifs_async_writev")
Fixes: 3ee1a1fc3981 ("cifs: Cut over to using netfslib")
Cc: <stable@vger.kernel.org> # 6.19.x
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 9 +++++++++
 fs/smb/client/trace.h   | 1 +
 2 files changed, 10 insertions(+)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index b1525d491ce5..30b9621664e8 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -1954,6 +1954,15 @@ cifs_writev_callback(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		if (written > wdata->subreq.len)
 			written &= 0xFFFF;
 
+		if (written > wdata->subreq.len) {
+			/* check that the server did not write more than requested */
+			cifs_dbg(FYI, "%s: bad count %zu for length %zu\n",
+				 __func__, written, wdata->subreq.len);
+			result = smb_EIO2(smb_eio_trace_write_overlarge,
+					  written, wdata->subreq.len);
+			break;
+		}
+
 		if (written < wdata->subreq.len) {
 			result = -ENOSPC;
 		} else {
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index a0ad8068425e..a1c0cb1a5833 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -149,6 +149,7 @@
 	EM(smb_eio_trace_user_iter,			"user_iter") \
 	EM(smb_eio_trace_write_bad_buf_type,		"write_bad_buf_type") \
 	EM(smb_eio_trace_write_mid_state_unknown,	"write_mid_state_unknown") \
+	EM(smb_eio_trace_write_overlarge,		"write_overlarge") \
 	EM(smb_eio_trace_write_rsp_malformed,		"write_rsp_malformed") \
 	EM(smb_eio_trace_write_rsp_short,		"write_rsp_short") \
 	E_(smb_eio_trace_write_too_far,			"write_too_far")
-- 
2.39.5

[PATCH 3/3] smb: client: reject short responses in SMBQueryInformation()

[Diego Oliva]

SMBQueryInformation() reads attr, last_write_time and size out of the
QUERY_INFORMATION_RSP returned by the server without first checking
that a whole QUERY_INFORMATION_RSP was actually received. The length
of the response is recorded in bytes_returned, but nothing constrains
it to be at least sizeof(QUERY_INFORMATION_RSP) before those fields
are dereferenced.

A malicious or compromised SMB1 server can return a reply as short as
35 bytes, a header carrying WordCount and ByteCount of zero, which
checkSMB() accepts because the calculated size matches the length
received. last_write_time and size then lie beyond the received data.
smb_init() hands out a single allocation for both the request and the
response, so those fields are read back out of the request that was
just sent, and a timestamp and file size synthesised from the path
name end up in the inode and are reported by stat(). SMB1 is not
negotiated by default; reaching this code requires an explicit
vers=1.0 mount.

Reject the response unless it is at least
sizeof(QUERY_INFORMATION_RSP) bytes long. This cannot reject a
conforming server: the response is documented as wct = 10, so the
smallest valid reply is sizeof(struct smb_hdr) + 2 * 10 + 2, which is
sizeof(QUERY_INFORMATION_RSP).

Fixes: 6b8edfe0f918 ("[CIFS] Support for mounting to older servers part 2.  Add support for legacy getattr (lookup).")
Cc: <stable@vger.kernel.org> # 6.19.x
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 6 ++++++
 fs/smb/client/trace.h   | 1 +
 2 files changed, 7 insertions(+)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 30b9621664e8..aad1c866d8e1 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -4068,6 +4068,12 @@ SMBQueryInformation(const unsigned int xid, struct cifs_tcon *tcon,
 			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
 	if (rc) {
 		cifs_dbg(FYI, "Send error in QueryInfo = %d\n", rc);
+	} else if (bytes_returned < (int)sizeof(QUERY_INFORMATION_RSP)) {
+		/* check that the received response can hold a whole rsp */
+		cifs_dbg(FYI, "%s: server returned short header. got=%d expected=%zu\n",
+			 __func__, bytes_returned, sizeof(QUERY_INFORMATION_RSP));
+		rc = smb_EIO2(smb_eio_trace_qinfo_rsp_short,
+			      bytes_returned, sizeof(QUERY_INFORMATION_RSP));
 	} else if (data) {
 		struct timespec64 ts;
 		__u32 time = le32_to_cpu(pSMBr->last_write_time);
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index a1c0cb1a5833..6926a62ddb05 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -71,6 +71,7 @@
 	EM(smb_eio_trace_qfsinfo_bcc_too_small,		"qfsinfo_bcc_too_small") \
 	EM(smb_eio_trace_qfsposixinfo_bcc_too_small,	"qfsposixinfo_bcc_too_small") \
 	EM(smb_eio_trace_qfsunixinfo_bcc_too_small,	"qfsunixinfo_bcc_too_small") \
+	EM(smb_eio_trace_qinfo_rsp_short,		"qinfo_rsp_short") \
 	EM(smb_eio_trace_qpathinfo_bcc_too_small,	"qpathinfo_bcc_too_small") \
 	EM(smb_eio_trace_qpathinfo_invalid,		"qpathinfo_invalid") \
 	EM(smb_eio_trace_qreparse_data_area,		"qreparse_data_area") \
-- 
2.39.5

Re: [PATCH 1/3] smb: client: reject short WRITE responses in the SMB1 write paths

[Paulo Alcantara]

Diego Oliva <diego@bynar.io> writes:

> cifs_writev_callback(), CIFSSMBWrite() and CIFSSMBWrite2() all read
> Count and CountHigh out of the WRITE_RSP returned by the server
> without first checking that a whole WRITE_RSP was actually received.
> The length of the response is available to each of them, in
> mid->response_pdu_len, bytes_returned and rsp_iov.iov_len
> respectively, but none of them constrains it to be at least
> sizeof(WRITE_RSP) before those fields are dereferenced. In the
> asynchronous case cifs_check_receive() has run first, but it only
> verifies the signature and maps the SMB error; it performs no length
> validation.
>
> A malicious or compromised SMB1 server can therefore return a response
> shorter than the WRITE_RSP header and still have it parsed. In
> CIFSSMBWrite() the response buffer is the request buffer, since
> smb_init() hands out a single allocation for both, so the count is
> read back out of the request that was just sent; in the other two the
> reply lives in the demultiplex thread's buffer, so it comes from
> recycled slab memory. Either way the client reports a number of bytes
> written that the server never sent. SMB1 is not negotiated by default;
> reaching this code requires an explicit vers=1.0 mount.
>
> Reject the response unless it is at least sizeof(WRITE_RSP) bytes
> long. This cannot reject a conforming server: WRITE_RSP is documented
> as wct = 6, so the smallest valid reply is
> sizeof(struct smb_hdr) + 2 * 6 + 2, which is sizeof(WRITE_RSP).
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Fixes: c28c89fc43e3 ("cifs: add cifs_async_writev")
> Cc: <stable@vger.kernel.org> # 6.19.x
> Assisted-by: Bynario AI
> Signed-off-by: Diego Oliva <diego@bynar.io>
> ---
>  fs/smb/client/cifssmb.c | 21 +++++++++++++++++++++
>  fs/smb/client/trace.h   |  1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index f9aff0712794..b1525d491ce5 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -1880,6 +1880,12 @@ CIFSSMBWrite(const unsigned int xid, struct cifs_io_parms *io_parms,
>  	cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
>  	if (rc) {
>  		cifs_dbg(FYI, "Send error in write = %d\n", rc);
> +	} else if (bytes_returned < (int)sizeof(WRITE_RSP)) {
> +		/* check that the received response can hold a whole WRITE_RSP */

Please don't add these useless comments.  The bound check, trace and
debug messages are already enough to understand it.

Ditto for the rest of the series.

Also, check sashiko comments [1] and see if any of those make sense to
address.

Thanks.

[1] https://sashiko.dev/#/patchset/20260911145758.3833254-1-diego%40bynar.io