Re: kernel svc_rpc_gss_update_seq missing 2017 Coverity fix + TOCTOU with check_replay

Author is even better, thanks! -Stan

On Sun, Apr 12, 2026 at 2:23 PM Rick Macklem <[email protected]> wrote:

> On Sun, Apr 12, 2026 at 12:08 AM Stanislav Fort
> <[email protected]> wrote:
> >
> > Hi Rick,
> >
> > Thanks for picking up the first fix. Could you please include a Reported
> by: tag for me in the commit? (Stanislav Fort of Aisle Research <
> [email protected]>)
> I was going to list you as author, but if you prefer
> reported by, I can do that.
>
> >
> > For the TOCTOU issue, I went ahead and wrote a minimal patch = just a
> bounds check in update_seq rather than widening the lock scope, so it
> shouldn't affect the validate/nextverf path at all:
> >
> > --- a/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
> > +++ b/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
> > @@ -1387,6 +1387,8 @@
> >   } else {
> >   offset = client->cl_seqlast - seq;
> > + if (offset >= SVC_RPC_GSS_SEQWINDOW)
> > + goto out;
> >   word = offset / 32;
> >   bit = offset % 32;
> >   client->cl_seqmask[word] |= (1 << bit);
> > @@ -1392,6 +1394,8 @@
> >   }
> > +out:
> >   sx_xunlock(&client->cl_lock);
> >  }
> >
> > The idea: if another thread advanced cl_seqlast past the window between
> check_replay and update_seq, the sequence number is already too old to
> track, so we just skip the write. This avoids the OOB on cl_seqmask without
> changing the locking around validate/nextverf.
> >
> > Happy to also put this up on Phabricator if that's easier for review.
> >
> > Best wishes,
> > Stanislav Fort
> > Aisle Research
> >
> > On Sun, Apr 12, 2026 at 2:24 AM Rick Macklem <[email protected]>
> wrote:
> >>
> >> On Sat, Apr 11, 2026 at 4:59 PM Rick Macklem <[email protected]>
> wrote:
> >> >
> >> > On Sat, Apr 11, 2026 at 3:42 PM Stanislav Fort <
> [email protected]> wrote:
> >> > >
> >> > > Hi there,
> >> > >
> >> > > I think there might be two bugs in
> sys/rpc/rpcsec_gss/svc_rpcsec_gss.c in svc_rpc_gss_update_seq():
> >> > >
> >> > > 1. Missing port of lib/librpcsec_gss fix from 11bc2c1ca77e
> >> > >
> >> > > The userspace lib/librpcsec_gss/svc_rpcsec_gss.c was fixed in 2017
> (commit 11bc2c1ca77e, Coverity CID 1198859) to change "while (offset > 32)"
> to "while (offset >= 32)" and guard the subsequent bit-shift loop with "if
> (offset > 0)". The kernel copy still has the old pattern:
> >> > >
> >> > >     while (offset > 32) {          /* should be >= 32 */
> >> > >         /* ... word shift ... */
> >> > >         offset -= 32;
> >> > >     }
> >> > >                                     /* missing: if (offset > 0) { */
> >> > >     carry = 0;
> >> > >     for (i = 0; i < SVC_RPC_GSS_SEQWINDOW / 32; i++) {
> >> > >         newcarry = client->cl_seqmask[i] >> (32 - offset);
> >> > >         client->cl_seqmask[i] =
> >> > >             (client->cl_seqmask[i] << offset) | carry;  /* UB when
> offset==32 */
> >> > >         carry = newcarry;
> >> > >     }
> >> > >
> >> > > When offset is exactly 32, the while loop doesn't execute and the
> shift loop does << 32 on a uint32_t, which is undefined behavior(?).
> >> Btw, I tried "s << 32", where s is uint32_t on amd64 with clang and it
> >> does not change value of s.
> >>
> >> Therefore, the loop ends up setting extra bits (or of the adjacent
> >> seqmask with the next seqmask) and, if I
> >> recall RPCSEC_GSS correctly, that will result in RPCs
> >> which should be valid being discarded as retries.
> >> (This window of seq#s is meant to subvert replay attempts.)
> >>
> >> rick
> >>
> >> > The fix is to match what lib/librpcsec_gss already does: use >= 32
> and wrap the shift loop in if (offset > 0).
> >> >
> >> > This makes sense. I will commit the patch.
> >> > I think the most this bug can do is result in a bogus
> >> > retry of an RPC. Since kerberized NFS isn't used a
> >> > lot and almost always over TCP (where the sequence#
> >> > increments by 1 for each RPC and they generally stay
> >> > in order), I don't think this bug introduced a serious issue.
> >> > (The seq# in RPCSEC_GSS is basically for UDP.)
> >> >
> >> > >
> >> > > 2. TOCTOU between check_replay and update_seq
> >> > >
> >> > > In svc_rpc_gss(), svc_rpc_gss_check_replay() and
> svc_rpc_gss_update_seq() each independently acquire and release cl_lock.
> Between the two calls, svc_rpc_gss_validate() and svc_rpc_gss_nextverf()
> run without the lock held. Since svc_rpc_gss_find_client() matches clients
> by opaque handle and doesn't bind them to a specific transport, two
> connections presenting the same GSS handle can drive concurrent threads
> through svc_rpc_gss() on the same client struct.
> >> > >
> >> > > If thread A passes check_replay for a low sequence number, then
> thread B advances cl_seqlast significantly before thread A enters
> update_seq, the else branch in update_seq computes:
> >> > >
> >> > >     offset = client->cl_seqlast - seq;
> >> > >     word = offset / 32;
> >> > >     bit = offset % 32;
> >> > >     client->cl_seqmask[word] |= (1 << bit);
> >> > >
> >> > > Word can exceed SVC_RPC_GSS_SEQWINDOW/32 - 1 (i.e., >= 4), causing
> an OOB write to cl_seqmask. This requires authenticated access (GSS MIC
> verification still gates both paths) and winning a race, so practical
> impact is limited.
> >> > >
> >> > > A simple fix would be to hold cl_lock across both check and update,
> or to re-validate offset < SVC_RPC_GSS_SEQWINDOW inside update_seq before
> writing.
> >> >
> >> > I'll look at fixing this one later, but as you note, it is also
> unlikely
> >> > to cause serious trouble, imho.
> >> >
> >> > rick
> >> >
> >> > >
> >> > > Let me know what you think.
> >> > >
> >> > > Best wishes,
> >> > > Stanislav Fort
> >> > > Aisle Research
>