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

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

[Frank Sorenson]

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

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

Patch 1 fixes multiple pointer lifecycle and bounds-checking issues
in receive_encrypted_standard(), including a stale next_buffer pointer
that causes a UAF on error paths, and an integer overflow that allows
malformed trailing slices to bypass length checks.

Patch 2 adds a table of per-command minimum-response struct sizes,
used 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 so that smb2_check_message()
validates against the actual per-sub-PDU size, rather than the full
remaining compound tail. Without this, a truncated non-last sub-PDU
could bypass the guards added in patch 2.

Note for stable: although patch 3 carries a Fixes: tag and Cc: stable,
I am not sure whether patch 2 should, since it adds a new table rather
than being a true fix. They are complementary: patch 3 supplies the
correct per-sub-PDU length that patch 2's guard consumes. Patch 3 is
safe on its own, but its computed length will not be checked against
the per-command minimum unless patch 2 is included as well.

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

The create-context patch carried through v3 as patch 11 has been dropped
from this series. Zihan Xi's recent series:

  [PATCH v3 0/2] smb: client: fix create context out-of-bounds reads
  https://lore.kernel.org/r/cover.1788516372.git.zihanx@nebusec.ai

covers the same defects, arrives with a PoC, and fixes
parse_query_id_ctxt() in a better way. Rather than post two
overlapping/competing fixes, I will help with reviewing and improving
that series instead. If it stalls, I'll re-post my version.

Testing compared cifs-next (7.2+) with and without this patchset:

samba - v3.1.1, v3.1.1+sign, v3.1.1+seal, v2.1, v2.1+sign, v1:
  - no new failures attributable to this series.
  - found netfs bug causing generic/759 with signing to fail
    (resolved by David Howells--now succeeds).

Windows Server 2022 - v3.1.1, v3.1.1+sign, v3.1.1+seal,
  v3.1.1+multichannel:
  - no failures.

v4 changes:
 - patch 2: dropped the smb2_check_min_pdu_len_table() BUILD_BUG_ON helper.
 - dropped patch 11 ("smb: client: fix OOB reads in smb2_parse_contexts()")
   in favor of Zihan Xi's series, as described above.
 - patch 9: corrected a bogus Fixes: tag. Explained bound choice of
   sizeof(struct smb_snapshot_array) vs the 16-byte MIN_SNAPSHOT_ARRAY_SIZE
   of MS-SMB2 3.3.5.15.1.
 - testing details
 - commit subjects and messages tightened throughout.

v3 changes:
 https://lore.kernel.org/linux-cifs/20260826153147.4112943-1-sorenson@redhat.com
 - respin entire series

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

Frank Sorenson (10):
  smb: client: fix next_buffer UAF and NextCommand bounds in compound
    PDUs
  smb: client: validate minimum PDU size before smb2_get_data_area_len()
  smb: client: fix server->total_read for compound encrypted PDUs
  smb: client: fix missing lower-bound check on DFS referral string
    offsets
  smb: client: reject short Next offsets 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 reparse buffer bounds in cifs_query_reparse_point()

 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  | 40 ++++++++++++++++++++++++++++++
 fs/smb/client/smb2ops.c   | 51 +++++++++++++++++++++++++++++----------
 fs/smb/client/trace.h     |  1 +
 7 files changed, 103 insertions(+), 17 deletions(-)

-- 
2.55.0

[PATCH v4 01/10] smb: client: fix next_buffer UAF and NextCommand bounds in compound PDUs

[Frank Sorenson]

Fix several related bounds checking and pointer lifecycle issues in
receive_encrypted_standard()'s handling of compound encrypted frames:

- Clear next_buffer after assigning it to server->bigbuf. A stale
  next_buffer pointer can lead to a use-after-free on subsequent
  error paths.
- Update pdu_length to the decrypted plaintext size (buf_size). Using
  the pre-decryption length allows NextCommand to point into stale
  ciphertext residue.
- Reject next_cmd values smaller than MID_HEADER_SIZE(server).
- Fix an integer overflow in the upper bound check by verifying
  pdu_length - next_cmd < MID_HEADER_SIZE(server), ensuring the
  trailing slice is large enough for a header.

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 cb4fd09f996e..fcf7033889c7 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -5365,6 +5365,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:
@@ -5377,8 +5378,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
@@ -5414,6 +5422,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 v4 02/10] smb: client: validate minimum PDU size before smb2_get_data_area_len()

[Frank Sorenson]

__smb2_calc_size() calls smb2_get_data_area_len(), which reads
command-specific struct fields to locate the data area. However,
smb2_check_message() only validates StructureSize2, meaning a truncated
response could cause smb2_get_data_area_len() to read out-of-bounds.

Add smb2_min_pdu_len[] to track the size of the fixed response struct for
each command with a data area, and reject PDUs shorter than this minimum
before they are parsed.

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

diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index 9068175e57cd..22afc203a1ef 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -85,6 +85,36 @@ 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,
+};
+
 #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 +263,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 v4 03/10] smb: client: fix server->total_read for compound encrypted PDUs

[Frank Sorenson]

In receive_encrypted_standard(), server->total_read is left at the
full decrypted frame size when walking sub-PDUs of a compound encrypted
frame. As a result, cifs_handle_standard() passes this full size
to smb2_check_message(), causing the PDU length guards to incorrectly
validate the entire compound frame instead of the current sub-PDU.

This allows truncated non-last sub-PDUs to bypass length validation,
leading to out-of-bounds reads in smb2_get_data_area_len().

Fix this by setting server->total_read to the true length of the
current sub-PDU: next_cmd for non-last sub-PDUs, and the remaining
pdu_length for the last one.

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 fcf7033889c7..7f2177f6fc01 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -5371,6 +5371,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 v4 04/10] smb: client: fix missing lower-bound check on DFS referral string offsets

[Frank Sorenson]

parse_dfs_referrals() checks that DfsPathOffset and NetworkAddressOffset
do not exceed the buffer end, but fails to check that they don't point
inside the referral header itself.

If a server provides an offset smaller than sizeof(struct dfs_referral_level_3),
the derived string pointer overlaps with the struct fields, causing
cifs_strndup_from_utf16() to interpret header data as UTF-16 strings.

Fix this by enforcing that string offsets are 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 945194fe7a97..05168284f205 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -788,7 +788,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;
 		}
@@ -802,7 +806,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 v4 05/10] smb: client: reject short Next offsets in parse_server_interfaces()

[Frank Sorenson]

In parse_server_interfaces(), the server-supplied Next offset is
validated against bytes_left, but not against the size of the interface
structure itself.

A small, non-zero Next value can pass the bounds check but advance the
pointer by less than sizeof(*p). This causes the next iteration of the
loop to read misaligned, overlapping structure fields.

Fix this by ensuring the Next offset is at least sizeof(*p).

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 7f2177f6fc01..bda940cb3784 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 v4 06/10] smb: client: fix OOB struct field reads in move_smb2_ea_to_cifs()

[Frank Sorenson]

In move_smb2_ea_to_cifs(), the while (src_size > 0) loop condition is
insufficient. It allows iteration to continue even if the remaining
src_size is too small to contain a complete smb2_ea_info structure.
Consequently, reads of ea_name_length and ea_value_length can occur
out-of-bounds.

Fix this by ensuring src_size >= sizeof(*src) before attempting to read
any structure fields. Additionally, reject any next_entry_offset that is
smaller than sizeof(*src) or that would advance the pointer beyond the
available buffer.

Note that for calls where the server returns a malformed EA list, the
error returned to userspace changes from -ENODATA (getxattr) or
-ERANGE (listxattr) to -EIO. This correctly signals a server protocol
error rather than misleadingly indicating "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 bda940cb3784..ee3c98e3f316 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 b442cccd1530..bb8d0197cb54 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 v4 07/10] smb: client: fix missing iov bounds check in parse_posix_sids()

[Frank Sorenson]

In parse_posix_sids(), sidsbuf_end is calculated using the server-supplied
out_len without being validated against the actual length of the received
iov (iov_len).

If a server provides an inflated out_len, sidsbuf_end will point past the
end of the iov. This defeats the bounds guards in posix_info_sid_size(),
allowing out-of-bounds reads into adjacent kernel memory.

Fix this by rejecting responses where the calculated sidsbuf_end would
exceed the received iov boundaries or cause 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 96063e355186..13fe8e3b48f3 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 v4 08/10] smb: client: fix underflow in is_valid_oplock_break() notify offset check

[Frank Sorenson]

The bounds check intended to validate data_offset against the received
frame length:

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

suffers from an integer underflow if len is smaller than the size of the
file_notify_information structure. This allows a malicious or malformed
packet to bypass the check and construct pnotify using an invalid,
unchecked data_offset.

Fix this by explicitly checking that len is at least the size of
struct file_notify_information before evaluating the data_offset.

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 v4 09/10] smb: client: fix potential OOB read in smb3_enum_snapshots()

[Frank Sorenson]

If snapshot_array_size is smaller than GMT_TOKEN_SIZE, smb3_enum_snapshots()
sets ret_data_len to sizeof(struct smb_snapshot_array) without verifying
the actual length of the server's reply.

Because SMB2_ioctl() places no lower bound on the server-supplied OutputCount
and allocates retbuf to exactly that length, a short reply results in
ret_data_len exceeding the size of retbuf. The subsequent copy_to_user()
then reads past the end of retbuf, leaking adjacent slab memory to userspace.
The subsequent clamp check is ineffective as it only reduces ret_data_len.

Fix this by rejecting replies shorter than sizeof(struct smb_snapshot_array)
with -EIO. Note that the bound is set to the 12-byte struct size rather than
the 16-byte MIN_SNAPSHOT_ARRAY_SIZE defined in MS-SMB2 3.3.5.15.1, because
12 bytes is exactly what copy_to_user() attempts to read to ensure memory
safety.

Fixes: e02789a53d71 ("smb3: enumerating snapshots was leaving part of the data off end")
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 ee3c98e3f316..3464470d3297 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2463,8 +2463,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 v4 10/10] smb: client: fix reparse buffer bounds in cifs_query_reparse_point()

[Frank Sorenson]

In cifs_query_reparse_point(), the start >= end check before casting to
struct reparse_data_buffer * only ensures the start pointer is within the
response. It fails to verify that there is enough space remaining for the
fixed 8-byte header of the structure.

If a server provides a DataOffset that leaves less than 8 bytes remaining,
the check passes, but subsequent reads of ReparseTag and ReparseDataLength
will occur out-of-bounds.

Fix this by ensuring the remaining space is at least the size of the
reparse_data_buffer structure before accessing its fields.

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 f9aff0712794..6dddbd84b93b 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3080,7 +3080,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

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

[Frank Sorenson]

Notes on the Sashiko findings (https://sashiko.dev/#/patchset/20260913214510.3071370-1-sorenson%40redhat.com):

patch:

  1) pre-existing; memory leak

  2) pre-existing; OOB read + memory leak

  3) 3 findings
     a. pre-existing; could incorrect iov_len be propagated
     b. pre-existing; error handler could leak memory
     c. pre-existing; could uninitialized heap memory be leaked
        to user-space?

  4) does the lower bound protect against header data parsed as
       UTF-16 strings with multiple referrals?
     Sashiko is correct, however the read stays in bounds so the
       result would be a garbage path, not an OOB access.
     Separate hardening could be done, if exact bound is desired.

  5) pre-existing; potential race condition
  8) 4 findings:
     a. pre-existing; can uninitialized memory be read on truncated
        packets?
     b. is the new check possible to hit?
        *** Sashiko is correct; the new check will never be true ***

     c. pre-existing; can logging via %s format string trigger OOB read
     d. pre-existing; do malformed packets bypass error handling and
        get processed as oplock breaks?
10) pre-existing; does bounds check rely on a structural offset to
     calculate end, potentially leading to leak of uninitialized heap
     memory?

Recommendation:
   patch 4 is still good
   drop patch 8 entirely--it's a dead check

The remainder of the findings are for pre-existing issues; could be
investigated & addressed in future work.


I can respin v5 without patch 8, or (assuming there isn't further
work to do on the rest of the patches) we can simply drop patch 8.


Frank

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

Re: [PATCH v4 02/10] smb: client: validate minimum PDU size before smb2_get_data_area_len()

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> __smb2_calc_size() calls smb2_get_data_area_len(), which reads
> command-specific struct fields to locate the data area. However,
> smb2_check_message() only validates StructureSize2, meaning a truncated
> response could cause smb2_get_data_area_len() to read out-of-bounds.
>
> Add smb2_min_pdu_len[] to track the size of the fixed response struct for
> each command with a data area, and reject PDUs shorter than this minimum
> before they are parsed.
>
> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
> ---
>  fs/smb/client/smb2misc.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
> index 9068175e57cd..22afc203a1ef 100644
> --- a/fs/smb/client/smb2misc.c
> +++ b/fs/smb/client/smb2misc.c
> @@ -85,6 +85,36 @@ 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,
> +};

I don't understand why you're creating a new array.

What about to replace @has_smb2_data_area with the above array, then
have something like

        #define smb2_has_data_area(cmd) (smb2_min_pdu_len[cmd] != 0)

and in __smb2_calc_size()

        if (!smb2_has_data_area(le16_to_cpu(shdr->Command)))
                ....

Then we don't need to worry about keeping both arrays in sync.

> +
>  #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 +263,16 @@ smb2_check_message(char *buf, unsigned int pdu_len, unsigned int len,
>  		}
>  	}
>  
> +	if ((shdr->Status == 0 ||

nit: shdr->Status == STATUS_SUCCESS

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

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

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> I can respin v5 without patch 8, or (assuming there isn't further
> work to do on the rest of the patches) we can simply drop patch 8.

You can keep patch 4 and drop 8 in the next version.  Thanks.

Re: [PATCH v4 02/10] smb: client: validate minimum PDU size before smb2_get_data_area_len()

[Frank Sorenson]

On 9/15/26 10:04 AM, Paulo Alcantara wrote:
> I don't understand why you're creating a new array.
>
> What about to replace @has_smb2_data_area with the above array, then
> have something like
>
>          #define smb2_has_data_area(cmd) (smb2_min_pdu_len[cmd] != 0)
>
> and in __smb2_calc_size()
>
>          if (!smb2_has_data_area(le16_to_cpu(shdr->Command)))
>                  ....
>
> Then we don't need to worry about keeping both arrays in sync.

*facepalm*  yes, of course.  I will fix.


Frank

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

Re: [PATCH v4 02/10] smb: client: validate minimum PDU size before smb2_get_data_area_len()

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> On 9/15/26 10:04 AM, Paulo Alcantara wrote:
>> I don't understand why you're creating a new array.
>>
>> What about to replace @has_smb2_data_area with the above array, then
>> have something like
>>
>>          #define smb2_has_data_area(cmd) (smb2_min_pdu_len[cmd] != 0)
>>
>> and in __smb2_calc_size()
>>
>>          if (!smb2_has_data_area(le16_to_cpu(shdr->Command)))
>>                  ....
>>
>> Then we don't need to worry about keeping both arrays in sync.
>
> *facepalm*  yes, of course.  I will fix.

Thanks.  Please also check if you could replace some of the cifs_dbg()
calls with cifs_server_dbg() or cifs_tcon_dbg(), respectively.  It could
be useful to log the hostnames or UNCs from such broken servers.

Re: [PATCH v4 02/10] smb: client: validate minimum PDU size before smb2_get_data_area_len()

[Frank Sorenson]

On 9/16/26 9:22 AM, Paulo Alcantara wrote:
> Thanks.  Please also check if you could replace some of the cifs_dbg()
> calls with cifs_server_dbg() or cifs_tcon_dbg(), respectively.  It could
> be useful to log the hostnames or UNCs from such broken servers.

I can replace all the cifs_dbg() calls in smb2_check_message() easily,
and in parse_server_interfaces() with just a little work, but the others
would require plumbing in either the server or ses variables to the
functions.

I'll change the cifs_dbg() I'm adding in smb2_check_message, and
then make a couple follow-up patches to convert the existing ones.

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