Page MenuHomeFreeBSD

crsetgroups(): Improve and factor out groups normalization
ClosedPublic

Authored by olce on Oct 4 2024, 8:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 26, 4:18 AM
Unknown Object (File)
Wed, Dec 25, 10:01 AM
Unknown Object (File)
Tue, Dec 24, 12:28 AM
Unknown Object (File)
Mon, Dec 23, 9:14 AM
Unknown Object (File)
Mon, Dec 9, 1:07 AM
Unknown Object (File)
Nov 21 2024, 10:56 PM
Unknown Object (File)
Nov 18 2024, 4:00 PM
Unknown Object (File)
Nov 13 2024, 2:59 PM
Subscribers

Details

Summary

The groups array has been sorted (not including the first element, which
is always the effective GID) to enable performing a binary search for
determining if some group is part of the supplementary groups set.

Factor out this sorting operation into an internal normalization
function (groups_normalize()), adding to it the removal of duplicates
after the sort.

Separating groups normalization code allows to perform it in advance,
and in particular before calling MAC hooks which need the groups array
to be sorted to perform better. This also enables sorting input arrays
ahead of acquiring the process lock (which is not necessary for this
operation).

kern_setgroups() has been changed accordingly, so MAC modules
implementing the mac_cred_check_setgroups() hook now can assume
a normalized groups array (and also that it has at least one element, as
if kern_setgroups() is passed no groups, the hook is called with an
array of one element being the current effective GID, as this is
effectively the effect of such a call to kern_setgroups()). Further
commits introducing the setcred() system call and associated MAC hooks
will also guarantee a normalized groups array to MAC modules
implementing these hooks.

Rename crsetgroups_locked() into crsetgroups_internal(), as it is no
more "locked" than crsetgroups() itself. However, it can be called
under any lock (as needed), whereas the second may sleep to allocate
memory. Update their herald comments to make that explicit.

In passing, using qsort() instead of the old open-coded insertion sort
(in crsetgroups_locked()) fixes the performance concern about the latter
when using a large number of groups. Also, our qsort() falls back to
insertion sort for small arrays and in case the array is likely to be
mostly sorted, so this shouldn't cause concerns for the small number of
groups common case.

While here, add assertions in inner modification routines to check that
the passed credentials object has a reference count of exactly 1 (in
particular, it must not be shared). Remove a redundant one from some
outer routine.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60263
Build 57147: arc lint + arc unit

Event Timeline

olce requested review of this revision.Oct 4 2024, 8:07 AM

LGTM with some small comments.

sys/kern/kern_prot.c
854

👍

869

I've never seen 'CAUTION' in our sources, but XXX is common to mean the same thing.

2382–2389

I am being picky, but it helps to use a different verb, where we are already using "set" (noun) in the same sentence. You might think of something better than "applied to".

2384
2387
2392

More logical ordering, but this is totally opinionated.

2403
2459

To me, the comment is not so useful.

olce marked 8 inline comments as done.

Cater to comments.

sys/kern/kern_prot.c
869

Ah... To me, XXX (in FreeBSD and elsewhere) means something quite different, along the lines of: "some potential problem to be fixed here, or some code to be improved", so a weaker FIXME of sorts. Here, I really just want to draw the attention of readers to the unusual feature of modifying the input arguments for a function that is just supposed to be a procedure (and thus should not produce outputs), without suggesting there is anything to fix.

Will add an "also" between "possibly" and "changing", so as not to confuse the reader about whether groups is also modified (to me, it is obvious it is, but don't want to leave to a casual reader any reason to think otherwise).

2382–2389

I easily can be *that* picky myself (and even more) so I completely understand and agree with you. Was just something I missed (didn't re-read, or whatever). Thanks.

2392

I think it's more logical to have ngrp first, as you have to read it in any case to know which part pointed to by groups is valid. Unless there would be some other reason to have it the other way around (such as some custom practice or a style(9) requirement) which I don't see.

2403

On one hand, the original version is more to the point (less verbose). On the other hand, your suggestion makes it clear that the arithmetic used is not the regular one but that of pointers. I usually prefer the original version as it also forces people to understand what the augend's type is, but this isn't a strong preference.

2459

I've removed it.

sys/kern/kern_prot.c
869

IMO although something with an XXX comment might never be changed it does have a connotation of being undesired or unwanted, and doesn't apply in this case here.

We do use NOTE: somewhat regularly in comments, CAUTION: very occasionally (e.g. there is one in libexec/rtld-elf/rtld.h).

sys/kern/kern_prot.c
869

Right, I agree that XXX is not quite equivalent. It is mostly that 'CAUTION' is quite alien to our codebase.

This revision is now accepted and ready to land.Oct 31 2024, 1:27 PM