[PATCH 0/7] smb: client: fix OOB and use-after-free infoleaks in SMB1 FIND responses

[PATCH 0/7] smb: client: fix OOB and use-after-free infoleaks in SMB1 FIND responses

[Diego Oliva]

The SMB1 client parses TRANS2_FIND_FIRST2 and TRANS2_FIND_NEXT2
responses without checking them against the number of bytes it
received. A malicious or compromised server can make the client read
past the end of a response, and past the end of the cifs_request
buffer holding it, and send what it read back to the server. Both the
out-of-bounds reads and the use-after-free read are information leaks:
the bytes leave the machine in the client's next FindNext request,
from a position the server picks:

 - LastNameOffset, which the client uses to locate the last entry of
   a response, is only checked against CIFSMaxBufSize, never against
   the response that was actually received, so psrch_inf->last_entry
   can be placed past the end of the allocation.
 - cifs_save_resume_key() parses the entry there without a bound. The
   name length comes from the entry itself, and for SMB_FIND_FILE_UNIX
   from a NUL scan of up to PATH_MAX + 1 UTF-16 units. The name is
   recorded as the resume name of the search, and the next
   CIFSFindNext() rejects only a recorded length of PATH_MAX or more
   before copying the name into its request and sending it, so up to
   4 KiB of memory from beyond the response leaves the machine,
   together with the resume key read from the same entry.
 - When the last entry is rejected, and when a search rewind frees
   the buffer without its FindFirst recording a new name, the resume
   name keeps pointing into a buffer that has been released, and the
   next CIFSFindNext() copies it from there.
 - The response parameters (search handle, entry count, end-of-search
   flag, LastNameOffset) and the data area are read at offsets that
   validate_t2() caps at 1024. It bounds the parameter and data
   counts by the byte count of the response, but nothing ties either
   area to the bytes that were received, so one can begin or end past
   them; those reads stay inside the allocation but can return stale
   bytes.

A FindNext response that reports no entries without ending the search
is enough to make the client send the resume name, and each further
response can do the same. Reaching the code needs a malicious or
compromised server, an explicit vers=1.0 mount of it
(CONFIG_CIFS_ALLOW_INSECURE_LEGACY, default y) and a directory listing
on that mount; SMB1 is not negotiated by default. The same code is in
every maintained stable tree.

Commit f8cf09a53a0d ("smb: client: bound dirent name against end of
SMB response in cifs_filldir"), in v7.2, bounded the name of the
entries cifs_readdir() emits, after the parse; the resume-key path was
left unbounded and the parse itself still ran before any bound.

The fix is one change conceptually, but as a single patch it is well
over the 100 lines with context that stable-kernel-rules.rst allows,
so stable could not take it. This series splits it into patches that
each fix one thing, build and pass checkpatch on their own, and stay
under that limit.

What each patch fixes, and what to backport:

  1/7 fix use-after-free infoleak via the readdir resume name
      The use-after-free. It comes first so that the patches after it
      cannot widen the window it closes. Also resets resume_key with
      the name and skips the zero-length copy in CIFSFindNext().
      Tagged Cc: stable.
  2/7 fix OOB last_entry pointer from unbounded LastNameOffset
      The out-of-bounds pointer, bounded against the response that
      was received rather than against the declared data area. It
      applies on top of 1/7, whose resets sit in its context and
      which makes the paths this patch sends more responses down
      safe. Also records the search handle before the last entry is
      examined, because the new no-entries case would otherwise lose
      the handle. Cc: stable.
  3/7 fix OOB read of the resume name sent in TRANS2_FIND_NEXT2
      Bounds the entry parse in cifs_fill_dirent() for both callers,
      before anything is read. This is what stops a name from being
      recorded and sent from past the end of the response.
      Tagged Cc: stable.
  4/7 fix OOB read in the SMB_FIND_FILE_UNIX name scan
      Caps the NUL scan at the response end; without it the scan
      still runs several KiB past the allocation. Depends on 3/7 for
      the end of the response. A scan that reaches the end of the
      response without finding a terminator is rejected; a name
      longer than the PATH_MAX cap of the scans is still truncated
      there, as before. Cc: stable.
  5/7 reject FIND data areas that run past the received response
      Hardening rather than memory safety: it stops entries being
      parsed from stale bytes inside the allocation, and bounds the
      data area that the nxt_dir_entry() walk starts from and that
      the two query helpers read their entry from. It rejects with
      -EINVAL, as validate_t2() does, logging with cifs_dbg(), and
      adds no tracepoint, so it applies to trees before 6.19.
      Cc: stable.
  6/7 reject short or out-of-range FIND response parameters
      The parameters are read inside the allocation, but the stale
      search handle among them is sent back to the server. Depends on
      5/7, whose declarations it extends. It returns smb_EIO2() with
      two new smb_eio_traces values; backports before v6.19 need
      -EINVAL instead. Cc: stable.
  7/7 drop the redundant name bounds in cifs_filldir()
      Cleanup of the two post-parse checks that 3/7 made redundant.
      Depends on 3/7. Not for stable.

No patch needs one that follows it, so the series can be cut after any
patch. Patches 1/7 to 4/7 are every out-of-bounds and use-after-free
fix; 5/7 stands on its own; 6/7 can follow where smb_EIO2() exists.
Trees before v7.2 need the adjustments noted in the patches that need
them.

cifs_fill_dirent() is shared with the SMB2 readdir path; 3/7 and 4/7
add checks there that a valid SMB2 response cannot fail, since
smb2_parse_query_directory() already walks its entries against the
end of the response.

Left out on purpose: an entry-size check for cifs_query_path_info()
and cifs_backup_query_path_info(), which still read one whole entry
from a data area that 5/7 bounds only as a whole (those reads stay
inside the allocation); passing the end to posix_info_parse() in
cifs_fill_dirent_posix(), which only SMB3.1.1 POSIX mounts reach,
with entries that num_entries() has already validated; and a lower
bound on ParameterOffset and DataOffset, since reads at a low offset
stay inside the received response.

Each patch builds with W=1 without warnings and passes checkpatch.
The out-of-bounds reads and the use-after-free read were reproduced
against a test server with KASAN enabled, by listing a directory on
an SMB1 mount; the splats are in 1/7, 2/7, 3/7 and 4/7, all taken on
the unpatched tree. The one in 4/7 shows the case the preceding
patches leave open, where the entry starts inside the response and
the scan still leaves the allocation.

The series is based on cifs-next at commit f14572c203d5 ("Merge tag
'cifs-fixes-7.3-rc5' of https://git.manguebit.org/linux"). The files it
touches are identical there and in v7.3-rc4.

Diego Oliva (7):
  smb: client: fix use-after-free infoleak via the readdir resume name
  smb: client: fix OOB last_entry pointer from unbounded LastNameOffset
  smb: client: fix OOB read of the resume name sent in TRANS2_FIND_NEXT2
  smb: client: fix OOB read in the SMB_FIND_FILE_UNIX name scan
  smb: client: reject FIND data areas that run past the received
    response
  smb: client: reject short or out-of-range FIND response parameters
  smb: client: drop the redundant name bounds in cifs_filldir()

 fs/smb/client/cifssmb.c | 78 ++++++++++++++++++++++++++++++++++-------
 fs/smb/client/readdir.c | 68 ++++++++++++++++++++++++-----------
 fs/smb/client/trace.h   |  2 ++
 3 files changed, 114 insertions(+), 34 deletions(-)


base-commit: f14572c203d57492e1d4e5d7851a3b143e083b82
-- 
2.39.5

[PATCH 1/7] smb: client: fix use-after-free infoleak via the readdir resume name

[Diego Oliva]

CIFSFindNext() copies the resume name that cifs_save_resume_key()
recorded from the last entry of the previous response into its request;
psrch_inf->presume_name points into the response buffer that entry came
from, and nothing ever clears it. find_cifs_entry() records a new
name only from a response with a usable last entry, and cifs_readdir()
only from an entry it walks to, so a response that carries no entries
and leaves last_entry NULL, because CIFSFindFirst() or CIFSFindNext()
rejected LastNameOffset, refreshes neither and presume_name keeps
pointing into the previous response. That buffer has already been
released: by CIFSFindNext() before it installs the new response, or by
find_cifs_entry() when it rewinds the search, which does not drop the
name either and can then fail in initiate_cifs_search() before a new
response replaces it. The next CIFSFindNext() then copies up to
PATH_MAX - 1 bytes of the resume name out of the released buffer into
its request and sends them to the server: a use-after-free read
that leaks kernel memory to a malicious or compromised server, which
can provoke it with a single FindNext response that carries no entries
and a LastNameOffset above CIFSMaxBufSize, which is all the existing
check rejects.

With KASAN enabled, a server that answers a listing that way gives:

 CIFS: VFS: ignoring corrupt resume name
 ==================================================================
 BUG: KASAN: slab-use-after-free in CIFSFindNext+0x8ca/0x14c0
 Read of size 4080 at addr ffff88807c823f98 by task ls/83

 CPU: 0 UID: 0 PID: 83 Comm: ls Tainted: G    B               7.3.0-rc4-00457-gf14572c203d5 #69 PREEMPT(lazy)
 Tainted: [B]=BAD_PAGE
 Call Trace:
  <TASK>
  kasan_report+0xdf/0x1a0
  kasan_check_range+0x10f/0x1e0
  __asan_memcpy+0x23/0x60
  CIFSFindNext+0x8ca/0x14c0
  cifs_readdir+0xf51/0x29c0
  iterate_dir+0x1c0/0x570
  __x64_sys_getdents64+0x133/0x270
  do_syscall_64+0x109/0x5d0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 Allocated by task 83 on cpu 1 at 13.497119s:
  cifs_buf_get+0x36/0x90
  smb_init+0x4f/0x110
  CIFSFindNext+0xf7/0x14c0
  cifs_readdir+0xf51/0x29c0

 Freed by task 83 on cpu 0 at 13.584018s:
  cifs_buf_release+0x41/0x80
  CIFSFindNext+0xfce/0x14c0
  cifs_readdir+0xf51/0x29c0

 The buggy address is located 16280 bytes inside of
  freed 16588-byte region [ffff88807c820000, ffff88807c8240cc)
 ==================================================================

The same memcpy() first reads past the end of that object while it is
still live, a slab-out-of-bounds read that the following patches
bound; that is the taint the report above carries.

SMB1 is not negotiated by default; reaching this code requires an
explicit vers=1.0 mount.

Clear presume_name and resume_name_len whenever a new response is
installed and when the rewind path releases the search buffer, so that
the name never outlives the buffer it points into, and reset
resume_key with them, since it was taken from the same entry and would
otherwise be sent with an empty name. A response without a usable
last entry then continues the search with an empty resume name rather
than a stale one; CIFSFindNext() skips the copy of a zero-length name
so that it never runs on the NULL pointer.

The next patch in the series leaves last_entry NULL for a response
that carries no entries, and bounds LastNameOffset against the
received response rather than against CIFSMaxBufSize. Responses that
are given a last entry today then take the path fixed here, so that
patch must not be applied without this one.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: b77d753c413e ("[CIFS] Check that last search entry resume key is valid")
Cc: <stable@vger.kernel.org>
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 9 ++++++++-
 fs/smb/client/readdir.c | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 6dddbd84b93b..67f033b960e6 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -4543,6 +4543,9 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 	psrch_inf->entries_in_buffer = le16_to_cpu(parms->SearchCount);
 	psrch_inf->index_of_last_entry = 2 /* skip . and .. */ +
 		psrch_inf->entries_in_buffer;
+	psrch_inf->presume_name = NULL;
+	psrch_inf->resume_name_len = 0;
+	psrch_inf->resume_key = 0;
 	lnoff = le16_to_cpu(parms->LastNameOffset);
 	if (CIFSMaxBufSize < lnoff) {
 		cifs_dbg(VFS, "ignoring corrupt resume name\n");
@@ -4607,7 +4610,8 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 	name_len = psrch_inf->resume_name_len;
 	params += name_len;
 	if (name_len < PATH_MAX) {
-		memcpy(pSMB->ResumeFileName, psrch_inf->presume_name, name_len);
+		if (name_len)
+			memcpy(pSMB->ResumeFileName, psrch_inf->presume_name, name_len);
 		byte_count += name_len;
 		/* 14 byte parm len above enough for 2 byte null terminator */
 		pSMB->ResumeFileName[name_len] = 0;
@@ -4662,6 +4666,9 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 	psrch_inf->endOfSearch = !!parms->EndofSearch;
 	psrch_inf->entries_in_buffer = le16_to_cpu(parms->SearchCount);
 	psrch_inf->index_of_last_entry += psrch_inf->entries_in_buffer;
+	psrch_inf->presume_name = NULL;
+	psrch_inf->resume_name_len = 0;
+	psrch_inf->resume_key = 0;
 	lnoff = le16_to_cpu(parms->LastNameOffset);
 	if (CIFSMaxBufSize < lnoff) {
 		cifs_dbg(VFS, "ignoring corrupt resume name\n");
diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index 9530e5b01564..6ab4e687c3e4 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -752,6 +752,9 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
 			cfile->srch_inf.ntwrk_buf_start = NULL;
 			cfile->srch_inf.srch_entries_start = NULL;
 			cfile->srch_inf.last_entry = NULL;
+			cfile->srch_inf.presume_name = NULL;
+			cfile->srch_inf.resume_name_len = 0;
+			cfile->srch_inf.resume_key = 0;
 		}
 		rc = initiate_cifs_search(xid, file, full_path);
 		if (rc) {
-- 
2.39.5

[PATCH 2/7] smb: client: fix OOB last_entry pointer from unbounded LastNameOffset

[Diego Oliva]

CIFSFindFirst() and CIFSFindNext() locate the last entry of a
TRANS2_FIND_FIRST2 or TRANS2_FIND_NEXT2 response by adding the
LastNameOffset of the response parameters to the start of the data
area, and store the result in psrch_inf->last_entry. MS-CIFS 2.2.6.2.2
defines that offset as the offset of the entry's file name; the client
follows the reading of NT and Samba servers, which point it at the
entry. LastNameOffset is only checked against CIFSMaxBufSize, which
says nothing about the response actually received: validate_t2() lets
the data area start up to 1024 bytes into the buffer, and SendReceive()
copies at most CIFSMaxBufSize + MAX_CIFS_HDR_SIZE bytes of a response,
16468 by default, into a cifs_request allocation of CIFSMaxBufSize +
MAX_SMB2_HDR_SIZE bytes, 16588 by default. The pointer itself can
therefore be placed anywhere from the end of the received data to 820
bytes past the end of the allocation.

The read through that pointer is far larger than its displacement.
find_cifs_entry() hands last_entry to cifs_save_resume_key() after every
FindNext and after the FindFirst of a search rewind, and
cifs_fill_dirent() takes the length of the name from the entry itself: a
__le32 FileNameLength for most levels, a __u8 for
SMB_FIND_FILE_INFO_STANDARD, and for SMB_FIND_FILE_UNIX the result of a
scan for the terminating NUL over up to PATH_MAX + 1 UTF-16 units. The
name is recorded as the resume name of the search, and the next
CIFSFindNext() rejects only a recorded length of PATH_MAX or more before
copying the name into the ResumeFileName of its request and sending it
with the resume key taken from the same entry: up to 4 KiB of memory
from past the end of the allocation leaks to the server, and for
SMB_FIND_FILE_UNIX the NUL scan runs about twice as far before the
length is even known. The entry does not have to lie outside the
response for that either: a maximal response ends about 120 bytes before
the end of the allocation, so an entry in its last bytes with a name
just under PATH_MAX has nearly all of that name copied from past the
allocation.

It needs nothing unusual from the client: a FindNext response that
reports no entries and does not end the search makes find_cifs_entry()
send the next FindNext at once, carrying whatever was parsed at
LastNameOffset, and a search rewind can do the same after its
FindFirst. SMB1 is not negotiated by default: this takes a malicious
or compromised server, a mount of it with an explicit vers=1.0, which
CONFIG_CIFS_ALLOW_INSECURE_LEGACY (default y) permits, and a directory
listing on that mount.

With KASAN enabled, a response that places the last entry past the
buffer, with a large DataOffset and a LastNameOffset near the end of
its range, gives:

 ==================================================================
 BUG: KASAN: slab-out-of-bounds in cifs_save_resume_key.isra.0+0x7aa/0x7f0
 Read of size 4 at addr ffff88807b55c33b by task ls/83

 CPU: 1 UID: 0 PID: 83 Comm: ls Not tainted 7.3.0-rc4-00457-gf14572c203d5 #69 PREEMPT(lazy)
 Call Trace:
  <TASK>
  kasan_report+0xdf/0x1a0
  cifs_save_resume_key.isra.0+0x7aa/0x7f0
  cifs_readdir+0x1ed6/0x29c0
  iterate_dir+0x1c0/0x570
  __x64_sys_getdents64+0x133/0x270
  do_syscall_64+0x109/0x5d0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 Allocated by task 83 on cpu 0 at 19.317556s:
  cifs_buf_get+0x36/0x90
  smb_init+0x4f/0x110
  CIFSFindNext+0xf7/0x14c0
  cifs_readdir+0xf51/0x29c0

 The buggy address belongs to the object at ffff88807b558000
  which belongs to the cache cifs_request of size 16588
 The buggy address is located 623 bytes to the right of
  allocated 16588-byte region [ffff88807b558000, ffff88807b55c0cc)
 ==================================================================

The four bytes read there are the FileNameLength of the entry, which
cifs_fill_dirent() takes before anything checks where the entry lies.

Ignore the last entry when it does not start inside the received
response or the response holds no entries; for a response without
entries, last_entry pointed into or past its empty data area and
cifs_save_resume_key() parsed whatever the buffer held there. The
entry is bounded by the response that was received, which is what the
buffer holds, rather than by the data area the response declares,
which a later patch checks against the received length. Record the
search handle before the last entry is examined, since a response
without a usable last entry still carries the handle that the search
is continued and closed with; leaving the assignment in the else
branch would drop the handle of every response without entries.

This bounds where the last entry starts, not how far it is parsed: an
entry that starts within the final bytes of the response still has
its fixed part read past the end of the received data and its name,
up to 4095 bytes, copied from past the end of the allocation, until
the following patch bounds the parse itself. A response without a
usable last entry continues the search with an empty resume name,
since the preceding patch, "smb: client: fix use-after-free infoleak
via the readdir resume name", drops the recorded name whenever a new
response is installed. This patch applies on top of that one and
relies on it: without it, every response rejected here would leave
the previous name in place, pointing into a buffer that has been
released.

A conforming server is not affected: the last entry lies inside the
data area of the response.

Fixes: 0752f1522a91 ("[CIFS] make sure we have the right resume info before calling CIFSFindNext")
Fixes: b77d753c413e ("[CIFS] Check that last search entry resume key is valid")
Cc: <stable@vger.kernel.org>
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 67f033b960e6..7eb83dd737fc 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -4546,14 +4546,17 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 	psrch_inf->presume_name = NULL;
 	psrch_inf->resume_name_len = 0;
 	psrch_inf->resume_key = 0;
+	if (pnetfid)
+		*pnetfid = parms->SearchHandle;
 	lnoff = le16_to_cpu(parms->LastNameOffset);
-	if (CIFSMaxBufSize < lnoff) {
+	if (!psrch_inf->entries_in_buffer) {
+		psrch_inf->last_entry = NULL;
+	} else if (psrch_inf->srch_entries_start + lnoff >=
+		   psrch_inf->ntwrk_buf_start + bytes_returned) {
 		cifs_dbg(VFS, "ignoring corrupt resume name\n");
 		psrch_inf->last_entry = NULL;
 	} else {
 		psrch_inf->last_entry = psrch_inf->srch_entries_start + lnoff;
-		if (pnetfid)
-			*pnetfid = parms->SearchHandle;
 	}
 	return 0;
 }
@@ -4670,7 +4673,10 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 	psrch_inf->resume_name_len = 0;
 	psrch_inf->resume_key = 0;
 	lnoff = le16_to_cpu(parms->LastNameOffset);
-	if (CIFSMaxBufSize < lnoff) {
+	if (!psrch_inf->entries_in_buffer) {
+		psrch_inf->last_entry = NULL;
+	} else if (psrch_inf->srch_entries_start + lnoff >=
+		   psrch_inf->ntwrk_buf_start + bytes_returned) {
 		cifs_dbg(VFS, "ignoring corrupt resume name\n");
 		psrch_inf->last_entry = NULL;
 	} else {
-- 
2.39.5

[PATCH 3/7] smb: client: fix OOB read of the resume name sent in TRANS2_FIND_NEXT2

[Diego Oliva]

cifs_fill_dirent() reads the fixed part of a directory entry and the
name length it holds, or for SMB_FIND_FILE_UNIX scans the name for its
terminating NUL, without knowing where the response ends.
cifs_save_resume_key() calls it for the entry at the LastNameOffset the
server chose, after every FindNext and after the FindFirst of a search
rewind, and records the name it returns as the resume name of the
search without bounding it; the next CIFSFindNext() copies up to
PATH_MAX - 1 bytes from that name into its request and sends them to
the server. cifs_filldir() bounds
the name against the end of the response since
commit f8cf09a53a0d ("smb: client: bound dirent name against end of
SMB response in cifs_filldir"), but only after cifs_fill_dirent() has
read the entry, and nxt_dir_entry() checks the entries it walks only
against sizeof(FILE_DIRECTORY_INFO), for every level but
SMB_FIND_FILE_INFO_STANDARD, and never sees the first entry of a
response.

Bounding where last_entry starts, as the preceding patch does, does
not bound the read. A last entry that starts within the final bytes of
the response still has its fixed part read past the end of the
received data, and the name length it declares, a __le32 for every
level but SMB_FIND_FILE_INFO_STANDARD and SMB_FIND_FILE_UNIX, is
recorded without being checked against anything. CIFSFindNext()
rejects only a length of PATH_MAX or more, then copies the name, up to
4095 bytes, into its request and sends it, and as the report below
shows that copy can run past the end of the cifs_request allocation
that holds the response. For SMB_FIND_FILE_UNIX the name is in
addition scanned for its terminating NUL over up to PATH_MAX + 1
UTF-16 units, or PATH_MAX bytes, before any check. The FindNext that
carries the name is only sent by the SMB1 dialect, which is not
negotiated by default; reaching it requires an explicit vers=1.0
mount.

With KASAN enabled, a response whose last entry declares a name
longer than the bytes that follow it gives:

 ==================================================================
 BUG: KASAN: slab-out-of-bounds in CIFSFindNext+0x8ca/0x14c0
 Read of size 4080 at addr ffff88807c823f98 by task ls/83

 CPU: 0 UID: 0 PID: 83 Comm: ls Not tainted 7.3.0-rc4-00457-gf14572c203d5 #69 PREEMPT(lazy)
 Call Trace:
  <TASK>
  kasan_report+0xdf/0x1a0
  kasan_check_range+0x10f/0x1e0
  __asan_memcpy+0x23/0x60
  CIFSFindNext+0x8ca/0x14c0
  cifs_readdir+0xf51/0x29c0
  iterate_dir+0x1c0/0x570
  __x64_sys_getdents64+0x133/0x270
  do_syscall_64+0x109/0x5d0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 Allocated by task 83 on cpu 1 at 13.497119s:
  cifs_buf_get+0x36/0x90
  smb_init+0x4f/0x110
  CIFSFindNext+0xf7/0x14c0
  cifs_readdir+0xf51/0x29c0

 The buggy address belongs to the object at ffff88807c820000
  which belongs to the cache cifs_request of size 16588
 The buggy address is located 16280 bytes inside of
  allocated 16588-byte region [ffff88807c820000, ffff88807c8240cc)
 ==================================================================

The read is the memcpy() that copies the recorded name into the next
request, so those bytes go to the server.

Pass the end of the response to cifs_fill_dirent() and have it reject
an entry whose fixed part does not fit or whose name extends past the
end, before either is used, so that both callers are covered;
cifs_save_resume_key() computes the end the same way cifs_readdir()
does. The name bound compares the bytes left between the name and the
end with the name length rather than adding the length to the name
pointer, since the length is a 32-bit value taken from the entry and
the sum could wrap on 32-bit systems. cifs_fill_dirent() is also used
on the SMB2 readdir path; there smb2_parse_query_directory() has
already checked every entry against the end of the response, so a
valid SMB2 response is not rejected by the new checks. The check
cifs_filldir() applies after the parse is left in place. The NUL scan
for SMB_FIND_FILE_UNIX names is capped by a separate patch.

An entry and its name lie inside the data area of the response, so a
server that points LastNameOffset at the entry is not affected. One
that points it at the file name, as MS-CIFS 2.2.6.2.2 describes, has
its last entry rejected here; the listing is unaffected, because
cifs_readdir() records the resume name from an entry it emitted, and
before this series such a response could instead end the listing early,
since a name length read from the wrong place is usually PATH_MAX or
more, which CIFSFindNext() rejects with -EINVAL.

On trees before v6.19 the type for the level 0x105 fixed part is
spelled SEARCH_ID_FULL_DIR_INFO; adjust when backporting.

Fixes: 0752f1522a91 ("[CIFS] make sure we have the right resume info before calling CIFSFindNext")
Cc: <stable@vger.kernel.org> # 6.1.x: f8cf09a53a0d: smb: client: bound dirent name against end of SMB response in cifs_filldir
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/readdir.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index 6ab4e687c3e4..fdbe22b24c47 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -586,30 +586,46 @@ static void cifs_fill_dirent_std(struct cifs_dirent *de,
 }
 
 static int cifs_fill_dirent(struct cifs_dirent *de, const void *info,
-		u16 level, bool is_unicode)
+		const char *end, u16 level, bool is_unicode)
 {
+	size_t len = end > (const char *)info ? end - (const char *)info : 0;
+
 	memset(de, 0, sizeof(*de));
 
 	switch (level) {
 	case SMB_FIND_FILE_POSIX_INFO:
+		if (len < sizeof(struct smb2_posix_info))
+			goto too_short;
 		cifs_fill_dirent_posix(de, info);
 		break;
 	case SMB_FIND_FILE_UNIX:
+		if (len < offsetof(FILE_UNIX_INFO, FileName))
+			goto too_short;
 		cifs_fill_dirent_unix(de, info, is_unicode);
 		break;
 	case SMB_FIND_FILE_DIRECTORY_INFO:
+		if (len < offsetof(FILE_DIRECTORY_INFO, FileName))
+			goto too_short;
 		cifs_fill_dirent_dir(de, info);
 		break;
 	case SMB_FIND_FILE_FULL_DIRECTORY_INFO:
+		if (len < offsetof(FILE_FULL_DIRECTORY_INFO, FileName))
+			goto too_short;
 		cifs_fill_dirent_full(de, info);
 		break;
 	case SMB_FIND_FILE_ID_FULL_DIR_INFO:
+		if (len < offsetof(FILE_ID_FULL_DIR_INFO, FileName))
+			goto too_short;
 		cifs_fill_dirent_search(de, info);
 		break;
 	case SMB_FIND_FILE_BOTH_DIRECTORY_INFO:
+		if (len < offsetof(FILE_BOTH_DIRECTORY_INFO, FileName))
+			goto too_short;
 		cifs_fill_dirent_both(de, info);
 		break;
 	case SMB_FIND_FILE_INFO_STANDARD:
+		if (len < offsetof(FIND_FILE_STANDARD_INFO, FileName))
+			goto too_short;
 		cifs_fill_dirent_std(de, info);
 		break;
 	default:
@@ -617,7 +633,17 @@ static int cifs_fill_dirent(struct cifs_dirent *de, const void *info,
 		return -EINVAL;
 	}
 
+	if (de->name && (end < de->name ||
+			 (size_t)(end - de->name) < de->namelen)) {
+		cifs_dbg(VFS, "search entry name extends past end of SMB\n");
+		return -EINVAL;
+	}
+
 	return 0;
+
+too_short:
+	cifs_dbg(VFS, "search entry extends past end of SMB\n");
+	return -EINVAL;
 }
 
 #define UNICODE_DOT cpu_to_le16(0x2e)
@@ -672,10 +698,14 @@ static int is_dir_changed(struct file *file)
 static int cifs_save_resume_key(const char *current_entry,
 	struct cifsFileInfo *file_info)
 {
+	struct TCP_Server_Info *server = tlink_tcon(file_info->tlink)->ses->server;
+	char *buf = file_info->srch_inf.ntwrk_buf_start;
+	const char *end = buf + server->ops->calc_smb_size(buf);
 	struct cifs_dirent de;
 	int rc;
 
-	rc = cifs_fill_dirent(&de, current_entry, file_info->srch_inf.info_level,
+	rc = cifs_fill_dirent(&de, current_entry, end,
+			      file_info->srch_inf.info_level,
 			      file_info->srch_inf.unicode);
 	if (!rc) {
 		file_info->srch_inf.presume_name = de.name;
@@ -977,7 +1007,8 @@ static int cifs_filldir(char *find_entry, struct file *file,
 	struct qstr name;
 	int rc = 0;
 
-	rc = cifs_fill_dirent(&de, find_entry, file_info->srch_inf.info_level,
+	rc = cifs_fill_dirent(&de, find_entry, end_of_smb,
+			      file_info->srch_inf.info_level,
 			      file_info->srch_inf.unicode);
 	if (rc)
 		return rc;
-- 
2.39.5

[PATCH 4/7] smb: client: fix OOB read in the SMB_FIND_FILE_UNIX name scan

[Diego Oliva]

cifs_fill_dirent_unix() finds the length of an SMB_FIND_FILE_UNIX
entry name by scanning for its terminating NUL, over up to PATH_MAX + 1
UTF-16 units in cifs_unicode_bytelen() or PATH_MAX bytes in strnlen().
Neither scan knows where the response ends. cifs_fill_dirent() only
requires the fixed part of the entry to fit, so the name can start
exactly at the end of the received data, and a name that holds no
terminator is then scanned for 8194 bytes, or 4096 bytes without
unicode, past the end of the received data. SendReceive() copies at
most 16468 bytes into the 16588-byte cifs_request allocation, so for a
response of that length up to 8074 of the bytes scanned lie past the
end of the allocation. Before the preceding patches bounded where the
entry starts, the entry itself could start up to 820 bytes past the
end of the allocation at the default CIFSMaxBufSize, and its name 928
bytes past it, so all 8194 scanned bytes would lie outside.
cifs_fill_dirent() rejects the name afterwards because it extends past
the end of the response, so nothing of what was read is used, but the
out-of-bounds read itself remains.

SMB_FIND_FILE_UNIX is only used with SMB1 servers that offer the Unix
extensions; SMB1 is not negotiated by default and requires an explicit
vers=1.0 mount.

Before the preceding patches, an entry placed past the buffer left the
scan out of bounds from its first read. They bound where the entry
starts, but nothing bounds the scan, so a name that holds no
terminator still runs off the end of the allocation. Reproduced on
the unpatched tree with KASAN enabled, for an entry that starts
inside the response, which is the case the preceding patches leave
open:

 ==================================================================
 BUG: KASAN: slab-out-of-bounds in cifs_save_resume_key.isra.0+0x75c/0x7f0
 Read of size 2 at addr ffff88807ee640cc by task ls/84

 CPU: 0 UID: 0 PID: 84 Comm: ls Not tainted 7.3.0-rc4-00457-gf14572c203d5 #69 PREEMPT(lazy)
 Call Trace:
  <TASK>
  kasan_report+0xdf/0x1a0
  cifs_save_resume_key.isra.0+0x75c/0x7f0
  cifs_readdir+0x1ed6/0x29c0
  iterate_dir+0x1c0/0x570
  __x64_sys_getdents64+0x133/0x270
  do_syscall_64+0x109/0x5d0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 Allocated by task 84 on cpu 1 at 15.751078s:
  cifs_buf_get+0x36/0x90
  smb_init+0x4f/0x110
  CIFSFindNext+0xf7/0x14c0
  cifs_readdir+0xf51/0x29c0

 The buggy address belongs to the object at ffff88807ee60000
  which belongs to the cache cifs_request of size 16588
 The buggy address is located 0 bytes to the right of
  allocated 16588-byte region [ffff88807ee60000, ffff88807ee640cc)
 ==================================================================

The two bytes read there are one UTF-16 unit of the scan, at the first
byte past the end of the object. The scan carries on from there,
reported again 2, 4 and 6 bytes to the right.

Pass the number of bytes that follow the fixed part of the entry to
cifs_fill_dirent_unix() and stop both scans there. A scan that reaches
the end of the response without finding a terminator now fails: it
returns a name that ends exactly at the end of the response and would
otherwise pass the bound, so cifs_fill_dirent() requires the position
of the terminator of an SMB_FIND_FILE_UNIX name to lie inside the
response as well. Accepting such a name truncated at the end of the
response would turn a malformed entry into a directory entry, or a
resume name, that no server sent. A name longer than the PATH_MAX cap
of the scans is still truncated there, as before, when the response
extends past the cap. The message cifs_unicode_bytelen() logs when it
finds no terminator now covers both limits.

This patch depends on "smb: client: fix OOB read of the resume name
sent in TRANS2_FIND_NEXT2", which adds the end argument of
cifs_fill_dirent() and the length it derives from it; the backport
note in that patch applies to this one as well.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/readdir.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index fdbe22b24c47..1e121c970685 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -456,16 +456,16 @@ initiate_cifs_search(const unsigned int xid, struct file *file,
 }
 
 /* return length of unicode string in bytes */
-static int cifs_unicode_bytelen(const char *str)
+static int cifs_unicode_bytelen(const char *str, size_t maxlen)
 {
 	int len;
 	const __le16 *ustr = (const __le16 *)str;
 
-	for (len = 0; len <= PATH_MAX; len++) {
+	for (len = 0; len <= PATH_MAX && (size_t)len < maxlen / 2; len++) {
 		if (ustr[len] == 0)
 			return len << 1;
 	}
-	cifs_dbg(FYI, "Unicode string longer than PATH_MAX found\n");
+	cifs_dbg(FYI, "Unicode string not terminated within %d bytes\n", len << 1);
 	return len << 1;
 }
 
@@ -532,13 +532,13 @@ static void cifs_fill_dirent_posix(struct cifs_dirent *de,
 }
 
 static void cifs_fill_dirent_unix(struct cifs_dirent *de,
-		const FILE_UNIX_INFO *info, bool is_unicode)
+		const FILE_UNIX_INFO *info, bool is_unicode, size_t maxlen)
 {
 	de->name = &info->FileName[0];
 	if (is_unicode)
-		de->namelen = cifs_unicode_bytelen(de->name);
+		de->namelen = cifs_unicode_bytelen(de->name, maxlen);
 	else
-		de->namelen = strnlen(de->name, PATH_MAX);
+		de->namelen = strnlen(de->name, min_t(size_t, maxlen, PATH_MAX));
 	de->resume_key = info->ResumeKey;
 	de->ino = le64_to_cpu(info->basic.UniqueId);
 }
@@ -589,6 +589,7 @@ static int cifs_fill_dirent(struct cifs_dirent *de, const void *info,
 		const char *end, u16 level, bool is_unicode)
 {
 	size_t len = end > (const char *)info ? end - (const char *)info : 0;
+	size_t nul_len = 0;
 
 	memset(de, 0, sizeof(*de));
 
@@ -601,7 +602,9 @@ static int cifs_fill_dirent(struct cifs_dirent *de, const void *info,
 	case SMB_FIND_FILE_UNIX:
 		if (len < offsetof(FILE_UNIX_INFO, FileName))
 			goto too_short;
-		cifs_fill_dirent_unix(de, info, is_unicode);
+		cifs_fill_dirent_unix(de, info, is_unicode,
+				      len - offsetof(FILE_UNIX_INFO, FileName));
+		nul_len = is_unicode ? 2 : 1;
 		break;
 	case SMB_FIND_FILE_DIRECTORY_INFO:
 		if (len < offsetof(FILE_DIRECTORY_INFO, FileName))
@@ -634,7 +637,7 @@ static int cifs_fill_dirent(struct cifs_dirent *de, const void *info,
 	}
 
 	if (de->name && (end < de->name ||
-			 (size_t)(end - de->name) < de->namelen)) {
+			 (size_t)(end - de->name) < de->namelen + nul_len)) {
 		cifs_dbg(VFS, "search entry name extends past end of SMB\n");
 		return -EINVAL;
 	}
-- 
2.39.5

[PATCH 5/7] smb: client: reject FIND data areas that run past the received response

[Diego Oliva]

CIFSFindFirst() and CIFSFindNext() take the directory entries of a
TRANS2_FIND_FIRST2 or TRANS2_FIND_NEXT2 response from DataOffset for
DataCount bytes. validate_t2() bounds DataOffset at 1024 and
ParameterCount + DataCount by the byte count, and nothing compares
DataOffset + DataCount with the number of bytes SendReceive() copied
into the buffer.

smb_init() places the request and the response in the same
cifs_request allocation and cifs_buf_get() clears only the start of
it, so a short response whose data area extends past its end has
srch_entries_start point at bytes of the request that was just sent,
or at whatever the allocation held before, and those bytes are parsed
as directory entries. cifs_readdir() and find_cifs_entry() walk the
entries with nxt_dir_entry(), which checks every entry but the first
of a response against the end of the SMB; cifs_query_path_info() and
cifs_backup_query_path_info() read the first entry with no check at
all. Those reads stay inside the allocation, but what they return is
not part of the response. The last entry, which LastNameOffset locates
relative to the data area, is bounded against the received response
by an earlier patch of this series, as is every entry that
cifs_fill_dirent() parses; the walk itself and the two query functions
are not.

SMB1 is not negotiated by default; reaching this code requires an
explicit vers=1.0 mount.

Reject a response whose data area extends past the received length,
with the -EINVAL that validate_t2() returns for the other malformed
transaction responses. cifs_query_path_info() and
cifs_backup_query_path_info() still read a whole entry from a data
area that may be smaller than one; that is not changed here.

A conforming server is not affected. The data block of a transaction
response is part of the SMB, so DataOffset + DataCount cannot exceed
the smbCalcSize() bytes that SendReceive() copies; coalesce_t2()
appends each secondary response to the data area and adds its length
to both DataCount and the byte count, which keeps that relation.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 7eb83dd737fc..107ca76e53d1 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -4412,6 +4412,7 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 	TRANSACTION2_FFIRST_RSP *pSMBr = NULL;
 	T2_FFIRST_RSP_PARMS *parms;
 	struct nls_table *nls_codepage;
+	unsigned int data_off, data_count;
 	unsigned int in_len, lnoff;
 	__u16 params, byte_count;
 	int bytes_returned = 0;
@@ -4530,11 +4531,19 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 		return rc;
 	}
 
+	data_off = le16_to_cpu(pSMBr->t2.DataOffset);
+	data_count = le16_to_cpu(pSMBr->t2.DataCount);
+	if (data_off + data_count > (unsigned int)bytes_returned) {
+		cifs_dbg(VFS, "%s: data area offset %u count %u past response length %d\n",
+			 __func__, data_off, data_count, bytes_returned);
+		cifs_buf_release(pSMB);
+		return -EINVAL;
+	}
+
 	psrch_inf->unicode = !!(pSMBr->hdr.Flags2 & SMBFLG2_UNICODE);
 	psrch_inf->ntwrk_buf_start = (char *)pSMBr;
 	psrch_inf->smallBuf = false;
-	psrch_inf->srch_entries_start = (char *)&pSMBr->hdr.Protocol +
-		le16_to_cpu(pSMBr->t2.DataOffset);
+	psrch_inf->srch_entries_start = (char *)&pSMBr->hdr.Protocol + data_off;
 
 	parms = (T2_FFIRST_RSP_PARMS *)((char *)&pSMBr->hdr.Protocol +
 					le16_to_cpu(pSMBr->t2.ParameterOffset));
@@ -4568,6 +4577,7 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 	TRANSACTION2_FNEXT_REQ *pSMB = NULL;
 	TRANSACTION2_FNEXT_RSP *pSMBr = NULL;
 	T2_FNEXT_RSP_PARMS *parms;
+	unsigned int data_off, data_count;
 	unsigned int name_len, in_len;
 	unsigned int lnoff;
 	__u16 params, byte_count;
@@ -4650,13 +4660,22 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 		cifs_buf_release(pSMB);
 		return rc;
 	}
+
+	data_off = le16_to_cpu(pSMBr->t2.DataOffset);
+	data_count = le16_to_cpu(pSMBr->t2.DataCount);
+	if (data_off + data_count > (unsigned int)bytes_returned) {
+		cifs_dbg(VFS, "%s: data area offset %u count %u past response length %d\n",
+			 __func__, data_off, data_count, bytes_returned);
+		cifs_buf_release(pSMB);
+		return -EINVAL;
+	}
+
 	/* BB fixme add lock for file (srch_info) struct here */
 	psrch_inf->unicode = !!(pSMBr->hdr.Flags2 & SMBFLG2_UNICODE);
 	response_data = (char *)&pSMBr->hdr.Protocol +
 		le16_to_cpu(pSMBr->t2.ParameterOffset);
 	parms = (T2_FNEXT_RSP_PARMS *)response_data;
-	response_data = (char *)&pSMBr->hdr.Protocol +
-		le16_to_cpu(pSMBr->t2.DataOffset);
+	response_data = (char *)&pSMBr->hdr.Protocol + data_off;
 
 	if (psrch_inf->smallBuf)
 		cifs_small_buf_release(psrch_inf->ntwrk_buf_start);
-- 
2.39.5

[PATCH 6/7] smb: client: reject short or out-of-range FIND response parameters

[Diego Oliva]

CIFSFindFirst() and CIFSFindNext() read the SearchHandle, SearchCount,
EndofSearch and LastNameOffset of a TRANS2_FIND_FIRST2 or
TRANS2_FIND_NEXT2 response at ParameterOffset. validate_t2() bounds
the offset at 1024 and ParameterCount below 512, and nothing compares
them with the number of bytes SendReceive() copied into the buffer or
with the size of the response parameters. smb_init() places the
request and the response in the same cifs_request allocation and
cifs_buf_get() clears only the start of it, so a short response with a
ParameterOffset past its end has the search handle, the entry count,
the end-of-search flag and the last-entry offset read out of the
request that was just sent, or out of whatever the allocation held
before, and a ParameterCount smaller than the parameters has them read
past the end of the parameter block. The reads stay inside the
allocation, but what they return is not part of the response, and
CIFSFindFirst() then hands back a search handle the server never sent.
That handle is sent back to the server in the next FIND_NEXT2 or
FIND_CLOSE2, so two bytes of the buffer leak at an offset the server
picks.

SMB1 is not negotiated by default; reaching this code requires an
explicit vers=1.0 mount.

Reject a response whose parameter area is smaller than the response
parameters or extends past the received length, returning smb_EIO2()
with a new smb_eio_traces value for each of the two responses, like
the other response checks this file gained since v6.19. Backports to
trees before v6.19 need smb_EIO2() replaced with -EINVAL and the
trace.h hunk dropped.

A conforming server is not affected. The parameter block of a
transaction response is part of the SMB, so ParameterOffset +
ParameterCount cannot exceed the smbCalcSize() bytes that
SendReceive() copies, and the response parameters are 10 bytes for
FIND_FIRST2 and 8 bytes for FIND_NEXT2, which is what
T2_FFIRST_RSP_PARMS and T2_FNEXT_RSP_PARMS hold.

This patch depends on "smb: client: reject FIND data areas that run
past the received response", whose declarations it extends.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org> # 6.19.x
Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/cifssmb.c | 32 ++++++++++++++++++++++++++------
 fs/smb/client/trace.h   |  2 ++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 107ca76e53d1..962877450eca 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -4412,7 +4412,7 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 	TRANSACTION2_FFIRST_RSP *pSMBr = NULL;
 	T2_FFIRST_RSP_PARMS *parms;
 	struct nls_table *nls_codepage;
-	unsigned int data_off, data_count;
+	unsigned int parm_off, parm_count, data_off, data_count;
 	unsigned int in_len, lnoff;
 	__u16 params, byte_count;
 	int bytes_returned = 0;
@@ -4531,6 +4531,17 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 		return rc;
 	}
 
+	parm_off = le16_to_cpu(pSMBr->t2.ParameterOffset);
+	parm_count = le16_to_cpu(pSMBr->t2.ParameterCount);
+	if (parm_count < sizeof(*parms) ||
+	    parm_off + parm_count > (unsigned int)bytes_returned) {
+		cifs_dbg(FYI, "%s: bad parameter area offset %u count %u for response of %d\n",
+			 __func__, parm_off, parm_count, bytes_returned);
+		rc = smb_EIO2(smb_eio_trace_ffirst_param_area, parm_off, parm_count);
+		cifs_buf_release(pSMB);
+		return rc;
+	}
+
 	data_off = le16_to_cpu(pSMBr->t2.DataOffset);
 	data_count = le16_to_cpu(pSMBr->t2.DataCount);
 	if (data_off + data_count > (unsigned int)bytes_returned) {
@@ -4545,8 +4556,7 @@ CIFSFindFirst(const unsigned int xid, struct cifs_tcon *tcon,
 	psrch_inf->smallBuf = false;
 	psrch_inf->srch_entries_start = (char *)&pSMBr->hdr.Protocol + data_off;
 
-	parms = (T2_FFIRST_RSP_PARMS *)((char *)&pSMBr->hdr.Protocol +
-					le16_to_cpu(pSMBr->t2.ParameterOffset));
+	parms = (T2_FFIRST_RSP_PARMS *)((char *)&pSMBr->hdr.Protocol + parm_off);
 	psrch_inf->endOfSearch = !!parms->EndofSearch;
 
 	psrch_inf->entries_in_buffer = le16_to_cpu(parms->SearchCount);
@@ -4577,7 +4587,7 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 	TRANSACTION2_FNEXT_REQ *pSMB = NULL;
 	TRANSACTION2_FNEXT_RSP *pSMBr = NULL;
 	T2_FNEXT_RSP_PARMS *parms;
-	unsigned int data_off, data_count;
+	unsigned int parm_off, parm_count, data_off, data_count;
 	unsigned int name_len, in_len;
 	unsigned int lnoff;
 	__u16 params, byte_count;
@@ -4661,6 +4671,17 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 		return rc;
 	}
 
+	parm_off = le16_to_cpu(pSMBr->t2.ParameterOffset);
+	parm_count = le16_to_cpu(pSMBr->t2.ParameterCount);
+	if (parm_count < sizeof(*parms) ||
+	    parm_off + parm_count > (unsigned int)bytes_returned) {
+		cifs_dbg(FYI, "%s: bad parameter area offset %u count %u for response of %d\n",
+			 __func__, parm_off, parm_count, bytes_returned);
+		rc = smb_EIO2(smb_eio_trace_fnext_param_area, parm_off, parm_count);
+		cifs_buf_release(pSMB);
+		return rc;
+	}
+
 	data_off = le16_to_cpu(pSMBr->t2.DataOffset);
 	data_count = le16_to_cpu(pSMBr->t2.DataCount);
 	if (data_off + data_count > (unsigned int)bytes_returned) {
@@ -4672,8 +4693,7 @@ int CIFSFindNext(const unsigned int xid, struct cifs_tcon *tcon,
 
 	/* BB fixme add lock for file (srch_info) struct here */
 	psrch_inf->unicode = !!(pSMBr->hdr.Flags2 & SMBFLG2_UNICODE);
-	response_data = (char *)&pSMBr->hdr.Protocol +
-		le16_to_cpu(pSMBr->t2.ParameterOffset);
+	response_data = (char *)&pSMBr->hdr.Protocol + parm_off;
 	parms = (T2_FNEXT_RSP_PARMS *)response_data;
 	response_data = (char *)&pSMBr->hdr.Protocol + data_off;
 
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index bb8d0197cb54..2708bedb49ab 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -30,6 +30,8 @@
 	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_ffirst_param_area,		"ffirst_param_area") \
+	EM(smb_eio_trace_fnext_param_area,		"fnext_param_area") \
 	EM(smb_eio_trace_forced_shutdown,		"forced_shutdown") \
 	EM(smb_eio_trace_getacl_bcc_too_small,		"getacl_bcc_too_small") \
 	EM(smb_eio_trace_getcifsacl_param_count,	"getcifsacl_param_count") \
-- 
2.39.5

[PATCH 7/7] smb: client: drop the redundant name bounds in cifs_filldir()

[Diego Oliva]

cifs_fill_dirent() rejects an entry whose name extends past the end of
the response before it returns, for both of its callers. The two
checks cifs_filldir() applies after the parse can no longer fail: the
one that commit f8cf09a53a0d ("smb: client: bound dirent name against
end of SMB response in cifs_filldir") added tests the same name
against the same end, and the older one, which compares the name
length with the length of the response, is implied by it because the
name starts inside the response. Remove both.

This patch depends on "smb: client: fix OOB read of the resume name
sent in TRANS2_FIND_NEXT2", which added the check to
cifs_fill_dirent().

No functional change intended.

Assisted-by: Bynario AI
Signed-off-by: Diego Oliva <diego@bynar.io>
---
 fs/smb/client/readdir.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index 1e121c970685..569efdfb7360 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -1016,17 +1016,6 @@ static int cifs_filldir(char *find_entry, struct file *file,
 	if (rc)
 		return rc;
 
-	if (de.namelen > max_len) {
-		cifs_dbg(VFS, "bad search response length %zd past smb end\n",
-			 de.namelen);
-		return -EINVAL;
-	}
-
-	if (de.name + de.namelen > end_of_smb) {
-		cifs_dbg(VFS, "search entry name extends past end of SMB\n");
-		return -EINVAL;
-	}
-
 	/* skip . and .. since we added them first */
 	if (cifs_entry_is_dot(&de, file_info->srch_inf.unicode))
 		return 0;
-- 
2.39.5

Re: [PATCH 0/7] smb: client: fix OOB and use-after-free infoleaks in SMB1 FIND responses

[Paulo Alcantara]

Diego Oliva <diego@bynar.io> writes:

> The SMB1 client parses TRANS2_FIND_FIRST2 and TRANS2_FIND_NEXT2
> responses without checking them against the number of bytes it
> received. A malicious or compromised server can make the client read
> past the end of a response, and past the end of the cifs_request
> buffer holding it, and send what it read back to the server. Both the
> out-of-bounds reads and the use-after-free read are information leaks:
> the bytes leave the machine in the client's next FindNext request,
> from a position the server picks:
>
>  - LastNameOffset, which the client uses to locate the last entry of
>    a response, is only checked against CIFSMaxBufSize, never against
>    the response that was actually received, so psrch_inf->last_entry
>    can be placed past the end of the allocation.
>  - cifs_save_resume_key() parses the entry there without a bound. The
>    name length comes from the entry itself, and for SMB_FIND_FILE_UNIX
>    from a NUL scan of up to PATH_MAX + 1 UTF-16 units. The name is
>    recorded as the resume name of the search, and the next
>    CIFSFindNext() rejects only a recorded length of PATH_MAX or more
>    before copying the name into its request and sending it, so up to
>    4 KiB of memory from beyond the response leaves the machine,
>    together with the resume key read from the same entry.
>  - When the last entry is rejected, and when a search rewind frees
>    the buffer without its FindFirst recording a new name, the resume
>    name keeps pointing into a buffer that has been released, and the
>    next CIFSFindNext() copies it from there.
>  - The response parameters (search handle, entry count, end-of-search
>    flag, LastNameOffset) and the data area are read at offsets that
>    validate_t2() caps at 1024. It bounds the parameter and data
>    counts by the byte count of the response, but nothing ties either
>    area to the bytes that were received, so one can begin or end past
>    them; those reads stay inside the allocation but can return stale
>    bytes.
>
> A FindNext response that reports no entries without ending the search
> is enough to make the client send the resume name, and each further
> response can do the same. Reaching the code needs a malicious or
> compromised server, an explicit vers=1.0 mount of it
> (CONFIG_CIFS_ALLOW_INSECURE_LEGACY, default y) and a directory listing
> on that mount; SMB1 is not negotiated by default. The same code is in
> every maintained stable tree.
>
> Commit f8cf09a53a0d ("smb: client: bound dirent name against end of
> SMB response in cifs_filldir"), in v7.2, bounded the name of the
> entries cifs_readdir() emits, after the parse; the resume-key path was
> left unbounded and the parse itself still ran before any bound.
>
> The fix is one change conceptually, but as a single patch it is well
> over the 100 lines with context that stable-kernel-rules.rst allows,
> so stable could not take it. This series splits it into patches that
> each fix one thing, build and pass checkpatch on their own, and stay
> under that limit.
>
> What each patch fixes, and what to backport:
>
>   1/7 fix use-after-free infoleak via the readdir resume name
>       The use-after-free. It comes first so that the patches after it
>       cannot widen the window it closes. Also resets resume_key with
>       the name and skips the zero-length copy in CIFSFindNext().
>       Tagged Cc: stable.
>   2/7 fix OOB last_entry pointer from unbounded LastNameOffset
>       The out-of-bounds pointer, bounded against the response that
>       was received rather than against the declared data area. It
>       applies on top of 1/7, whose resets sit in its context and
>       which makes the paths this patch sends more responses down
>       safe. Also records the search handle before the last entry is
>       examined, because the new no-entries case would otherwise lose
>       the handle. Cc: stable.
>   3/7 fix OOB read of the resume name sent in TRANS2_FIND_NEXT2
>       Bounds the entry parse in cifs_fill_dirent() for both callers,
>       before anything is read. This is what stops a name from being
>       recorded and sent from past the end of the response.
>       Tagged Cc: stable.
>   4/7 fix OOB read in the SMB_FIND_FILE_UNIX name scan
>       Caps the NUL scan at the response end; without it the scan
>       still runs several KiB past the allocation. Depends on 3/7 for
>       the end of the response. A scan that reaches the end of the
>       response without finding a terminator is rejected; a name
>       longer than the PATH_MAX cap of the scans is still truncated
>       there, as before. Cc: stable.
>   5/7 reject FIND data areas that run past the received response
>       Hardening rather than memory safety: it stops entries being
>       parsed from stale bytes inside the allocation, and bounds the
>       data area that the nxt_dir_entry() walk starts from and that
>       the two query helpers read their entry from. It rejects with
>       -EINVAL, as validate_t2() does, logging with cifs_dbg(), and
>       adds no tracepoint, so it applies to trees before 6.19.
>       Cc: stable.
>   6/7 reject short or out-of-range FIND response parameters
>       The parameters are read inside the allocation, but the stale
>       search handle among them is sent back to the server. Depends on
>       5/7, whose declarations it extends. It returns smb_EIO2() with
>       two new smb_eio_traces values; backports before v6.19 need
>       -EINVAL instead. Cc: stable.
>   7/7 drop the redundant name bounds in cifs_filldir()
>       Cleanup of the two post-parse checks that 3/7 made redundant.
>       Depends on 3/7. Not for stable.
>
> No patch needs one that follows it, so the series can be cut after any
> patch. Patches 1/7 to 4/7 are every out-of-bounds and use-after-free
> fix; 5/7 stands on its own; 6/7 can follow where smb_EIO2() exists.
> Trees before v7.2 need the adjustments noted in the patches that need
> them.
>
> cifs_fill_dirent() is shared with the SMB2 readdir path; 3/7 and 4/7
> add checks there that a valid SMB2 response cannot fail, since
> smb2_parse_query_directory() already walks its entries against the
> end of the response.
>
> Left out on purpose: an entry-size check for cifs_query_path_info()
> and cifs_backup_query_path_info(), which still read one whole entry
> from a data area that 5/7 bounds only as a whole (those reads stay
> inside the allocation); passing the end to posix_info_parse() in
> cifs_fill_dirent_posix(), which only SMB3.1.1 POSIX mounts reach,
> with entries that num_entries() has already validated; and a lower
> bound on ParameterOffset and DataOffset, since reads at a low offset
> stay inside the received response.
>
> Each patch builds with W=1 without warnings and passes checkpatch.
> The out-of-bounds reads and the use-after-free read were reproduced
> against a test server with KASAN enabled, by listing a directory on
> an SMB1 mount; the splats are in 1/7, 2/7, 3/7 and 4/7, all taken on
> the unpatched tree. The one in 4/7 shows the case the preceding
> patches leave open, where the entry starts inside the response and
> the scan still leaves the allocation.
>
> The series is based on cifs-next at commit f14572c203d5 ("Merge tag
> 'cifs-fixes-7.3-rc5' of https://git.manguebit.org/linux"). The files it
> touches are identical there and in v7.3-rc4.
> ...

Applied.