[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@xxxxxxxxxxxxxxx> # 6.19.x
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@xxxxxxxx>
---
 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