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

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(?). 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