[RFC PATCH 0/1] smb: client: tighten validate_t2() offset bounds against actual buffer size

[Frank Sorenson <sorenson@redhat.com>]

Hi all,

The accompanying patch tightens bounds checking in validate_t2(), the
central validator called before all 21 SMB1 TRANSACT2 response sites.
I'm posting it as an RFC to request review of design choices and the
approach.

Background
----------

validate_t2() currently rejects ParameterOffset and DataOffset only when
they exceed 1024.  The actual allocated buffer size is
CIFSMaxBufSize + MAX_CIFS_HDR_SIZE (~16468 bytes), so a server can supply
an offset in [1025, 16468] and have callers dereference typed structs from
uninitialized pool memory.

cifs_buf_get() clears only the first 207 bytes of each pool buffer on
allocation; bytes beyond that retain stale data from prior uses.  A
response with DataOffset=15000, DataCount=0, BCC=40 produces a 94-byte
received frame: 15000 + 0 <= 16468 passes the individual guard, yet the
dereference lands far into the stale region.  Raising the individual bound
alone does not close this window; a joint offset+count check against the
actual received frame extent is needed.

Approach
--------

Rather than adding ad-hoc bounds checks at each of the 21 call sites,
the patch extends validate_t2() with two new parameters — min_param_size
and min_data_size — that let callers declare the minimum struct size they
need to read.  validate_t2() verifies that the respective offset clears
the fixed T2 response header and that offset + struct size fits within
frame_end.  Centralizing this in validate_t2() keeps the validation
auditable in one place rather than scattered across 21 call sites.

Design choice 1: frame_end as the joint bound
---------------------------------------------

frame_end is computed as:

    frame_end = sizeof(struct smb_hdr) + 2 * WordCount + sizeof(__le16) + BCC

This matches smbCalcSize() in smb1misc.c exactly.  checkSMB() validates
consistency between the RFC1001 length prefix and WordCount + BCC before
validate_t2() is reached, so frame_end reflects the number of bytes
actually received and is not purely server-controlled.

Note: struct smb_hdr opens with Protocol[4] at offset 0 — the RFC1001
length prefix is stripped by the receive path before the pool buffer is
filled, so no subtraction from sizeof(struct smb_hdr) is needed.  This is
confirmed by smbCalcSize() using sizeof(struct smb_hdr) with no adjustment.

All joint offset+count checks use frame_end.  The individual offset guards
retain CIFSMaxBufSize + MAX_CIFS_HDR_SIZE as a buffer-overflow backstop.

A secondary benefit: the raised individual bound creates a new overflow
vector — DataOffset=202 and DataCount=16387 each pass the bound separately
but 202 + 16387 = 16589 overflows the 16588-byte buffer.  The frame_end
joint check catches this too.

Design choice 2: lnoff <= DataCount (not lnoff <= frame_end)
------------------------------------------------------------

CIFSFindFirst and CIFSFindNext use LastNameOffset (lnoff) as an offset
within the data area.  Rather than exporting frame_end from validate_t2()
or recomputing it at the call site, the patch bounds lnoff against
DataCount.  Since validate_t2() has already verified
data_off + DataCount <= frame_end, this transitively ensures
data_off + lnoff <= frame_end.  It is also semantically correct:
LastNameOffset is an offset within the declared data area, not an
arbitrary buffer offset.

Design choice 3: CIFSSMBPosixLock small-buffer exception
---------------------------------------------------------

CIFSSMBPosixLock receives into a small buffer (MAX_CIFS_SMALL_BUFFER_SIZE
= 448 bytes).  Passing a non-zero min_data_size to validate_t2() would be
wrong: validate_t2()'s large-buffer upper bound of ~16468 bytes would
accept data_offset values that overflow the 448-byte allocation.

The patch passes (0, 0) to validate_t2() for the buffer-overflow backstop
only, and follows with a per-call tight check:

    if (data_offset < SMB_T2_MIN_OFFSET ||
        data_offset + sizeof(struct cifs_posix_lock) > MAX_CIFS_SMALL_BUFFER_SIZE)

This is the only call site with this exception; all other T2 callers use
the standard large buffer.

---

I welcome any feedback on these design choices, or on the patch in
general.

Frank Sorenson (1):
  smb: client: tighten validate_t2() offset bounds against actual buffer
    size

 fs/smb/client/cifssmb.c | 133 +++++++++++++++++++++++++++++-----------
 1 file changed, 97 insertions(+), 36 deletions(-)

-- 
2.55.0