linux-hardening - Re: [PATCH v2] creds: Convert cred.usage to refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEXv5_h4T38JSMxQd7Px3MrgqOyBsDDSMh8Q=Wr8Tu7=TBvG1Q@mail.gmail.com>
Date:   Fri, 18 Aug 2023 16:21:30 -0400
From:   David Windsor <dwindsor@...il.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-hardening@...r.kernel.org,
        Elena Reshetova <elena.reshetova@...el.com>,
        Hans Liljestrand <ishkamiel@...il.com>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>,
        Chuck Lever <chuck.lever@...cle.com>,
        Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Alexey Gladkov <legion@...nel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Yu Zhao <yuzhao@...gle.com>, linux-kernel@...r.kernel.org,
        linux-nfs@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2] creds: Convert cred.usage to refcount_t

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@...omium.org> 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@...omium.org> wrote:
> >
> > > From: Elena Reshetova <elena.reshetova@...el.com>
> > >
> > > 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