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

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

[Diego Oliva]

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.

Both patches use smb_EIO2(), introduced in v6.19, so they do not apply
to older stable trees as-is. Anyone who wants them in an older tree
only needs to return plain -EIO in place of smb_EIO2().

v3:
 - split into two patches; validate the minimum response size before
   dereferencing the READ_RSP header fields (Paulo Alcantara)
 - print data_length with %u and add __func__ to cifs_dbg() calls
   (Paulo Alcantara)
 - rebased on current upstream
 - v2: https://lore.kernel.org/linux-cifs/20260831125045.479576-1-diego@bynar.io/

v2:
 - make data_length unsigned, as suggested by Namjae Jeon
 - v1: https://lore.kernel.org/linux-cifs/20260828150203.1419003-1-diego@bynar.io/

Diego Oliva (2):
  smb: client: reject short READ responses in CIFSSMBRead()
  smb: client: reject out-of-bounds DataOffset in CIFSSMBRead()

 fs/smb/client/cifssmb.c | 25 ++++++++++++++++++++-----
 fs/smb/client/trace.h   |  1 +
 2 files changed, 21 insertions(+), 5 deletions(-)


base-commit: 89a312991dc6e638a36adc43ccb91dbc25504c04
-- 
2.39.5

[PATCH v3 1/2] smb: client: reject short READ responses in CIFSSMBRead()

[Diego Oliva]

CIFSSMBRead() reads DataLengthHigh, DataLength and DataOffset out of
the READ_RSP returned by the server without first checking that a
whole READ_RSP was actually received. The length of the response is
recorded in rsp_iov.iov_len, but nothing constrains it to be at least
read_rsp_size before those fields are dereferenced.

A malicious or compromised SMB1 server can return a response shorter
than the READ_RSP header, so that parsing the header itself reads past
the end of the receive buffer. SMB1 is not negotiated by default;
reaching this code requires an explicit vers=1.0 mount.

Reject the response unless it is at least read_rsp_size bytes long.

smb_EIO2() was introduced in v6.19, so this does not apply to older
stable trees without returning plain -EIO instead.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Suggested-by: Paulo Alcantara <pc@manguebit.org>
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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f5aad5f61dce..aa6b904ad866 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -1719,6 +1719,14 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
 	pSMBr = (READ_RSP *)rsp_iov.iov_base;
 	if (rc) {
 		cifs_dbg(VFS, "Send error in read = %d\n", rc);
+	} else if (rsp_iov.iov_len < tcon->ses->server->vals->read_rsp_size) {
+		/* check that the received response can hold a whole READ_RSP */
+		cifs_dbg(FYI, "%s: server returned short header. got=%zu expected=%zu\n",
+			 __func__, rsp_iov.iov_len,
+			 tcon->ses->server->vals->read_rsp_size);
+		rc = smb_EIO2(smb_eio_trace_read_rsp_short,
+			      rsp_iov.iov_len, tcon->ses->server->vals->read_rsp_size);
+		*nbytes = 0;
 	} else {
 		int data_length = le16_to_cpu(pSMBr->DataLengthHigh);
 		data_length = data_length << 16;
-- 
2.39.5

[PATCH v3 2/2] smb: client: reject out-of-bounds DataOffset in CIFSSMBRead()

[Diego Oliva]

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 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.
The response length has been validated by the previous patch, so the
DataOffset and DataLength fields can be read safely here.

While here, make data_length unsigned. It holds a length derived from
unsigned on-the-wire fields and is only ever compared against unsigned
quantities; print it with %u accordingly, and add __func__ to the
cifs_dbg() calls in this function.

smb_EIO2() was introduced in v6.19, so this does not apply to older
stable trees without returning plain -EIO instead.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
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 | 17 ++++++++++++-----
 fs/smb/client/trace.h   |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index aa6b904ad866..3c86eb6cf76e 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -1728,7 +1728,8 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
 			      rsp_iov.iov_len, tcon->ses->server->vals->read_rsp_size);
 		*nbytes = 0;
 	} 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);
 		data_length = data_length << 16;
 		data_length += le16_to_cpu(pSMBr->DataLength);
 		*nbytes = data_length;
@@ -1736,14 +1737,20 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
 		/*check that DataLength would not go beyond end of SMB */
 		if ((data_length > CIFSMaxBufSize)
 				|| (data_length > count)) {
-			cifs_dbg(FYI, "bad length %d for count %d\n",
-				 data_length, count);
+			cifs_dbg(FYI, "%s: bad length %u for count %u\n",
+				 __func__, data_length, count);
 			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, "%s: bad data offset %u length %u for response of %zu\n",
+				 __func__, data_offset, data_length, rsp_iov.iov_len);
+			rc = smb_EIO2(smb_eio_trace_read_bad_offset,
+				      data_offset, data_length);
+			*nbytes = 0;
 		} else {
-			pReadData = (char *) (&pSMBr->hdr.Protocol) +
-					le16_to_cpu(pSMBr->DataOffset);
+			pReadData = (char *) (&pSMBr->hdr.Protocol) + data_offset;
 /*			if (rc = copy_to_user(buf, pReadData, data_length)) {
 				cifs_dbg(VFS, "Faulting on read rc = %d\n",rc);
 				rc = -EFAULT;
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index 12241abb8e2e..b442cccd1530 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -79,6 +79,7 @@
 	EM(smb_eio_trace_qreparse_setup_count,		"qreparse_setup_count") \
 	EM(smb_eio_trace_qreparse_sizes_wrong,		"qreparse_sizes_wrong") \
 	EM(smb_eio_trace_qsym_bcc_too_small,		"qsym_bcc_too_small") \
+	EM(smb_eio_trace_read_bad_offset,		"read_bad_offset") \
 	EM(smb_eio_trace_read_mid_state_unknown,	"read_mid_state_unknown") \
 	EM(smb_eio_trace_read_overlarge,		"read_overlarge") \
 	EM(smb_eio_trace_read_rsp_malformed,		"read_rsp_malformed") \
-- 
2.39.5

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

[Paulo Alcantara]

Diego Oliva <diego@bynar.io> writes:

> 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.
> ....

Applied.

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

[Frank Sorenson]

Hi Diego,

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

-- 
Frank Sorenson
sorenson@redhat.com
Principal Software Maintenance Engineer, filesystems
Red Hat

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

[Paulo Alcantara]

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);

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

[Diego Oliva]

On Wed, Sep 2, 2026 at 10:29 PM Frank Sorenson <sorenson@redhat.com> wrote:
>
> 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.

Hi Frank, that's true, thanks for spotting the missing check!

On Wed, Sep 2, 2026 at 11:32 PM Paulo Alcantara <pc@manguebit.org> wrote:
>
> 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);

Looks good to me, thanks Paulo!

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

[Paulo Alcantara]

Diego Oliva <diego@bynar.io> writes:

> On Wed, Sep 2, 2026 at 11:32 PM Paulo Alcantara <pc@manguebit.org> wrote:
>>
>> 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);
>
> Looks good to me, thanks Paulo!

Done, thanks.  Updated #cifs-next with it.