kern: Fix credentials leaks on RACCT but no RCTL

[olce]

Summary

Affected system calls: setuid(), setreuid(), setresuid(), jail_attach(),
setloginclass().

In these system calls, the crhold() calls that, on RACCT, make the
just-installed process credentials survive a concurrent change of the
same credentials just after PROC_UNLOCK() were not matched by
a corresponding crfree() when RCTL is off. In fact, in that latter
case, they are simply not necessary, so wrap them with '#ifdef RCTL'
stances. 'kern_rctl.c' causes a compile error if RACCT is not defined
but RCTL is, so ease reading by not nesting '#ifdef's.

[kib]

sys/kern/kern_jail.c

3052

I would restructure it differently. Instead of relying on p_ucred stability under the proc_lock, which is only needed to keep newcred valid, I would do

  newcred = crget();
#ifdef RCTL
  crhold(newcred);
#endif

earlier. Otherwise it depends on the impl details of proc_set_cred(), in particular, that p_ucred is newcred after the call.

[olce]

Sorry, I'm running out of time and this (and also next revision, D53457) needs to land in 15.0, so please urgently tell me if you think the current version is fine functionality-wise. Unless it is not, I will commit it as is tomorrow, and can then amend it with more time later, as I agree with your comments and have prepared an update which unfortunately I haven't had much time to test. I also fear that modifying this relatively delicate code close to committing it could introduce some unseen problem, that's why I'd really prefer to avoid that.

sys/kern/kern_jail.c

3052

Indeed, even if in practice this doesn't make a difference in the current state, relying only on credential references to ensure liveness makes it easier to reason about correctness of this code, and makes potential future changes in this area less error-prone.

This is not the case for this occurrence, but in those of kern_prot.c and in new code in D53457, taking another reference to the new credentials that early (i.e., just after crget()) leads to code "complexification" (e.g., double crfree() in the error paths, which looks quite astonishing).

Since we are relying on credentials references, we still can count on proc_set_cred() taking over the reference to the passed credentials, which in practice means that the additional crhold() can be called only just before calling proc_set_cred(). This makes for a minimal change, so I'd rather implement that variant around your suggestion, even if that means calling crhold() under the process lock (that call is uncontended).

Note that the calls to racct_proc_ucred_changed() are currently also relying on that. In order to suppress this assumption, I had to slightly tweak the order of some operations.