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

[zihan xi]

On Sat, Sep 12, 2026 at 1:05 AM Frank Sorenson <sorenson@xxxxxxxxxx> wrote:
>
> 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
>

Hi Frank,

Thank you for the review.  I'm sorry to hear about Steve.  I'll send
future versions to Paulo Alcantara, with Cc to Namjae Jeon.

I also appreciate you carrying the overlapping bounds-checking series.

I'll address both comments in v4 and send it as a new thread:

1) smb2_parse_lease_buf() and smb3_parse_lease_buf() will copy the
   lease context from DataOffset, as the QFid handler does for
   DiskFileId.  Agreed this is not a security fix; it just keeps
   LeaseState correct when DataOffset is non-canonical.

2) DataLength will be required to equal sizeof(struct lease_context)
   or sizeof(struct lease_context_v2) for the corresponding parser,
   rather than a minimum.  A mismatch skips lease parsing and does
   not fail the open.

Thanks,
Zihan