Re: [PATCH v3 0/2] smb: client: fix out-of-bounds reads in CIFSSMBRead()

[Paulo Alcantara <pc@manguebit.org>]

Frank Sorenson <sorenson@redhat.com> writes:

> On 9/2/26 5:42 AM, Diego Oliva wrote:
>> CIFSSMBRead() parses the server's READ_RSP without validating either
>> the length of the response or the DataOffset it carries. A malicious
>> or compromised SMB1 server can exploit either to read past the end of
>> the receive buffer, leaking adjacent kernel heap into the caller's
>> read buffer or oopsing on unmapped memory. SMB1 is not negotiated by
>> default; reaching this code requires an explicit vers=1.0 mount.
>>
>> Patch 1 rejects responses too short to contain a whole READ_RSP, so
>> the header fields can be dereferenced safely. Patch 2 ejects a
>> DataOffset/DataLength pair that falls outside the received response.
>
> Your patch 2 checks that data_offset + data_length fit:
>
> +		} else if ((size_t)data_offset + data_length > rsp_iov.iov_len) {
>
> but I think you may also need a lower-bound check to make sure
> data_offset is at least sizeof(READ_RSP):
>
> +               } else if (data_offset < sizeof(READ_RSP)) {
>
> otherwise, the data would overlap the response header itself.

Frank is right.

Diego, do you want me to fold this in:

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f3cba16f6e17..f9aff0712794 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -1742,7 +1742,8 @@ 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) {
+               } else if (data_offset < sizeof(*pSMBr) ||
+                          (size_t)data_offset + data_length > rsp_iov.iov_len) {
                        /* check that the data lies within the received response */
                        cifs_dbg(FYI, "%s: bad data offset %u length %u for response of %zu\n",
                                 __func__, data_offset, data_length, rsp_iov.iov_len);