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

[Paulo Alcantara <pc@manguebit.org>]

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