linux-kernel - Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs

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]

Date:   Tue, 4 Oct 2022 10:10:48 -0500
From:   David Vernet <void@...ifault.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     Martin KaFai Lau <martin.lau@...ux.dev>, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, kernel-team@...com,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org, yhs@...com,
        song@...nel.org, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org, tj@...nel.org
Subject: Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs

On Tue, Oct 04, 2022 at 12:22:08AM +0200, Kumar Kartikeya Dwivedi wrote:
> > Thanks for providing additional context, Kumar. So what do we want to do
> > for this patch set? IMO it doesn't seem useful to restrict
> > bpf_kfunc_acquire() to only be callable by non-sleepable programs if our
> > goal is to avoid crashes for nested task structs. We could easily
> > accidentally crash if e.g. those pointers are NULL, or someone is doing
> > something weird like stashing some extra flag bits in unused portions of
> > the pointer which are masked out when it's actually dereferenced
> > regardless of whether we're in RCU.  Trusting ctx loads sounds like the
> > right approach, barring some of the challenges you pointed out such as
> > dealing with fexit paths after free where the object may not be valid
> > anymore.
> >
> > In general, it seems like we should maybe decide on what our policy
> > should be for kfuncs until we can wire up whatever we need to properly
> > trust ctx.
> 
> Well, we could add it now and work towards closing the gaps after
> this, especially if bpf_task_acquire is really only useful in
> sleepable programs where it works on the tracing args. A lot of other
> kfuncs need these fixes as well, so it's a general problem and not
> specific to this set. I am not very familiar with your exact use case.
> Hopefully when it is fixed this particular case won't really break, if
> you only use the tracepoint argument.

I'm also interested in using this with struct_ops, not just tracing. I
think that struct_ops should be totally fine though, and easier to
reason about than tracing as we just have to make sure that a few
specific callbacks are always passed a valid, referenced task, rather
than e.g. worrying about fexit on __put_task_struct().

I'm fine with adding this now and working towards closing the gaps
later, though I'd like to hear what Martin, Alexei, and the rest of the
BPF maintainers think. I think Martin asked if there was any preliminary
work you'd already done that we could try to tie into this patch set,
and I'm similarly curious.

> It is true that waiting for all the fixes will unnecessarily stall
> this, it is not clear how each of the issues will be addressed either.
> 
> Later its use can be made conditional in sleepable programs for
> trusted and rcu tagged pointers under appropriate RCU read lock. I
> will try to prioritize sending it out so that we resolve this soon.

Much appreciated!