[PATCH v3 00/11] smb: client: fix OOB reads and UAFs in SMB2/3 receive paths

[PATCH v3 00/11] smb: client: fix OOB reads and UAFs in SMB2/3 receive paths

[Frank Sorenson]

This series fixes eleven bounds-checking defects in the SMB2/3 client,
all reachable from a malicious or compromised server.

Patches 1-3 address the compound encrypted frame processing path:

Patch 1 fixes four interacting bugs in receive_encrypted_standard():
a missing lower bound on NextCommand, an off-by-one upper bound that
admitted trailing slices too small for an SMB2 header (producing a
write-after-free via next_buffer aliasing server->bigbuf), a stale
next_buffer pointer not cleared before goto one_more, and use of the
pre-decryption pdu_length instead of the plaintext extent for bounds
checking.

Patch 2 adds smb2_min_pdu_len[], a per-command table of minimum response
struct sizes, and uses it in smb2_check_message() to reject responses
too short for smb2_get_data_area_len() to safely read command-specific
struct fields.

Patch 3 fixes server->total_read tracking in receive_encrypted_standard()
so that smb2_check_message() sees the actual per-sub-PDU size rather than
the full remaining compound tail.  Without this, a rogue server can craft
a compound frame where any non-last sub-PDU is shorter than its declared
fixed struct, bypassing the guards added in patch 2.

The remaining patches fix lower-bound gaps and OOB reads in DFS referral
parsing, EA list traversal, posix SID bounds, change-notify offset,
snapshot enumeration, and SMB1 reparse point validation.

Note on overlap with a concurrent series: Zihan Xi's
[PATCH v2 0/2] "smb: client: fix create context out-of-bounds reads"
(Message-ID: <cover.1787486936.git.zihanx@nebusec.ai>) touches
smb2_parse_contexts() and parse_posix_ctxt() independently.  Patch 11
here addresses the same function (smb2_parse_contexts()) but focuses on
complementary issues that their series does not cover: NameOffset
validation (lower and upper bounds) and gating all three handler
dispatches on a non-zero DataLength to prevent zero-DataLength contexts
from exercising parse_lease_buf, parse_query_id_ctxt, or parse_posix_ctxt.
Their per-context cc_len bounding and lease/QFid minimum-length checks
are not duplicated here.  parse_posix_ctxt() DataLength validation is
omitted from this series entirely since their patch 2/2 addresses it.


v3 changes:
 - respin entire series

v2 changes:
 - patch 6: reject next_entry_offset values that leave fewer than
   sizeof(*src) bytes remaining after advancing.

Frank Sorenson (11):
  smb: client: fix NextCommand bounds and aliasing UAF in
    receive_encrypted_standard()
  smb: client: validate PDU length before smb2_get_data_area_len()
    struct access
  smb: client: fix server->total_read not tracking sub-PDU size in
    receive_encrypted_standard()
  smb: client: fix missing lower-bound check on DFS referral string
    offsets
  smb: client: fix missing lower-bound on Next field in
    parse_server_interfaces()
  smb: client: fix OOB struct field reads in move_smb2_ea_to_cifs()
  smb: client: fix missing iov bounds check in parse_posix_sids()
  smb: client: fix underflow in is_valid_oplock_break() notify offset
    check
  smb: client: fix potential OOB read in smb3_enum_snapshots()
  smb: client: fix incomplete bounds check on reparse buffer in
    cifs_query_reparse_point()
  smb: client: fix NameOffset and Next field validation in
    smb2_parse_contexts()

 fs/smb/client/cifssmb.c   |  2 +-
 fs/smb/client/misc.c      | 12 +++++++--
 fs/smb/client/smb1misc.c  |  3 ++-
 fs/smb/client/smb2inode.c | 11 ++++++++
 fs/smb/client/smb2misc.c  | 53 +++++++++++++++++++++++++++++++++++++++
 fs/smb/client/smb2ops.c   | 51 +++++++++++++++++++++++++++----------
 fs/smb/client/smb2pdu.c   | 11 ++++----
 fs/smb/client/trace.h     |  1 +
 8 files changed, 122 insertions(+), 22 deletions(-)

-- 
2.55.0

[PATCH v3 01/11] smb: client: fix NextCommand bounds and aliasing UAF in receive_encrypted_standard()

[Frank Sorenson]

Four related bugs in receive_encrypted_standard():

Lower bound: next_cmd is not checked against MID_HEADER_SIZE(server).  A
non-zero NextCommand smaller than the SMB2 header size passes the
upper-bound check; memcpy pulls bytes from within the current PDU header
into next_buffer, and the next iteration casts that region as a fresh
smb2_hdr.

Upper bound / trailing slice: the original check uses strict greater-than
(`next_cmd > pdu_length`), so next_cmd == pdu_length passes; memcpy copies
zero bytes, smb2_check_message() rejects the zeroed buffer, ret != 0, and
free_rsp_buf() releases next_buffer (aliasing server->bigbuf) while
allocate_buffers() still holds it: write-after-free.  Even with strict
less-than enforced, a next_cmd satisfying next_cmd < pdu_length but
pdu_length - next_cmd < MID_HEADER_SIZE(server) leaves a trailing slice too
small to hold an SMB2 header.  The next iteration reads shdr->NextCommand
from uninitialized slab content; if it reads as 0, the else-if (ret != 0)
branch frees next_buffer, again aliasing server->bigbuf: another
write-after-free.  Replace with three unsigned comparisons that avoid
addition: `next_cmd > pdu_length` to guard against underflow, then
`pdu_length - next_cmd < MID_HEADER_SIZE(server)` for the trailing slice
minimum.  The addition form (`(size_t)next_cmd +
MID_HEADER_SIZE(server) > pdu_length`) wraps to zero on 32-bit
kernels for next_cmd near UINT_MAX, silently admitting out-of-bounds.

Aliasing after goto: once server->bigbuf = buf = next_buffer and the goto
fires, next_buffer still holds the live server->bigbuf pointer.  If
smb2_check_message() fails on that iteration, ret != 0 and the else-if path
frees next_buffer, freeing server->bigbuf while it remains referenced.
Set next_buffer = NULL before goto one_more to prevent the aliased pointer
from being freed as a standalone buffer.

Decrypted extent: decrypt_raw_data() moves plaintext to buf[0..buf_size-1]
via memmove, where buf_size = pdu_length -
sizeof(struct smb2_transform_hdr).  Using pdu_length as the upper bound
allows next_cmd values in (buf_size, pdu_length], letting buf + next_cmd
land in the stale 52-byte transform-header residue and memcpy to copy
stale ciphertext into next_buffer as if it were the next SMB2 frame.
Assign pdu_length = buf_size after successful decryption so bounds checks
and the per-iteration decrement operate on the correct plaintext extent.

smb2_next_header() already enforces the equivalent lower-bound:

    if (unlikely(*noff && *noff < MID_HEADER_SIZE(server)))
        return -EINVAL;

Apply the same constraint here.

Fixes: b24df3e30cbf ("cifs: update receive_encrypted_standard to handle compounded responses")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2ops.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 7d6738ffcb80..e75420dbe950 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -5242,6 +5242,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
 	length = decrypt_raw_data(server, buf, buf_size, NULL, false);
 	if (length)
 		return length;
+	pdu_length = buf_size;
 
 	next_is_large = server->large_buf;
 one_more:
@@ -5254,8 +5255,15 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
 	}
 
 	if (next_cmd) {
-		if (WARN_ON_ONCE(next_cmd > pdu_length))
+		if (next_cmd < MID_HEADER_SIZE(server) ||
+		    next_cmd > pdu_length ||
+		    pdu_length - next_cmd < MID_HEADER_SIZE(server)) {
+			unsigned int max_next = pdu_length > (unsigned int)MID_HEADER_SIZE(server) ?
+					pdu_length - (unsigned int)MID_HEADER_SIZE(server) : 0;
+			cifs_server_dbg(VFS, "invalid NextCommand offset %u out of range [%zu, %u]\n",
+					next_cmd, MID_HEADER_SIZE(server), max_next);
 			return -1;
+		}
 		if (next_is_large)
 			next_buffer = (char *)cifs_buf_get();
 		else
@@ -5291,6 +5299,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
 			server->bigbuf = buf = next_buffer;
 		else
 			server->smallbuf = buf = next_buffer;
+		next_buffer = NULL;
 		goto one_more;
 	} else if (ret != 0) {
 		/*
-- 
2.55.0

[PATCH v3 02/11] smb: client: validate PDU length before smb2_get_data_area_len() struct access

[Frank Sorenson]

smb2_check_message() validates StructureSize2 but not that the received
PDU is large enough to hold the fixed response struct before calling
__smb2_calc_size() -> smb2_get_data_area_len().

smb2_get_data_area_len() reads command-specific struct fields (e.g.,
CreateContextsOffset at offset 144 in smb2_create_rsp) to locate the
data area before the length is validated.  A rogue server can send a
truncated response that passes the StructureSize2 check but causes
smb2_get_data_area_len() to read stale kmalloc'd content.

Add smb2_min_pdu_len[], parallel to smb2_rsp_struct_sizes[], holding
the minimum PDU size (sizeof the fixed response struct) for each command
with a data area.  Reject responses shorter than this minimum before
calling __smb2_calc_size().

The guard condition is the complement of smb2_get_data_area_len()'s
early-return predicate: fires when Status == 0, Status ==
STATUS_MORE_PROCESSING_REQUIRED, or StructureSize2 !=
SMB2_ERROR_STRUCTURE_SIZE2_LE.

SESSION_SETUP with STATUS_MORE_PROCESSING_REQUIRED is covered by the
second term; READ and IOCTL with STATUS_BUFFER_OVERFLOW by the third —
their StructureSize2 (17 and 49 respectively) differs from the error
struct size 9, so smb2_get_data_area_len() reaches the switch without
the guard.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2misc.c | 53 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index 9068175e57cd..7bf7c602cda0 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -85,6 +85,49 @@ static const __le16 smb2_rsp_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = {
 	/* SMB2_OPLOCK_BREAK */ cpu_to_le16(24)
 };
 
+/*
+ * Minimum received PDU size for commands whose fixed response struct is read
+ * by smb2_get_data_area_len() before the packet length is validated.  Must
+ * be non-zero for every command where has_smb2_data_area[] is true; zero
+ * otherwise (guard in smb2_check_message() is skipped).  Keep in sync with
+ * has_smb2_data_area[] above: adding a data area for a currently-zero command
+ * requires a matching sizeof() entry here.
+ */
+static const size_t smb2_min_pdu_len[NUMBER_OF_SMB2_COMMANDS] = {
+	/* SMB2_NEGOTIATE */       sizeof(struct smb2_negotiate_rsp),
+	/* SMB2_SESSION_SETUP */   sizeof(struct smb2_sess_setup_rsp),
+	/* SMB2_LOGOFF */          0,
+	/* SMB2_TREE_CONNECT */    0,
+	/* SMB2_TREE_DISCONNECT */ 0,
+	/* SMB2_CREATE */          sizeof(struct smb2_create_rsp),
+	/* SMB2_CLOSE */           0,
+	/* SMB2_FLUSH */           0,
+	/* SMB2_READ */            sizeof(struct smb2_read_rsp),
+	/* SMB2_WRITE */           0,
+	/* SMB2_LOCK */            0,
+	/* SMB2_IOCTL */           sizeof(struct smb2_ioctl_rsp),
+	/* SMB2_CANCEL */          0,
+	/* SMB2_ECHO */            0,
+	/* SMB2_QUERY_DIRECTORY */ sizeof(struct smb2_query_directory_rsp),
+	/* SMB2_CHANGE_NOTIFY */   sizeof(struct smb2_change_notify_rsp),
+	/* SMB2_QUERY_INFO */      sizeof(struct smb2_query_info_rsp),
+	/* SMB2_SET_INFO */        0,
+	/* SMB2_OPLOCK_BREAK */    0,
+};
+
+/* Enforce: smb2_min_pdu_len[x] != 0 for every x where has_smb2_data_area[x]. */
+static void __maybe_unused smb2_check_min_pdu_len_table(void)
+{
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_NEGOTIATE_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_SESSION_SETUP_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_CREATE_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_READ_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_IOCTL_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_QUERY_DIRECTORY_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_CHANGE_NOTIFY_HE]);
+	BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_QUERY_INFO_HE]);
+}
+
 #define SMB311_NEGPROT_BASE_SIZE (sizeof(struct smb2_hdr) + sizeof(struct smb2_negotiate_rsp))
 
 static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len,
@@ -233,6 +276,16 @@ smb2_check_message(char *buf, unsigned int pdu_len, unsigned int len,
 		}
 	}
 
+	if ((shdr->Status == 0 ||
+	     shdr->Status == STATUS_MORE_PROCESSING_REQUIRED ||
+	     pdu->StructureSize2 != SMB2_ERROR_STRUCTURE_SIZE2_LE) &&
+	    smb2_min_pdu_len[command] &&
+	    len < smb2_min_pdu_len[command]) {
+		cifs_dbg(VFS, "SMB2 command %d response too short: %u < %zu\n",
+			 command, len, smb2_min_pdu_len[command]);
+		return 1;
+	}
+
 	have_data = false;
 	data_area_overlap = false;
 	calc_len = __smb2_calc_size(buf, &have_data, &data_area_overlap);
-- 
2.55.0

[PATCH v3 03/11] smb: client: fix server->total_read not tracking sub-PDU size in receive_encrypted_standard()

[Frank Sorenson]

receive_encrypted_standard() processes compound encrypted frames by
iterating sub-PDUs, decrementing pdu_length by NextCommand on each
hop but leaving server->total_read at the original full-frame size.
cifs_handle_standard() passes server->total_read as len to
smb2_check_message(), so every length guard in smb2_check_message()
(smb2_rsp_struct_sizes[], smb2_min_pdu_len[]) operates on the
full-frame size rather than the current sub-PDU.

A malicious server can craft a compound encrypted frame where any
non-last sub-PDU is shorter than the fixed response struct it
declares.  The oversized len bypasses both guards, smb2_get_data_area_len()
reads struct fields at offsets past the received data, and the
data-area length arithmetic produces a spurious pointer into
uninitialized slab content or the next sub-PDU, which a rogue server
controls.

At the top of each iteration, pdu_length is the full remaining
compound tail (current sub-PDU + all subsequent ones), not the current
sub-PDU alone.  For a non-last sub-PDU, server->total_read must be
derived from next_cmd, not pdu_length.  Read next_cmd before assigning
server->total_read, then use `next_cmd ? next_cmd : pdu_length` so
that smb2_check_message() sees the actual per-sub-PDU extent on every
pass through the loop.

Fixes: b24df3e30cbf ("cifs: update receive_encrypted_standard to handle compounded responses")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2ops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index e75420dbe950..1bc3f8266eda 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -5248,6 +5248,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
 one_more:
 	shdr = (struct smb2_hdr *)buf;
 	next_cmd = le32_to_cpu(shdr->NextCommand);
+	server->total_read = next_cmd ? next_cmd : pdu_length;
 
 	if (*num_mids >= MAX_COMPOUND) {
 		cifs_server_dbg(VFS, "too many PDUs in compound\n");
-- 
2.55.0

[PATCH v3 04/11] smb: client: fix missing lower-bound check on DFS referral string offsets

[Frank Sorenson]

parse_dfs_referrals() checks DfsPathOffset and NetworkAddressOffset
against the buffer end but not against sizeof(*ref).  An offset smaller
than sizeof(struct dfs_referral_level_3) places the derived pointer
inside the referral header, causing cifs_strndup_from_utf16() to read
struct fields as string data.

Require each offset to be at least sizeof(*ref).

Fixes: 4ecce920e13a ("CIFS: move DFS response parsing out of SMB1 code")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/misc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 46e1382e8e04..bc7cccebf387 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -787,7 +787,11 @@ parse_dfs_referrals(struct get_dfs_referral_rsp *rsp, u32 rsp_size,
 		node->ref_flag = le16_to_cpu(ref->ReferralEntryFlags);
 
 		/* copy DfsPath */
-		if (le16_to_cpu(ref->DfsPathOffset) > data_end - (char *)ref) {
+		if (le16_to_cpu(ref->DfsPathOffset) < sizeof(*ref) ||
+		    le16_to_cpu(ref->DfsPathOffset) > data_end - (char *)ref) {
+			cifs_dbg(VFS, "%s: DfsPathOffset %u out of range [%zu, %td]\n",
+				 __func__, le16_to_cpu(ref->DfsPathOffset),
+				 sizeof(*ref), data_end - (char *)ref);
 			rc = -EINVAL;
 			goto parse_DFS_referrals_exit;
 		}
@@ -801,7 +805,11 @@ parse_dfs_referrals(struct get_dfs_referral_rsp *rsp, u32 rsp_size,
 		}
 
 		/* copy link target UNC */
-		if (le16_to_cpu(ref->NetworkAddressOffset) > data_end - (char *)ref) {
+		if (le16_to_cpu(ref->NetworkAddressOffset) < sizeof(*ref) ||
+		    le16_to_cpu(ref->NetworkAddressOffset) > data_end - (char *)ref) {
+			cifs_dbg(VFS, "%s: NetworkAddressOffset %u out of range [%zu, %td]\n",
+				 __func__, le16_to_cpu(ref->NetworkAddressOffset),
+				 sizeof(*ref), data_end - (char *)ref);
 			rc = -EINVAL;
 			goto parse_DFS_referrals_exit;
 		}
-- 
2.55.0

[PATCH v3 05/11] smb: client: fix missing lower-bound on Next field in parse_server_interfaces()

[Frank Sorenson]

parse_server_interfaces() validates the server-supplied Next offset
against bytes_left but not against sizeof(*p).  A small non-zero value
passes the upper-bound check yet advances p by less than one struct,
causing the next iteration to read misaligned, overlapping fields.

Add next < sizeof(*p) to the existing bounds check.

Fixes: 7d34ec36abb8 ("smb3: fix for slab out of bounds on mount to ksmbd")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 1bc3f8266eda..9065c94e5ec6 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -785,9 +785,9 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 			break;
 		}
 		/* Validate that Next doesn't point beyond the buffer */
-		if (next > bytes_left) {
-			cifs_dbg(VFS, "%s: invalid Next pointer %zu > %zd\n",
-				 __func__, next, bytes_left);
+		if (next < sizeof(*p) || next > bytes_left) {
+			cifs_dbg(VFS, "%s: invalid Next pointer %zu out of range [%zu, %zd]\n",
+				 __func__, next, sizeof(*p), bytes_left);
 			rc = -EINVAL;
 			goto out;
 		}
-- 
2.55.0

[PATCH v3 06/11] smb: client: fix OOB struct field reads in move_smb2_ea_to_cifs()

[Frank Sorenson]

The while (src_size > 0) loop guard allows iteration after
next_entry_offset advances src past the point where a full struct fits
in src_size.  Reads of ea_name_length and ea_value_length on the next
iteration are then out-of-bounds.

Require src_size >= sizeof(*src) before reading any struct field,
and reject next_entry_offset values that are smaller than
sizeof(*src) or that leave fewer than sizeof(*src) bytes remaining
after advancing.

For calls where the server returns an EA list with an invalid
next_entry_offset, the error returned to userspace changes from
-ENODATA (getxattr) or -ERANGE (listxattr) to -EIO, correctly
signalling a server protocol error rather than "attribute not present"
or "output buffer too small".

Fixes: 95907fea4fd8 ("cifs: Add support for reading attributes on SMB2+")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2ops.c | 25 +++++++++++++++++--------
 fs/smb/client/trace.h   |  1 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 9065c94e5ec6..efc68c7aef8a 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -1053,8 +1053,9 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size,
 	char *name, *value;
 	size_t buf_size = dst_size;
 	size_t name_len, value_len, user_name_len;
+	u32 next_off;
 
-	while (src_size > 0) {
+	while (src_size >= sizeof(*src)) {
 		name_len = (size_t)src->ea_name_length;
 		value_len = (size_t)le16_to_cpu(src->ea_value_length);
 
@@ -1110,14 +1111,22 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size,
 		if (!src->next_entry_offset)
 			break;
 
-		if (src_size < le32_to_cpu(src->next_entry_offset)) {
-			/* stop before overrun buffer */
-			rc = -ERANGE;
-			break;
+		next_off = le32_to_cpu(src->next_entry_offset);
+		if (next_off < sizeof(*src) || src_size < next_off) {
+			cifs_dbg(FYI, "EA next_entry_offset %u out of range [%zu, %zu]\n",
+				 next_off, sizeof(*src), src_size);
+			rc = smb_EIO2(smb_eio_trace_ea_next_offset,
+				      next_off, src_size);
+			goto out;
+		}
+		src_size -= next_off;
+		src = (void *)((char *)src + next_off);
+		if (src_size > 0 && src_size < sizeof(*src)) {
+			cifs_dbg(FYI, "EA next_entry_offset %u left truncated entry (%zu bytes)\n",
+				 next_off, src_size);
+			rc = smb_EIO2(smb_eio_trace_ea_next_offset, next_off, src_size);
+			goto out;
 		}
-		src_size -= le32_to_cpu(src->next_entry_offset);
-		src = (void *)((char *)src +
-			       le32_to_cpu(src->next_entry_offset));
 	}
 
 	/* didn't find the named attribute */
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index 12241abb8e2e..6da395abae14 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -27,6 +27,7 @@
 	EM(smb_eio_trace_copychunk_overcopy_c,		"copychunk_overcopy_c") \
 	EM(smb_eio_trace_create_rsp_too_small,		"create_rsp_too_small") \
 	EM(smb_eio_trace_dfsref_no_rsp,			"dfsref_no_rsp") \
+	EM(smb_eio_trace_ea_next_offset,		"ea_next_offset") \
 	EM(smb_eio_trace_ea_overrun,			"ea_overrun") \
 	EM(smb_eio_trace_extract_will_pin,		"extract_will_pin") \
 	EM(smb_eio_trace_forced_shutdown,		"forced_shutdown") \
-- 
2.55.0

[PATCH v3 07/11] smb: client: fix missing iov bounds check in parse_posix_sids()

[Frank Sorenson]

parse_posix_sids() derives sidsbuf_end from the server-supplied out_len
without validating against the received iov:

    sidsbuf_end = sidsbuf + out_len - qi_len;

An inflated out_len places sidsbuf_end past the iov, defeating the
bounds guards in posix_info_sid_size() and allowing reads into adjacent
kernel memory.

Reject responses where out_len places sidsbuf_end past the received iov
or causes pointer wraparound.

Fixes: a90f37e3d7ac ("smb: client: parse owner/group when creating reparse points")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2inode.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 98ea5c6c34af..c7a2dddb3959 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -77,6 +77,17 @@ static int parse_posix_sids(struct cifs_open_info_data *data,
 
 	sidsbuf = (u8 *)qi + le16_to_cpu(qi->OutputBufferOffset) + qi_len;
 	sidsbuf_end = sidsbuf + out_len - qi_len;
+	if (sidsbuf_end < sidsbuf) {
+		cifs_dbg(VFS, "%s: server-supplied out_len %u caused pointer wraparound\n",
+			 __func__, out_len);
+		return -EINVAL;
+	}
+	if (sidsbuf_end > (u8 *)rsp_iov->iov_base + rsp_iov->iov_len) {
+		cifs_dbg(VFS, "%s: server-supplied out_len %u overruns iov by %td bytes\n",
+			 __func__, out_len,
+			 sidsbuf_end - ((u8 *)rsp_iov->iov_base + rsp_iov->iov_len));
+		return -EINVAL;
+	}
 
 	owner_len = posix_info_sid_size(sidsbuf, sidsbuf_end);
 	if (owner_len == -1)
-- 
2.55.0

[PATCH v3 08/11] smb: client: fix underflow in is_valid_oplock_break() notify offset check

[Frank Sorenson]

The check intended to bound data_offset against the received frame:

    if (data_offset > len - sizeof(struct file_notify_information))

underflows when len < sizeof(struct file_notify_information): the
subtraction wraps to a large value, the condition is false, and pnotify
is constructed from an unchecked data_offset.

Check len < sizeof(struct file_notify_information) first.

Fixes: 097f5863b1a0 ("cifs: read overflow in is_valid_oplock_break()")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb1misc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb1misc.c b/fs/smb/client/smb1misc.c
index cdfbbff24b72..a39c9140ceca 100644
--- a/fs/smb/client/smb1misc.c
+++ b/fs/smb/client/smb1misc.c
@@ -86,7 +86,8 @@ is_valid_oplock_break(char *buffer, struct TCP_Server_Info *srv)
 		if (get_bcc(buf) > sizeof(struct file_notify_information)) {
 			data_offset = le32_to_cpu(pSMBr->DataOffset);
 
-			if (data_offset >
+			if (len < sizeof(struct file_notify_information) ||
+			    data_offset >
 			    len - sizeof(struct file_notify_information)) {
 				cifs_dbg(FYI, "Invalid data_offset %u\n",
 					 data_offset);
-- 
2.55.0

[PATCH v3 09/11] smb: client: fix potential OOB read in smb3_enum_snapshots()

[Frank Sorenson]

When snapshot_array_size < GMT_TOKEN_SIZE, smb3_enum_snapshots()
unconditionally sets ret_data_len = sizeof(struct smb_snapshot_array),
silently inflating it beyond the actual server response size if the
server returned fewer bytes.  The subsequent copy_to_user() then reads
past the end of retbuf.

Reject responses shorter than sizeof(struct smb_snapshot_array) with -EIO.

Fixes: dcbf1c8af18a ("smb3: add ioctl to get a list of snapshots for volume or share")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2ops.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index efc68c7aef8a..c866d7c8c7dd 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2382,8 +2382,14 @@ smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
 		 * and retry the ioctl again with larger array size sufficient
 		 * to hold all of the snapshot GMT tokens on the second try.
 		 */
-		if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE)
+		if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE) {
+			if (ret_data_len < sizeof(struct smb_snapshot_array)) {
+				rc = -EIO;
+				kfree(retbuf);
+				return rc;
+			}
 			ret_data_len = sizeof(struct smb_snapshot_array);
+		}
 
 		/*
 		 * We return struct SRV_SNAPSHOT_ARRAY, followed by
-- 
2.55.0

[PATCH v3 10/11] smb: client: fix incomplete bounds check on reparse buffer in cifs_query_reparse_point()

[Frank Sorenson]

The start >= end check before casting to struct reparse_data_buffer *
ensures the pointer is in range but not that end - start >= sizeof(*buf).
A server-supplied DataOffset leaving fewer than 8 bytes passes the check
but allows OOB reads of ReparseTag and ReparseDataLength.

Fold the minimum-size check into the existing condition using `end`.

Fixes: c13b779d26b3 ("cifs: Fix validation of SMB1 query reparse point response")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/cifssmb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f5aad5f61dce..acb533383a3e 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3064,7 +3064,7 @@ int cifs_query_reparse_point(const unsigned int xid,
 
 	end = 2 + get_bcc(&io_rsp->hdr) + (__u8 *)&io_rsp->ByteCount;
 	start = (__u8 *)&io_rsp->hdr.Protocol + data_offset;
-	if (start >= end) {
+	if (start >= end || (size_t)(end - start) < sizeof(*buf)) {
 		rc = smb_EIO2(smb_eio_trace_qreparse_data_area,
 			      (unsigned long)start - (unsigned long)io_rsp,
 			      (unsigned long)end - (unsigned long)io_rsp);
-- 
2.55.0

[PATCH v3 11/11] smb: client: fix NameOffset and Next field validation in smb2_parse_contexts()

[Frank Sorenson]

Three related bounds issues in smb2_parse_contexts():

NameOffset: the existing 'noff + nlen > doff' check doesn't prevent
the name from starting inside the context header or extending past rem.
Replace it with noff >= sizeof(*cc) and noff + nlen <= rem, keeping
the name-before-data guard only when DataLength is non-zero.

Next: a non-zero Next smaller than sizeof(*cc) passes the existing
upper-bound check but creates an overlapping context pointer.  Add
off < sizeof(*cc) to reject it.

Dispatch: when DataLength is zero, calling a handler that reads the
data area (parse_lease_buf, parse_query_id_ctxt, parse_posix_ctxt)
reads stale memory past the declared context.  A rogue server can
craft a zero-DataLength context whose name matches a known tag (e.g.
'RqLs') and bypass the name-vs-data-offset guard, causing
parse_lease_buf to record a lease key and state from uninitialized
kmalloc content.  Gate all three dispatch paths on dlen > 0.

Fixes: af1689a9b770 ("smb: client: fix potential OOBs in smb2_parse_contexts()")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/smb2pdu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index dea05aeb53a1..dda7a1a88b44 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -2459,22 +2459,23 @@ int smb2_parse_contexts(struct TCP_Server_Info *server,
 
 		noff = le16_to_cpu(cc->NameOffset);
 		nlen = le16_to_cpu(cc->NameLength);
-		if (noff + nlen > doff)
+		if (noff < sizeof(*cc) || noff + nlen > rem ||
+		    (dlen && noff + nlen > doff))
 			return -EINVAL;
 
 		name = (char *)cc + noff;
 		switch (nlen) {
 		case 4:
-			if (!strncmp(name, SMB2_CREATE_REQUEST_LEASE, 4)) {
+			if (dlen && !strncmp(name, SMB2_CREATE_REQUEST_LEASE, 4)) {
 				*oplock = server->ops->parse_lease_buf(cc, epoch,
 								       lease_key);
-			} else if (buf &&
+			} else if (dlen && buf &&
 				   !strncmp(name, SMB2_CREATE_QUERY_ON_DISK_ID, 4)) {
 				parse_query_id_ctxt(cc, buf);
 			}
 			break;
 		case 16:
-			if (posix && !memcmp(name, smb3_create_tag_posix, 16))
+			if (dlen && posix && !memcmp(name, smb3_create_tag_posix, 16))
 				parse_posix_ctxt(cc, buf, posix);
 			break;
 		default:
@@ -2488,7 +2489,7 @@ int smb2_parse_contexts(struct TCP_Server_Info *server,
 		off = le32_to_cpu(cc->Next);
 		if (!off)
 			break;
-		if (check_sub_overflow(rem, off, &rem))
+		if (off < sizeof(*cc) || check_sub_overflow(rem, off, &rem))
 			return -EINVAL;
 		cc = (struct create_context *)((u8 *)cc + off);
 	}
-- 
2.55.0

Re: [PATCH v3 02/11] smb: client: validate PDU length before smb2_get_data_area_len() struct access

[Namjae Jeon]

> +/* Enforce: smb2_min_pdu_len[x] != 0 for every x where has_smb2_data_area[x]. */
> +static void __maybe_unused smb2_check_min_pdu_len_table(void)
> +{
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_NEGOTIATE_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_SESSION_SETUP_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_CREATE_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_READ_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_IOCTL_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_QUERY_DIRECTORY_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_CHANGE_NOTIFY_HE]);
> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_QUERY_INFO_HE]);
> +}
> +
I don't understand why this helper is necessary, and I don't prefer it.

Re: [PATCH v3 11/11] smb: client: fix NameOffset and Next field validation in smb2_parse_contexts()

[Namjae Jeon]

>                 case 16:
> -                       if (posix && !memcmp(name, smb3_create_tag_posix, 16))
> +                       if (dlen && posix && !memcmp(name, smb3_create_tag_posix, 16))
Could we check that dlen is large enough for the fixed POSIX fields
before calling parse_posix_ctxt()? It reads 12 bytes even when
DataLength is smaller.
>                                 parse_posix_ctxt(cc, buf, posix);
>                         break;
>                 default:

Re: [PATCH v3 11/11] smb: client: fix NameOffset and Next field validation in smb2_parse_contexts()

[Frank Sorenson]

On 8/29/26 9:01 AM, Namjae Jeon wrote:
>>                  case 16:
>> -                       if (posix && !memcmp(name, smb3_create_tag_posix, 16))
>> +                       if (dlen && posix && !memcmp(name, smb3_create_tag_posix, 16))
> Could we check that dlen is large enough for the fixed POSIX fields
> before calling parse_posix_ctxt()? It reads 12 bytes even when
> DataLength is smaller.

so this needs to be:
                      if (dlen > 12 && posix && !memcmp(name, smb3_create_tag_posix, 16))

I think we'll need similar before the calls to the 
server->ops->parse_lease_buf:
                     if (dlen && !strncmp(name, 
SMB2_CREATE_REQUEST_LEASE, 4)) {                                   
*oplock = server->ops->parse_lease_buf(cc, epoch,                       
                                                    lease_key);

for smb2: smb2_parse_lease_buf(void *buf, __u16 *epoch, char *lease_key) 
{         struct create_lease *lc = (struct create_lease *)buf;         
*epoch = 0; /* not used */         if (lc->lcontext.LeaseFlags & 
SMB2_LEASE_FLAG_BREAK_IN_PROGRESS_LE)                 return 
SMB2_OPLOCK_LEVEL_NOCHANGE;         return 
le32_to_cpu(lc->lcontext.LeaseState);

/* See MS-SMB2 2.2.13.2.8 */ struct lease_context {         __u8 
LeaseKey[SMB2_LEASE_KEY_SIZE];         __le32 LeaseState;         __le32 
LeaseFlags;         __le64 LeaseDuration; } __packed;

for smb2, we only access through LeaseFlags (24 bytes), but the entire 
struct is 32

static __u8 smb3_parse_lease_buf(void *buf, __u16 *epoch, char 
*lease_key) {         struct create_lease_v2 *lc = (struct 
create_lease_v2 *)buf;         *epoch = le16_to_cpu(lc->lcontext.Epoch); 
         if (lc->lcontext.LeaseFlags & 
SMB2_LEASE_FLAG_BREAK_IN_PROGRESS_LE)                 return 
SMB2_OPLOCK_LEVEL_NOCHANGE;         if (lease_key)                 
memcpy(lease_key, &lc->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE);         
return le32_to_cpu(lc->lcontext.LeaseState);

/* See MS-SMB2 2.2.13.2.10 */ struct lease_context_v2 {         __u8 
LeaseKey[SMB2_LEASE_KEY_SIZE];         __le32 LeaseState;         __le32 
LeaseFlags;         __le64 LeaseDuration;         __u8 
ParentLeaseKey[SMB2_LEASE_KEY_SIZE];         __le16 Epoch;         
__le16 Reserved; } __packed; and for smb3, we'll need through Epoch, so

     SMB2_LEASE_KEY_SIZE + 4 + 4 + 8 + SMB2_LEASE_KEY_SIZE + 2 = 50,

with the entire struct being 52 bytes

and also before the call to parse_query_id_ctxt():
                           } else if (dlen > 16 && buf &&               
                        !strncmp(name, SMB2_CREATE_QUERY_ON_DISK_ID, 4)) 
{                                   parse_query_id_ctxt(cc, buf);
parse_query_id_ctxt(struct create_context *cc, struct smb2_file_all_info *buf)
{
         struct create_disk_id_rsp *pdisk_id = (struct create_disk_id_rsp *)cc;

         cifs_dbg(FYI, "parse query id context 0x%llx 0x%llx\n",
                 pdisk_id->DiskFileId, pdisk_id->VolumeId);
         buf->IndexNumber = pdisk_id->DiskFileId;

/* See MS-SMB2 2.2.14.2.9 */
struct create_disk_id_rsp {
         struct create_context_hdr ccontext;
         __u8   Name[8];
         __le64 DiskFileId;
         __le64 VolumeId;
         __u8  Reserved[16];
} __packed;


With varying sizes, how do we handle the check prior to the call to the version-specificserver->ops->parse_lease_buf ?


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

Re: [PATCH v3 11/11] smb: client: fix NameOffset and Next field validation in smb2_parse_contexts()

[Frank Sorenson]

On 8/29/26 11:26 AM, Frank Sorenson wrote:
>
> On 8/29/26 9:01 AM, Namjae Jeon wrote:
>>>                  case 16:
>>> -                       if (posix && !memcmp(name, 
>>> smb3_create_tag_posix, 16))
>>> +                       if (dlen && posix && !memcmp(name, 
>>> smb3_create_tag_posix, 16))
>> Could we check that dlen is large enough for the fixed POSIX fields
>> before calling parse_posix_ctxt()? It reads 12 bytes even when
>> DataLength is smaller. 


make that 24 bytes for smb2 and 50 bytes for smb3


so if we test inside the version-specific function, can we simply return 
0 if the DataLength is not large enough:


smb2_parse_lease_buf(void *buf, __u16 *epoch, char *lease_key)
{
     struct create_lease *lc = (struct create_lease *)buf;

     *epoch = 0;
     if (le32_to_cpu(lc->ccontext.DataLength) < 24)
         return 0;


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

Re: [PATCH v3 11/11] smb: client: fix NameOffset and Next field validation in smb2_parse_contexts()

[Namjae Jeon]

On Sun, Aug 30, 2026 at 1:52 AM Frank Sorenson <sorenson@redhat.com> wrote:
>
>
> On 8/29/26 11:26 AM, Frank Sorenson wrote:
> >
> > On 8/29/26 9:01 AM, Namjae Jeon wrote:
> >>>                  case 16:
> >>> -                       if (posix && !memcmp(name,
> >>> smb3_create_tag_posix, 16))
> >>> +                       if (dlen && posix && !memcmp(name,
> >>> smb3_create_tag_posix, 16))
> >> Could we check that dlen is large enough for the fixed POSIX fields
> >> before calling parse_posix_ctxt()? It reads 12 bytes even when
> >> DataLength is smaller.
>
>
> make that 24 bytes for smb2 and 50 bytes for smb3
>
>
> so if we test inside the version-specific function, can we simply return
> 0 if the DataLength is not large enough:
>
>
> smb2_parse_lease_buf(void *buf, __u16 *epoch, char *lease_key)
> {
>      struct create_lease *lc = (struct create_lease *)buf;
>
>      *epoch = 0;
>      if (le32_to_cpu(lc->ccontext.DataLength) < 24)
>          return 0;
Right. ksmbd has similar validation in smb2_find_context_vals() and
parse_lease_state(). please refer to those functions. And DataOffset
also needs to be validated, Please check how ksmbd handles it in
smb2_find_context_vals() and parse_lease_state().

Re: [PATCH v3 00/11] smb: client: fix OOB reads and UAFs in SMB2/3 receive paths

[Namjae Jeon]

On Thu, Aug 27, 2026 at 12:32 AM Frank Sorenson <sorenson@redhat.com> wrote:
>
> This series fixes eleven bounds-checking defects in the SMB2/3 client,
> all reachable from a malicious or compromised server.
Looks good to me except for two patches(leave the comments), but do
you have any test results? Incorrectly applying validation patches can
lead to critical issues that do not work normal operation. It would be
helpful if you included test results with xfstests or Windows clients
in the cover letter.

Re: [PATCH v3 02/11] smb: client: validate PDU length before smb2_get_data_area_len() struct access

[Frank Sorenson]

On 8/29/26 5:31 AM, Namjae Jeon wrote:
>> +/* Enforce: smb2_min_pdu_len[x] != 0 for every x where has_smb2_data_area[x]. */
>> +static void __maybe_unused smb2_check_min_pdu_len_table(void)
>> +{
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_NEGOTIATE_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_SESSION_SETUP_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_CREATE_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_READ_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_IOCTL_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_QUERY_DIRECTORY_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_CHANGE_NOTIFY_HE]);
>> +       BUILD_BUG_ON(!smb2_min_pdu_len[SMB2_QUERY_INFO_HE]);
>> +}
>> +
> I don't understand why this helper is necessary, and I don't prefer it.

Understood.  I'll eliminate it in the next rev.

Frank

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