Re: [PATCH v2] smb: client: reject out-of-bounds DataOffset in CIFSSMBRead()

[Paulo Alcantara]

Diego Oliva <diego@xxxxxxxx> writes:

> The SMB1 synchronous read helper CIFSSMBRead() validates the server's
> DataLength against CIFSMaxBufSize and the caller's count, but never
> validates DataOffset. The copy source is formed as
>
> 	&pSMBr->hdr.Protocol + le16_to_cpu(pSMBr->DataOffset)
>
> and memcpy()'d for DataLength bytes with no check that the
> [DataOffset, DataOffset + DataLength) range lies within the response
> actually received from the server.
>
> A malicious or compromised SMB1 server can return a short response
> carrying an in-range DataLength and a large DataOffset, driving the
> source pointer past the end of the response buffer. The memcpy() then
> copies adjacent kernel heap into the caller's read buffer (information
> disclosure), or reads unmapped memory and oopses (denial of service).
> SMB1 is not negotiated by default; reaching this code requires an
> explicit vers=1.0 mount.
>
> Both DataOffset and the received response length recorded in
> rsp_iov.iov_len are relative to the start of the SMB header, so reject
> the response unless DataOffset + DataLength fits within that length,
> using overflow-safe arithmetic, before forming the source pointer.
>
> While here, make data_length unsigned. It holds a length derived from
> an unsigned on-the-wire field and is only ever compared against
> unsigned quantities. This is not required by the validation added
> above, but it matches what the variable represents.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Bynario AI
> Signed-off-by: Diego Oliva <diego@xxxxxxxx>
> ---
> v2:
>  - make data_length unsigned, as suggested by Namjae Jeon
>  - rebased on current upstream
>  - v1: https://lore.kernel.org/linux-cifs/20260828150203.1419003-1-diego@xxxxxxxx/
>
> Note for backporting: this uses the smb_EIO2() tracepoint helper added
> in v6.19 with f80ac7eda1cf5. For older kernels, the call can be
> replaced with a simple return of -EIO.
>
>  fs/smb/client/cifssmb.c | 13 ++++++++++---
>  fs/smb/client/trace.h   |  1 +
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index f5aad5f61dce..b89d49395362 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -1720,7 +1720,8 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
>  	if (rc) {
>  		cifs_dbg(VFS, "Send error in read = %d\n", rc);
>  	} else {
> -		int data_length = le16_to_cpu(pSMBr->DataLengthHigh);
> +		unsigned int data_length = le16_to_cpu(pSMBr->DataLengthHigh);
> +		__u16 data_offset = le16_to_cpu(pSMBr->DataOffset);

Shouldn't you also validate the minimum response size so that you can
safely dereference the above fields?  E.g.,

if (rsp_iov.iov_len < tcon->ses->server->read_rsp_size) {
        rc = smb_EIO(...);
        *nbytes = 0;
} else {
        unsigned int data_length = le16_to_cpu(pSMBr->DataLengthHigh);
        ...
}

>  		data_length = data_length << 16;
>  		data_length += le16_to_cpu(pSMBr->DataLength);
>  		*nbytes = data_length;
> @@ -1733,9 +1734,15 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
>  			rc = smb_EIO2(smb_eio_trace_read_overlarge,
>  				      data_length, count);
>  			*nbytes = 0;
> +		} else if ((size_t)data_offset + data_length > rsp_iov.iov_len) {
> +			/* check that the data lies within the received response */
> +			cifs_dbg(FYI, "bad data offset %u length %d for read response of %zu\n",
> +				 data_offset, data_length, rsp_iov.iov_len);

@data_length is now unsigned, so use '%u' instead.  Also, print __func__
in cifs_dbg().