linux-kernel - Re: [PATCH bpf-next v3] bpf: Remove redundant checks

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] [day] [month] [year] [list]

Message-ID: <CAEf4BzZtOUWyQego7Uj5GH48-bLM1stSyi-uZ=+JTtGzYojFAA@mail.gmail.com>
Date: Wed, 23 Apr 2025 10:05:00 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Zvi Effron <zeffron@...tgames.com>
Cc: Feng Yang <yangfeng59949@....com>, ast@...nel.org, daniel@...earbox.net, 
	andrii@...nel.org, martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	mattbobrowski@...gle.com, rostedt@...dmis.org, mhiramat@...nel.org, 
	mathieu.desnoyers@...icios.com, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v3] bpf: Remove redundant checks

On Wed, Apr 16, 2025 at 3:44 PM Zvi Effron <zeffron@...tgames.com> wrote:
>
> On Wed, Apr 16, 2025 at 2:56 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <yangfeng59949@....com> wrote:
> > >
> > > From: Feng Yang <yangfeng@...inos.cn>
> > >
> > > Many conditional checks in switch-case are redundant
> > > with bpf_base_func_proto and should be removed.
> > >
> > > Signed-off-by: Feng Yang <yangfeng@...inos.cn>
> > > Acked-by: Song Liu <song@...nel.org>
> > > ---
> > > Changes in v3:
> > > - Only modify patch description information.
> > > - Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@163.com/
> > >
> > > Changes in v2:
> > > - Only modify patch description information.
> > > - Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@163.com/
> > > ---
> > > kernel/trace/bpf_trace.c | 72 ----------------------------------------
> > > 1 file changed, 72 deletions(-)
> > >
> >
> > All this looks good, I checked that those functions indeed are allowed
> > in bpf_base_func_proto. The only (minor) differences are capabilities,
> > bpf_base_func_proto() correctly guards some of the helpers with
> > CAP_BPF and/or CAP_PERFMON checks, while bpf_tracing_func_proto()
> > doesn't seem to bother (which is either a bug or any tracing prog
> > implies CAP_BPF and CAP_PERFMON, I'm not sure, didn't check).
> >
> > But I think we can take it further and remove even more stuff from
> > bpf_tracing_func_proto and/or add more stuff into bpf_base_func_proto
> > (perhaps as a few patches in a series, so it's easier to review and
> > validate).
> >
> > Basically, except for a few custom implementations that depend on
> > tracing program type (like get_stack and others like that), if
> > something is OK to call from a tracing program it should be ok to call
> > from any program type. And as such it can (should?) be added to
> > bpf_base_func_proto, IMO.
>
> Is this true? Does it make sense? (See below.)
>
> > P.S. I'd name the patch/series as "bpf: streamline allowed helpers
> > between tracing and base sets" or something like that to make the
> > purpose clearer
> >
> > [...]
> >
> > > case BPF_FUNC_get_current_uid_gid:
> > > return &bpf_get_current_uid_gid_proto;
> > > case BPF_FUNC_get_current_comm:
> > > return &bpf_get_current_comm_proto;
> >
> > I'm surprised these two are not part of bpf_base_func_proto, tbh...
> > maybe let's move them there while we are cleaning all this up?
>
> Do these make sense in all BPF program types such that they belong in
> bpf_base_func_proto? For example, XDP programs don't have a current uid and
> gid, do they?

everything in the kernel, whether NMI handler, hardirq, softirq, or
whatnot runs with *some* current task. So in that sense there is
always pid/tgid and uid/gid. It might not be very relevant for XDP
programs, but it's there, and so if we allow to get current pid/tgid,
why not allow the current comm and uid/gid?

>
> > pw-bot: cr
> >
> > > - case BPF_FUNC_trace_printk:
> > > - return bpf_get_trace_printk_proto();
> > > case BPF_FUNC_get_smp_processor_id:
> > > return &bpf_get_smp_processor_id_proto;
> >
> > this one should be cleaned up as well and
> > bpf_get_smp_processor_id_proto removed. All BPF programs either
> > disable CPU preemption or CPU migration, so bpf_base_func_proto's
> > implementation should work just fine (but please do it as a separate
> > patch)
> >
> > > - case BPF_FUNC_get_numa_node_id:
> > > - return &bpf_get_numa_node_id_proto;
> > > case BPF_FUNC_perf_event_read:
> > > return &bpf_perf_event_read_proto;
> >
> > [...]
> >