* [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD
@ 2019-05-28 16:53 Andreas Steinmetz
2019-05-28 21:04 ` Song Liu
2019-06-03 17:12 ` Nicolas Dichtel
0 siblings, 2 replies; 6+ messages in thread
From: Andreas Steinmetz @ 2019-05-28 16:53 UTC (permalink / raw)
To: netdev, bpf
[sorry for crossposting but this affects both lists]
BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_XDP should be allowed
for CAP_NET_ADMIN capability. Nearly everything one can do with
these program types can be done some other way with CAP_NET_ADMIN
capability (e.g. NFQUEUE), but only slower.
This change is similar in behaviour to the /proc/sys/net
CAP_NET_ADMIN exemption.
Overall chances are of increased security as network related
applications do no longer require to keep CAP_SYS_ADMIN
admin capability for network related eBPF operations.
It may well be that other program types than BPF_PROG_TYPE_XDP
and BPF_PROG_TYPE_SCHED_CLS do need the same exemption, though
I do not have sufficient knowledge of other program types
to be able to decide this.
Preloading BPF programs is not possible in case of application
modified or generated BPF programs, so this is no alternative.
The verifier does prevent the BPF program from doing harmful
things anyway.
Signed-off-by: Andreas Steinmetz <ast@domdv.de>
--- a/kernel/bpf/syscall.c 2019-05-28 18:00:40.472841432 +0200
+++ b/kernel/bpf/syscall.c 2019-05-28 18:17:50.162811510 +0200
@@ -1561,8 +1561,13 @@ static int bpf_prog_load(union bpf_attr
return -E2BIG;
if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
type != BPF_PROG_TYPE_CGROUP_SKB &&
- !capable(CAP_SYS_ADMIN))
- return -EPERM;
+ !capable(CAP_SYS_ADMIN)) {
+ if (type != BPF_PROG_TYPE_SCHED_CLS &&
+ type != BPF_PROG_TYPE_XDP)
+ return -EPERM;
+ if(!capable(CAP_NET_ADMIN))
+ return -EPERM;
+ }
bpf_prog_load_fixup_attach_type(attr);
if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD
2019-05-28 16:53 [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD Andreas Steinmetz
@ 2019-05-28 21:04 ` Song Liu
2019-06-05 10:56 ` Andreas Steinmetz
2019-06-03 17:12 ` Nicolas Dichtel
1 sibling, 1 reply; 6+ messages in thread
From: Song Liu @ 2019-05-28 21:04 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: Networking, bpf
On Tue, May 28, 2019 at 9:59 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> [sorry for crossposting but this affects both lists]
>
> BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_XDP should be allowed
> for CAP_NET_ADMIN capability. Nearly everything one can do with
> these program types can be done some other way with CAP_NET_ADMIN
> capability (e.g. NFQUEUE), but only slower.
>
> This change is similar in behaviour to the /proc/sys/net
> CAP_NET_ADMIN exemption.
>
> Overall chances are of increased security as network related
> applications do no longer require to keep CAP_SYS_ADMIN
> admin capability for network related eBPF operations.
>
> It may well be that other program types than BPF_PROG_TYPE_XDP
> and BPF_PROG_TYPE_SCHED_CLS do need the same exemption, though
> I do not have sufficient knowledge of other program types
> to be able to decide this.
>
> Preloading BPF programs is not possible in case of application
> modified or generated BPF programs, so this is no alternative.
> The verifier does prevent the BPF program from doing harmful
> things anyway.
>
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
>
> --- a/kernel/bpf/syscall.c 2019-05-28 18:00:40.472841432 +0200
> +++ b/kernel/bpf/syscall.c 2019-05-28 18:17:50.162811510 +0200
> @@ -1561,8 +1561,13 @@ static int bpf_prog_load(union bpf_attr
> return -E2BIG;
> if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> type != BPF_PROG_TYPE_CGROUP_SKB &&
You should extend this if () statement instead of adding another
if () below.
Thanks,
Song
> - !capable(CAP_SYS_ADMIN))
> - return -EPERM;
> + !capable(CAP_SYS_ADMIN)) {
> + if (type != BPF_PROG_TYPE_SCHED_CLS &&
> + type != BPF_PROG_TYPE_XDP)
> + return -EPERM;
> + if(!capable(CAP_NET_ADMIN))
> + return -EPERM;
> + }
>
> bpf_prog_load_fixup_attach_type(attr);
> if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD
2019-05-28 21:04 ` Song Liu
@ 2019-06-05 10:56 ` Andreas Steinmetz
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Steinmetz @ 2019-06-05 10:56 UTC (permalink / raw)
To: Song Liu; +Cc: Networking, bpf
On Tue, 2019-05-28 at 14:04 -0700, Song Liu wrote:
> > if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> > type != BPF_PROG_TYPE_CGROUP_SKB &&
>
> You should extend this if () statement instead of adding another
> if () below.
Reworking the if-statement is possible but the result is something like:
if ((type != BPF_PROG_TYPE_SOCKET_FILTER &&
type != BPF_PROG_TYPE_CGROUP_SKB &&
!capable(CAP_SYS_ADMIN)) &&
!((type == BPF_PROG_TYPE_SCHED_CLS ||
type == BPF_PROG_TYPE_XDP) &&
capable(CAP_NET_ADMIN)))
return -EPERM;
This is not really readable and I do prefer an easy to verify code
when it comes to security, so how about the following version:
Signed-off-by: Andreas Steinmetz <ast@domdv.de>
--- a/kernel/bpf/syscall.c 2019-05-28 18:00:40.472841432 +0200
+++ b/kernel/bpf/syscall.c 2019-06-05 12:34:48.197107612 +0200
@@ -1559,10 +1559,18 @@ static int bpf_prog_load(union bpf_attr
if (attr->insn_cnt == 0 || attr->insn_cnt > BPF_MAXINSNS)
return -E2BIG;
- if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
- type != BPF_PROG_TYPE_CGROUP_SKB &&
- !capable(CAP_SYS_ADMIN))
- return -EPERM;
+ switch (type) {
+ case BPF_PROG_TYPE_SOCKET_FILTER:
+ case BPF_PROG_TYPE_CGROUP_SKB:
+ break;
+ case BPF_PROG_TYPE_SCHED_CLS:
+ case BPF_PROG_TYPE_XDP:
+ if (capable(CAP_NET_ADMIN))
+ break;
+ default:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ }
bpf_prog_load_fixup_attach_type(attr);
if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD
2019-05-28 16:53 [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD Andreas Steinmetz
2019-05-28 21:04 ` Song Liu
@ 2019-06-03 17:12 ` Nicolas Dichtel
2019-06-05 10:59 ` Andreas Steinmetz
1 sibling, 1 reply; 6+ messages in thread
From: Nicolas Dichtel @ 2019-06-03 17:12 UTC (permalink / raw)
To: Andreas Steinmetz, netdev, bpf
Le 28/05/2019 à 18:53, Andreas Steinmetz a écrit :
> [sorry for crossposting but this affects both lists]
>
> BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_XDP should be allowed
> for CAP_NET_ADMIN capability. Nearly everything one can do with
> these program types can be done some other way with CAP_NET_ADMIN
> capability (e.g. NFQUEUE), but only slower.
>
> This change is similar in behaviour to the /proc/sys/net
> CAP_NET_ADMIN exemption.
>
> Overall chances are of increased security as network related
> applications do no longer require to keep CAP_SYS_ADMIN
> admin capability for network related eBPF operations.
>
> It may well be that other program types than BPF_PROG_TYPE_XDP
> and BPF_PROG_TYPE_SCHED_CLS do need the same exemption, though
> I do not have sufficient knowledge of other program types
> to be able to decide this.
>
> Preloading BPF programs is not possible in case of application
> modified or generated BPF programs, so this is no alternative.
> The verifier does prevent the BPF program from doing harmful
> things anyway.
>
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
It makes sense to me.
Do you plan to submit it formally?
Looking a bit more at this topic, I see that most part of the bpf code uses
capable(CAP_NET_ADMIN). I don't see why we cannot use ns_capable(CAP_NET_ADMIN).
Regards,
Nicolas
>
> --- a/kernel/bpf/syscall.c 2019-05-28 18:00:40.472841432 +0200
> +++ b/kernel/bpf/syscall.c 2019-05-28 18:17:50.162811510 +0200
> @@ -1561,8 +1561,13 @@ static int bpf_prog_load(union bpf_attr
> return -E2BIG;
> if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> type != BPF_PROG_TYPE_CGROUP_SKB &&
> - !capable(CAP_SYS_ADMIN))
> - return -EPERM;
> + !capable(CAP_SYS_ADMIN)) {
> + if (type != BPF_PROG_TYPE_SCHED_CLS &&
> + type != BPF_PROG_TYPE_XDP)
> + return -EPERM;
> + if(!capable(CAP_NET_ADMIN))
> + return -EPERM;
> + }
>
> bpf_prog_load_fixup_attach_type(attr);
> if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD
2019-06-03 17:12 ` Nicolas Dichtel
@ 2019-06-05 10:59 ` Andreas Steinmetz
2019-06-05 11:51 ` Nicolas Dichtel
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Steinmetz @ 2019-06-05 10:59 UTC (permalink / raw)
To: nicolas.dichtel, netdev, bpf
On Mon, 2019-06-03 at 19:12 +0200, Nicolas Dichtel wrote:
> It makes sense to me.
> Do you plan to submit it formally?
>
> Looking a bit more at this topic, I see that most part of the bpf
> code uses
> capable(CAP_NET_ADMIN). I don't see why we cannot use
> ns_capable(CAP_NET_ADMIN).
If there is a change for this to get accepted, sure, I'm willing to
submit this formally (need some advice, though).
As for capable vs. ns_capable, this is a bit above my knowledge of
kernel internals.
Regards,
Andreas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD
2019-06-05 10:59 ` Andreas Steinmetz
@ 2019-06-05 11:51 ` Nicolas Dichtel
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Dichtel @ 2019-06-05 11:51 UTC (permalink / raw)
To: Andreas Steinmetz, netdev, bpf
Le 05/06/2019 à 12:59, Andreas Steinmetz a écrit :
[snip]
> If there is a change for this to get accepted, sure, I'm willing to
> submit this formally (need some advice, though).
At least, you need to submit it without the RFC tag. RFC patches are not aimed
to be merged.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-05 11:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 16:53 [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD Andreas Steinmetz
2019-05-28 21:04 ` Song Liu
2019-06-05 10:56 ` Andreas Steinmetz
2019-06-03 17:12 ` Nicolas Dichtel
2019-06-05 10:59 ` Andreas Steinmetz
2019-06-05 11:51 ` Nicolas Dichtel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).