Given this, I have no idea why this discussion is even being continued
further. These features have more than justified themselves. Shall we
speculate what may have happened had these self-protections not been
present? Of course not.
Also, with respect to a switch for turning this off, nobody is going
to want it. If they haven't yet requested it (this feature has been in
mainline for years), I seriously doubt anyone will care.
On Fri, Aug 18, 2023 at 2:46 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Aug 18, 2023 at 10:55:42AM -0700, Andrew Morton wrote:
> > On Thu, 17 Aug 2023 21:17:41 -0700 Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> >
> > > From: Elena Reshetova <elena.reshetova@xxxxxxxxx>
> > >
> > > atomic_t variables are currently used to implement reference counters
> > > with the following properties:
> > > - counter is initialized to 1 using atomic_set()
> > > - a resource is freed upon counter reaching zero
> > > - once counter reaches zero, its further
> > > increments aren't allowed
> > > - counter schema uses basic atomic operations
> > > (set, inc, inc_not_zero, dec_and_test, etc.)
> > >
> > > Such atomic variables should be converted to a newly provided
> > > refcount_t type and API that prevents accidental counter overflows and
> > > underflows. This is important since overflows and underflows can lead
> > > to use-after-free situation and be exploitable.
> >
> > ie, if we have bugs which we have no reason to believe presently exist,
> > let's bloat and slow down the kernel just in case we add some in the
> > future? Or something like that. dangnabbit, that refcount_t.
> >
> > x86_64 defconfig:
> >
> > before:
> > text data bss dec hex filename
> > 3869 552 8 4429 114d kernel/cred.o
> > 6140 724 16 6880 1ae0 net/sunrpc/auth.o
> >
> > after:
> > text data bss dec hex filename
> > 4573 552 8 5133 140d kernel/cred.o
> > 6236 724 16 6976 1b40 net/sunrpc/auth.o
> >
> >
> > Please explain, in a non handwavy and non cargoculty fashion why this
> > speed and space cost is justified.
>
> Since it's introduction, refcount_t has found a lot of bugs. This is easy
> to see even with a simplistic review of commits:
>
> $ git log --date=short --pretty='format:%ad %C(auto)%h ("%s")' \
> --grep 'refcount_t:' | \
> cut -d- -f1 | sort | uniq -c
> 1 2016
> 15 2017
> 9 2018
> 23 2019
> 24 2020
> 18 2021
> 24 2022
> 10 2023
>
> It's not really tapering off, either. All of these would have been silent
> memory corruptions, etc. In the face of _what_ is being protected,
> "cred" is pretty important for enforcing security boundaries, etc,
> so having it still not protected is a weird choice we've implicitly
> made. Given cred code is still seeing changes and novel uses (e.g.
> io_uring), it's not unreasonable to protect it from detectable (and
> _exploitable_) problems.
>
> While the size differences look large in cred.o, it's basically limited
> only to cred.o:
>
> text data bss dec hex filename
> 30515570 12255806 17190916 59962292 392f3b4 vmlinux.before
> 30517500 12255838 17190916 59964254 392fb5e vmlinux.after
>
> And we've even seen performance _improvements_ in some conditions:
> https://lore.kernel.org/lkml/20200615005732.GV12456@shao2-debian/
>
> Looking at confirmed security flaws, exploitable reference counting
> related bugs have dropped significantly. (The CVE database is irritating
> to search, but most recent refcount-related CVEs are due to counts that
> are _not_ using refcount_t.)
>
> I'd rather ask the question, "Why should we _not_ protect cred lifetime
> management?"
>
> -Kees
>
> --
> Kees Cook
