Re: [PATCH v3 1/2] smb: client: fix create context out-of-bounds reads

[Frank Sorenson]

On Fri, Sep 04, 2026 at 02:05:17PM +0000, Zihan Xi wrote:
> smb2_parse_contexts() validates the complete create-context area but does
> not bound each context record by its Next field before dispatching to a
> handler.  A malformed chain can therefore expose bytes past one context to
> the handler.  The QFid handler also used a full response-structure cast even
> though it only consumes DiskFileId.

Hello,

I have some thoughts/comments on your series.

First, a possible reason for not getting a response on your v2, and
delayed response on this v3: Steve French passed away in August, so
mail addressed to him isn't reaching a maintaner, and things may
have gotten missed during transitions.  You'll want to send future
versions to Paulo Alcantara <pc@xxxxxxxxxxxxx>, with Cc to
Namjae Jeon <linkinjeon@xxxxxxxxxx>


I've been carrying an overlapping patch (an earlier posting:
https://lore.kernel.org/r/20260826153147.4112943-12-sorenson@xxxxxxxxxx).
in a bounds-checking series of my own.  Like yours, my patch has
been addressing the memory safety problem in the three handlers
which read at fixed offsets, rather than the generic checks.

But I think yours is probably a better fix, and you've got the
PoC, so if we can perfect yours and get it in, I'll be dropping
mine in favor of this series.


A few points (take with a grain of salt):

1) The lease parser still reads at a fixed offset rather than from
   DataOffset:

>  		case 4:
>  			if (!strncmp(name, SMB2_CREATE_REQUEST_LEASE, 4)) {
> -				*oplock = server->ops->parse_lease_buf(cc, epoch,
> +				if (cc_len >= smb2_create_lease_min_cc_len(server))
> +					*oplock = server->ops->parse_lease_buf(cc, epoch,
>  								       lease_key);

cc_len bounds the record, so the read stays in bounds.  But this is
just like the QFid bug you just fixed nearby:  smb2_parse_lease_buf()
and smb3_parse_lease_buf() reach the fields at the canonical offset
of the create_lease layout, not at DataOffset, so a valid but non-
canonical DataOffset could get in-bounds garbage rather than an
OOB.  That's not a security fix, but since LeaseState drives client
caching decisions, it's probably worth closing.  Reading lcontext
from DataOffset, as with DiskFileId would make the two handlers
consistent.

(My version also had this, so it's more an observation than anything
else)


2) You may want to consider matching DataLength exactly, rather than
   taking a minimum.

ksmbd's parse_lease_state() requires:

	sizeof(struct lease_context_v2) == le32_to_cpu(cc->DataLength)

and validates DataOffset + DataLength against the create_lease_v2
size, rather than accepting anything at least long enough.  It was
suggested to me that having the client & server halves agree on
strictness would be good.

(I did confirm your minimums cover all the fields each of the parsers
actually touch, so this is about strictness, not a hole)


Frank
--
Frank Sorenson
sorenson@xxxxxxxxxx
Principal Software Maintenance Engineer, filesystems
Red Hat