netdev - Re: [PATCH bpf-next 03/29] bpf: introduce BPF token object

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: <CAADnVQ+QP_ZfY6gqEUtW2mYgO5usX+cK9w+X5kaRt1ovf-q1Cw@mail.gmail.com>
Date: Fri, 5 Jan 2024 14:27:43 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Linus Torvalds <torvalds@...uxfoundation.org>, Andrii Nakryiko <andrii@...nel.org>, 
	bpf <bpf@...r.kernel.org>, Network Development <netdev@...r.kernel.org>, 
	Paul Moore <paul@...l-moore.com>, Christian Brauner <brauner@...nel.org>, 
	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>, 
	LSM List <linux-security-module@...r.kernel.org>, Kernel Team <kernel-team@...a.com>
Subject: Re: [PATCH bpf-next 03/29] bpf: introduce BPF token object

On Fri, Jan 5, 2024 at 2:06 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Fri, Jan 5, 2024 at 12:26 PM Linus Torvalds
> <torvalds@...uxfoundation.org> wrote:
> >
> > I'm still looking through the patches, but in the early parts I do
> > note this oddity:
> >
> > On Wed, 3 Jan 2024 at 14:21, Andrii Nakryiko <andrii@...nel.org> wrote:
> > >
> > > +struct bpf_token {
> > > +       struct work_struct work;
> > > +       atomic64_t refcnt;
> > > +       struct user_namespace *userns;
> > > +       u64 allowed_cmds;
> > > +};
> >
> > Ok, not huge, and makes sense, although I wonder if that
> >
> >         atomic64_t refcnt;
> >
> > should just be 'atomic_long_t' since presumably on 32-bit
> > architectures you can't create enough references for a 64-bit atomic
> > to make much sense.
> >
> > Or are there references to tokens that might not use any memory?
> >
> > Not a big deal, but 'atomic64_t' is very expensive on 32-bit
> > architectures, and doesn't seem to make much sense unless you really
> > specifically need 64 bits for some reason.
>
> I used atomic64_t for consistency with other BPF objects (program,
> etc) and not to have to worry even about hypothetical overflows.
> 32-bit atomic performance doesn't seem to be a big concern as a token
> is passed into a pretty heavy-weight operations that create new BPF
> object (map, program, BTF object), no matter how slow refcounting is
> it will be lost in the noise for those operations.

To add a bit more context here...

Back in 2016 Jann managed to overflow 32-bit prog/map counters:
"
On a system with >32Gbyte of physical memory,
the malicious application may overflow 32-bit bpf program refcnt.
It's also possible to overflow map refcnt on 1Tb system.
"
We mitigated that with fixed limits:
-       atomic_inc(&map->refcnt);
+       if (atomic_inc_return(&map->refcnt) > BPF_MAX_REFCNT) {
+               atomic_dec(&map->refcnt);
+               return ERR_PTR(-EBUSY);
+       }
but it created quite a lot of error handling pain throughout
the code, so eventually in 2019 we switched to atomic64_t refcnt
and never looked back.
I suspect Jann will be able to overflow 32-bit token refcnt,
so atomic64 was chosen for simplicity.
atomic_long_t might work too, but the effort to think it through
is not worth it at this point, since performance of
inc/dec doesn't matter here.

Eventually we can do a follow up and consistently update
all such counters to atomic_long_t.