[RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Edwin Peer]

Being able to load, verify and test BPF programs in unprivileged
build environments is desirable. The two phase load and then
test API makes this goal difficult to achieve, since relaxing
permissions for BPF_PROG_TEST_RUN alone would be insufficient.

The approach taken in this proposal defers CAP_SYS_ADMIN checks
until program attach time in order to unencumber BPF_PROG_LOAD.

Edwin Peer (2):
  bpf: defer capability checks until program attach
  bpf: relax CAP_SYS_ADMIN requirement for BPF_PROG_TEST_RUN

 include/linux/filter.h |  3 ++-
 kernel/bpf/syscall.c   | 27 +++++++++++++++++----------
 2 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.24.1

[RFC PATCH bpf-next 2/2] bpf: relax CAP_SYS_ADMIN requirement for BPF_PROG_TEST_RUN

[Edwin Peer]

Introduce a bpf_prog_get_lax() API to provide unprivileged access to BPF
programs. The new API is provided to make explicit the intent to allow
such access at a given call site, however, it is not exposed beyond the
syscall interface here as there is no plan to use this beyond
BPF_PROG_TEST_RUN at this stage. The semantics remain unchanged for all
other existing callers.

This change allows unprivileged users to execute BPF_PROG_TEST_RUN for
all BPF program types.

Signed-off-by: Edwin Peer <epeer@juniper.net>
---
 kernel/bpf/syscall.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 8e56768ebc06..970aeff9a9d9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1578,7 +1578,7 @@ bool bpf_prog_get_ok(struct bpf_prog *prog,
 }
 
 static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type,
-				       bool attach_drv)
+				       bool attach_drv, bool privilege_required)
 {
 	struct fd f = fdget(ufd);
 	struct bpf_prog *prog;
@@ -1587,7 +1587,8 @@ static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type,
 	if (IS_ERR(prog))
 		return prog;
 	if (prog->type != BPF_PROG_TYPE_SOCKET_FILTER &&
-	    prog->type != BPF_PROG_TYPE_CGROUP_SKB && !prog->privileged_load) {
+	    prog->type != BPF_PROG_TYPE_CGROUP_SKB &&
+	    privilege_required && !prog->privileged_load) {
 		prog = ERR_PTR(-EPERM);
 		goto out;
 	}
@@ -1604,13 +1605,18 @@ static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type,
 
 struct bpf_prog *bpf_prog_get(u32 ufd)
 {
-	return __bpf_prog_get(ufd, NULL, false);
+	return __bpf_prog_get(ufd, NULL, false, true);
+}
+
+struct bpf_prog *bpf_prog_get_lax(u32 ufd)
+{
+	return __bpf_prog_get(ufd, NULL, false, false);
 }
 
 struct bpf_prog *bpf_prog_get_type_dev(u32 ufd, enum bpf_prog_type type,
 				       bool attach_drv)
 {
-	return __bpf_prog_get(ufd, &type, attach_drv);
+	return __bpf_prog_get(ufd, &type, attach_drv, true);
 }
 EXPORT_SYMBOL_GPL(bpf_prog_get_type_dev);
 
@@ -2254,8 +2260,6 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
 	struct bpf_prog *prog;
 	int ret = -ENOTSUPP;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
 	if (CHECK_ATTR(BPF_PROG_TEST_RUN))
 		return -EINVAL;
 
@@ -2267,7 +2271,7 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
 	    (!attr->test.ctx_size_out && attr->test.ctx_out))
 		return -EINVAL;
 
-	prog = bpf_prog_get(attr->test.prog_fd);
+	prog = bpf_prog_get_lax(attr->test.prog_fd);
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
-- 
2.24.1

[RFC PATCH bpf-next 1/2] bpf: defer capability checks until program attach

[Edwin Peer]

The intent of this patch is not to change the effective permissions
required to run a BPF program of a given type in the kernel. The
actual check, however, is now deferred until attach time. For
example, an XDP program will fail to bind to a device with EPERM if
the program was not originally loaded under CAP_SYS_ADMIN.

This is achieved by remembering whether the program was loaded by a
privileged user within the BPF program's context. The upshot of this
is that BPF_PROG_LOAD is no longer a privileged operation, thereby
exposing access to the verifier to normal users for all program
types.

Signed-off-by: Edwin Peer <epeer@juniper.net>
---
 include/linux/filter.h |  3 ++-
 kernel/bpf/syscall.c   | 11 +++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index a141cb07e76a..1957eea62bed 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -534,7 +534,8 @@ struct bpf_prog {
 				is_func:1,	/* program is a bpf function */
 				kprobe_override:1, /* Do we override a kprobe? */
 				has_callchain_buf:1, /* callchain buffer allocated? */
-				enforce_expected_attach_type:1; /* Enforce expected_attach_type checking at attach time */
+				enforce_expected_attach_type:1, /* Enforce expected_attach_type checking at attach time */
+				privileged_load:1; /* Loaded with CAP_SYS_ADMIN */
 	enum bpf_prog_type	type;		/* Type of BPF program */
 	enum bpf_attach_type	expected_attach_type; /* For some prog types */
 	u32			len;		/* Number of filter blocks */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e3461ec59570..8e56768ebc06 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1586,6 +1586,11 @@ static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type,
 	prog = ____bpf_prog_get(f);
 	if (IS_ERR(prog))
 		return prog;
+	if (prog->type != BPF_PROG_TYPE_SOCKET_FILTER &&
+	    prog->type != BPF_PROG_TYPE_CGROUP_SKB && !prog->privileged_load) {
+		prog = ERR_PTR(-EPERM);
+		goto out;
+	}
 	if (!bpf_prog_get_ok(prog, attach_type, attach_drv)) {
 		prog = ERR_PTR(-EINVAL);
 		goto out;
@@ -1733,10 +1738,6 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 	if (attr->insn_cnt == 0 ||
 	    attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
 		return -E2BIG;
-	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
-	    type != BPF_PROG_TYPE_CGROUP_SKB &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
 
 	bpf_prog_load_fixup_attach_type(attr);
 	if (bpf_prog_load_check_attach(type, attr->expected_attach_type,
@@ -1749,6 +1750,8 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 	if (!prog)
 		return -ENOMEM;
 
+	prog->privileged_load = capable(CAP_SYS_ADMIN);
+
 	prog->expected_attach_type = attr->expected_attach_type;
 	prog->aux->attach_btf_id = attr->attach_btf_id;
 	if (attr->attach_prog_fd) {
-- 
2.24.1

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Y Song]

Added cc to bpf@vger.kernel.org.
For bpf related patches, please cc bpf@vger.kernel.org which is major
place for bpf discussions.

On Wed, Dec 18, 2019 at 6:16 PM Edwin Peer <epeer@juniper.net> wrote:
>
> Being able to load, verify and test BPF programs in unprivileged
> build environments is desirable. The two phase load and then
> test API makes this goal difficult to achieve, since relaxing
> permissions for BPF_PROG_TEST_RUN alone would be insufficient.
>
> The approach taken in this proposal defers CAP_SYS_ADMIN checks
> until program attach time in order to unencumber BPF_PROG_LOAD.

I like the idea to *test* bpf program in unprivileged mode as sudo always
has some risk to break the development server.

Have you tried your patch with some bpf programs? verifier and jit  put some
restrictions on unpriv programs. To truely test the program, most if
not all these
restrictions should be lifted, so the same tested program should be able to
run on production server and vice verse.

>
> Edwin Peer (2):
>   bpf: defer capability checks until program attach
>   bpf: relax CAP_SYS_ADMIN requirement for BPF_PROG_TEST_RUN
>
>  include/linux/filter.h |  3 ++-
>  kernel/bpf/syscall.c   | 27 +++++++++++++++++----------
>  2 files changed, 19 insertions(+), 11 deletions(-)
>
> --
> 2.24.1

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Edwin Peer]

On 12/18/19, 23:19, "Y Song" <ys114321@gmail.com> wrote:

>  Added cc to bpf@vger.kernel.org.
    
Thank you, I will remember to do this next time.

> Have you tried your patch with some bpf programs? verifier and jit  put some
> restrictions on unpriv programs. To truely test the program, most if not all these
> restrictions should be lifted, so the same tested program should be able to
> run on production server and vice verse.

Agreed, I am aware of some of these differences in the load/verifier behavior with and without
CAP_SYS_ADMIN. In particular, without CAP_SYS_ADMIN programs are still restricted to 4k, some helpers are not available (spin locks, trace printk) and there are some differences in context access checks.

I think these can be addressed incrementally, assuming folk are on board with this approach in general?

Regards,
Edwin Peer
 

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Daniel Borkmann]

On Thu, Dec 19, 2019 at 02:50:42PM +0000, Edwin Peer wrote:
> On 12/18/19, 23:19, "Y Song" <ys114321@gmail.com> wrote:
> 
> >  Added cc to bpf@vger.kernel.org.
>
> Thank you, I will remember to do this next time.
>
> > Have you tried your patch with some bpf programs? verifier and jit  put some
> > restrictions on unpriv programs. To truely test the program, most if not all these
> > restrictions should be lifted, so the same tested program should be able to
> > run on production server and vice verse.
> 
> Agreed, I am aware of some of these differences in the load/verifier behavior with and without
> CAP_SYS_ADMIN. In particular, without CAP_SYS_ADMIN programs are still restricted to 4k, some helpers are not available (spin locks, trace printk) and there are some differences in context access checks.
> 
> I think these can be addressed incrementally, assuming folk are on board with this approach in general?

What about CAP_BPF? IIRC, there are also other issues e.g. you could abuse
the test interface as a packet generator (bpf_clone_redirect) which is not
something fully unpriv should be doing.

Thanks,
Daniel

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Edwin Peer]

On 12/19/19, 07:47, "Daniel Borkmann" <daniel@iogearbox.net> wrote:

>  What about CAP_BPF?

What is the status of this? It might solve some of the problems, but it is still puts testing
BPF outside reach of normal users.

> IIRC, there are also other issues e.g. you could abuse the test interface as a packet
>  generator (bpf_clone_redirect) which is not something fully unpriv should be doing.

Good point. I suspect solutions exist - I'm trying to ascertain if they are worth pursuing
or if the idea of unprivileged testing is a complete non-starter to begin with.

Are there other helpers of concern that come immediately to mind? A first stab might
add these to the list in the verifier that require privilege. This has the drawback that
programs that actually need this kind of functionality are beyond the test framework.

Another idea might be to have some kind of test classification stored in the program
context. That way, dangerous helpers could be replaced by versions that mock the
functionality in some safe way. Perhaps having test programs only have access to a
subset of virtual netdevs for clone redirect, for example. 

Regards,
Edwin Peer
  

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Alexei Starovoitov]

On Thu, Dec 19, 2019 at 05:05:42PM +0000, Edwin Peer wrote:
> On 12/19/19, 07:47, "Daniel Borkmann" <daniel@iogearbox.net> wrote:
> 
> >  What about CAP_BPF?
> 
> What is the status of this? It might solve some of the problems, but it is still puts testing
> BPF outside reach of normal users.

why?
I think CAP_BPF is solving exactly what you're trying to achieve.
Use CAP_BPF to load _any_ program type and use prog_test_run to run it.
While discussing CAP_BPF during plumbers conf we realized that the kernel doesn't need
to check CAP_BPF for prog_test_run. It's user supplied data. No security risk. Though
the kernel needs to make sure that dangerous helpers are not used for prog_test_run.
Whether bpf_clone_redirect() is such helper is still tbd. Unpriv user can flood netdevs
without any bpf.

> > IIRC, there are also other issues e.g. you could abuse the test interface as a packet
> >  generator (bpf_clone_redirect) which is not something fully unpriv should be doing.
> 
> Good point. I suspect solutions exist - I'm trying to ascertain if they are worth pursuing
> or if the idea of unprivileged testing is a complete non-starter to begin with.
> 
> Are there other helpers of concern that come immediately to mind? A first stab might
> add these to the list in the verifier that require privilege. This has the drawback that
> programs that actually need this kind of functionality are beyond the test framework.

So far majority of programs require root-only verifier features. The programs are
getting more complex and benefit the most from testing. Relaxing test_run for unpriv
progs is imo very narrow use case. I'd rather use CAP_BPF.

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Edwin Peer]

On 12/19/19, 11:26, "Alexei Starovoitov" <alexei.starovoitov@gmail.com> wrote:

> On Thu, Dec 19, 2019 at 05:05:42PM +0000, Edwin Peer wrote:
>> On 12/19/19, 07:47, "Daniel Borkmann" <daniel@iogearbox.net> wrote:
>> 
>> >  What about CAP_BPF?
>> 
>> What is the status of this? It might solve some of the problems, but it is still puts testing
>> BPF outside reach of normal users.
>    
> why?
> I think CAP_BPF is solving exactly what you're trying to achieve.

I'm trying to provide access to BPF testing infrastructure for unprivileged
users (assuming it can be done in a safe way, which I'm as yet unsure of).
CAP_BPF is not the same thing, because at least some kind of root
intervention is required to attain CAP_BPF in the first place.

> Whether bpf_clone_redirect() is such helper is still tbd. Unpriv user can flood netdevs
> without any bpf.
   
True, but presumably such would still be subject to administrator
controlled QoS and firewall policy? Also unprivileged users presumably
can't create arbitrary packets coming from spoofed IPs / MACs, which I
believe requires CAP_NET_RAW?
 
>> Are there other helpers of concern that come immediately to mind? A first stab might
>> add these to the list in the verifier that require privilege. This has the drawback that
>> programs that actually need this kind of functionality are beyond the test framework.
>    
>   So far majority of programs require root-only verifier features. The programs are
>   getting more complex and benefit the most from testing. Relaxing test_run for unpriv
>   progs is imo very narrow use case. I'd rather use CAP_BPF.
    
The more elaborate proposal called for mocking these aspects for
testing, which could conceivably resolve this? That said, I see an
incremental path to this, adding such as needed. The narrowness
of the use case really depends on exactly what you're trying to do.
Something in XDP, for example, has very little kernel dependencies
(possibly none that would be affected here) and represents an entire
class of use cases that could have unprivileged testing be supported.

Regards,
Edwin Peer

Re: [RFC PATCH bpf-next 0/2] unprivileged BPF_PROG_TEST_RUN

[Alexei Starovoitov]

On Thu, Dec 19, 2019 at 08:06:21PM +0000, Edwin Peer wrote:
> On 12/19/19, 11:26, "Alexei Starovoitov" <alexei.starovoitov@gmail.com> wrote:
> 
> > On Thu, Dec 19, 2019 at 05:05:42PM +0000, Edwin Peer wrote:
> >> On 12/19/19, 07:47, "Daniel Borkmann" <daniel@iogearbox.net> wrote:
> >> 
> >> >  What about CAP_BPF?
> >> 
> >> What is the status of this? It might solve some of the problems, but it is still puts testing
> >> BPF outside reach of normal users.
> >    
> > why?
> > I think CAP_BPF is solving exactly what you're trying to achieve.
> 
> I'm trying to provide access to BPF testing infrastructure for unprivileged
> users (assuming it can be done in a safe way, which I'm as yet unsure of).
> CAP_BPF is not the same thing, because at least some kind of root
> intervention is required to attain CAP_BPF in the first place.

yes and test infra can bet setup with CAP_BPF.
The desire of testing frameworks to work without root was one of the main
motivations for us to work on CAP_BPF.

> > Whether bpf_clone_redirect() is such helper is still tbd. Unpriv user can flood netdevs
> > without any bpf.
>    
> True, but presumably such would still be subject to administrator
> controlled QoS and firewall policy? Also unprivileged users presumably
> can't create arbitrary packets coming from spoofed IPs / MACs, which I
> believe requires CAP_NET_RAW?
>  
> >> Are there other helpers of concern that come immediately to mind? A first stab might
> >> add these to the list in the verifier that require privilege. This has the drawback that
> >> programs that actually need this kind of functionality are beyond the test framework.
> >    
> >   So far majority of programs require root-only verifier features. The programs are
> >   getting more complex and benefit the most from testing. Relaxing test_run for unpriv
> >   progs is imo very narrow use case. I'd rather use CAP_BPF.
>     
> The more elaborate proposal called for mocking these aspects for
> testing, which could conceivably resolve this? That said, I see an
> incremental path to this, adding such as needed. The narrowness
> of the use case really depends on exactly what you're trying to do.
> Something in XDP, for example, has very little kernel dependencies
> (possibly none that would be affected here) and represents an entire
> class of use cases that could have unprivileged testing be supported.

I'm looking at public and non-public XDP progs and none of them are verifiable
as unpriv. I don't think it's a good idea to build infra for toy programs.