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

[PATCH] 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 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.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
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 | 11 +++++++++--
 fs/smb/client/trace.h   |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f5aad5f61dce..1a822121a883 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -1721,6 +1721,7 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
 		cifs_dbg(VFS, "Send error in read = %d\n", rc);
 	} else {
 		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;
@@ -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);
+			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..982b6ba1e429 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") \

base-commit: 1b78070aaef63512688aebfbc82365ef9d6660f1
-- 
2.39.5

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

[Namjae Jeon]

On Sat, Aug 29, 2026 at 12:02 AM Diego Oliva <diego@bynar.io> wrote:
>
> 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.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Assisted-by: Bynario AI
> Signed-off-by: Diego Oliva <diego@bynar.io>
> ---
> 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 | 11 +++++++++--
>  fs/smb/client/trace.h   |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index f5aad5f61dce..1a822121a883 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -1721,6 +1721,7 @@ CIFSSMBRead(const unsigned int xid, struct cifs_io_parms *io_parms,
>                 cifs_dbg(VFS, "Send error in read = %d\n", rc);
>         } else {
>                 int data_length = le16_to_cpu(pSMBr->DataLengthHigh);
Should we make data_length unsigned also ?

> +               __u16 data_offset = le16_to_cpu(pSMBr->DataOffset);

[PATCH v2] 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 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@vger.kernel.org
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
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@bynar.io/

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);
 		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);
+			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..982b6ba1e429 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") \

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.39.5

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

[Paulo Alcantara]

Diego Oliva <diego@bynar.io> 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@vger.kernel.org
> Assisted-by: Bynario AI
> Signed-off-by: Diego Oliva <diego@bynar.io>
> ---
> 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@bynar.io/
>
> 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().

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

[Diego Oliva]

On Mon, Aug 31, 2026 at 11:15 PM Paulo Alcantara <pc@manguebit.org> wrote:

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

I've reproduced the scenario you referenced and confirmed an
additional check is needed.

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

Will do.

Thanks for your time!