fs/smb/client/cifssmb.c | 25 ++++++++++++++++++++----- fs/smb/client/trace.h | 1 + 2 files changed, 21 insertions(+), 5 deletions(-)
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
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
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);
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!
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.
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.
© 2016 - 2026 Red Hat, Inc.