netdev - Re: [PATCH bpf 1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK

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: Mon, 29 Apr 2024 14:17:05 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org, 
	daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev, 
	song@...nel.org, yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org, 
	haoluo@...gle.com, jolsa@...nel.org, 
	syzbot+838346b979830606c854@...kaller.appspotmail.com
Subject: Re: [PATCH bpf 1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type
 enforcement in BPF_LINK_CREATE

On 04/29, Eduard Zingerman wrote:
> On Fri, 2024-04-26 at 16:16 -0700, Stanislav Fomichev wrote:
> > bpf_prog_attach uses attach_type_to_prog_type to enforce proper
> > attach type for BPF_PROG_TYPE_CGROUP_SKB. link_create uses
> > bpf_prog_get and relies on bpf_prog_attach_check_attach_type
> > to properly verify prog_type <> attach_type association.
> > 
> > Add missing attach_type enforcement for the link_create case.
> > Otherwise, it's currently possible to attach cgroup_skb prog
> > types to other cgroup hooks.
> > 
> > Fixes: af6eea57437a ("bpf: Implement bpf_link-based cgroup BPF program attachment")
> > Link: https://lore.kernel.org/bpf/0000000000004792a90615a1dde0@google.com/
> > Reported-by: syzbot+838346b979830606c854@...kaller.appspotmail.com
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> 
> I've spent some time comparing:
> - syscall.c:bpf_prog_attach()
> - syscall.c:link_create()
> - syscall.c:bpf_prog_attach_check_attach_type()
> - syscall.c:attach_type_to_prog_type()
> - verifier.c:check_return_code()
> 
> And it looks like BPF_PROG_TYPE_CGROUP_SKB is the only thing with
> missing attach type checks.
> 
> Acked-by: Eduard Zingerman <eddyz87@...il.com>

[..]
 
> (The interplay between the above functions seems a bit messy,
>  but I don't have good suggestions for refactoring at the moment.
>  It appears that bpf_prog_attach_check_attach_type() could be simplified
>  if prog->enforce_expected_attach_type would be set more aggressively and
> 
> 	if (prog->enforce_expected_attach_type &&
> 	    prog->expected_attach_type != attach_type)
> 		return -EINVAL;
> 
>  moved as a top-level check outside of the switch.
>  Also BPF_PROG_TYPE_SCHED_CLS case could be removed as it is handled
>  by default branch. But these are larger changes).

Fully agree on this one. Now that we have some tests around that part,
we can probably try to simplify it a bit. Those small differences with
link_create vs prog_attach are a bit hard to follow.

Thanks for the review!