[bpf,1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE

Commit Message

Stanislav Fomichev April 26, 2024, 11:16 p.m. UTC

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/[e-mail]/
Reported-by: [e-mail]
Signed-off-by: Stanislav Fomichev <[e-mail]>
---
 kernel/bpf/syscall.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Eduard Zingerman April 29, 2024, 7:34 p.m. UTC | #1

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/[e-mail]/
> Reported-by: [e-mail]
> Signed-off-by: Stanislav Fomichev <[e-mail]>
> ---

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 <[e-mail]>

(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).

Stanislav Fomichev April 29, 2024, 9:17 p.m. UTC | #2

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/[e-mail]/
> > Reported-by: [e-mail]
> > Signed-off-by: Stanislav Fomichev <[e-mail]>
> > ---
> 
> 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 <[e-mail]>

[..]
 
> (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!

Patch

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c287925471f6..cb61d8880dbe 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3985,6 +3985,11 @@  static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
 			 * check permissions at attach time.
 			 */
 			return -EPERM;
+
+		ptype = attach_type_to_prog_type(attach_type);
+		if (prog->type != ptype)
+			return -EINVAL;
+
 		return prog->enforce_expected_attach_type &&
 			prog->expected_attach_type != attach_type ?
 			-EINVAL : 0;