| Message ID | 20170426182419.14574-5-hannes@stressinduktion.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 04/26/2017 08:24 PM, Hannes Frederic Sowa wrote: > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> Ahh, looks this got swapped with 3/6. > --- > include/linux/filter.h | 6 ++++-- > kernel/bpf/core.c | 4 +++- > kernel/bpf/syscall.c | 7 ++++--- > kernel/bpf/verifier.c | 4 ++-- > net/core/filter.c | 6 +++--- > 5 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index 63624c619e371b..635311f57bf24f 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -413,7 +413,8 @@ struct bpf_prog { > locked:1, /* Program image locked? */ > gpl_compatible:1, /* Is filter GPL compatible? */ > cb_access:1, /* Is control block accessed? */ > - dst_needed:1; /* Do we need dst entry? */ > + dst_needed:1, /* Do we need dst entry? */ > + priv_cap_sys_admin:1; /* Where we loaded as sys_admin? */ > kmemcheck_bitfield_end(meta); > enum bpf_prog_type type; /* Type of BPF program */ [...] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 6f8b6ed690be93..24c9dac374770f 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3488,7 +3488,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr) > if (ret < 0) > goto skip_full_check; > > - env->allow_ptr_leaks = capable(CAP_SYS_ADMIN); > + env->allow_ptr_leaks = env->prog->priv_cap_sys_admin; > > ret = do_check(env); > > @@ -3589,7 +3589,7 @@ int bpf_analyzer(struct bpf_prog *prog, const struct bpf_ext_analyzer_ops *ops, > if (ret < 0) > goto skip_full_check; > > - env->allow_ptr_leaks = capable(CAP_SYS_ADMIN); > + env->allow_ptr_leaks = prog->priv_cap_sys_admin; > > ret = do_check(env); > > diff --git a/net/core/filter.c b/net/core/filter.c > index 9a37860a80fc78..dc020d40bb770a 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1100,7 +1100,7 @@ int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog) > if (!bpf_check_basics_ok(fprog->filter, fprog->len)) > return -EINVAL; > > - fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0); > + fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0, false); > if (!fp) > return -ENOMEM; > Did you check that transferring allow_ptr_leaks doesn't have a side effect on the nfp JIT? I believe it can also do cbpf migrations to a certain extend.
On Wed, Apr 26, 2017 at 08:24:17PM +0200, Hannes Frederic Sowa wrote: > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > include/linux/filter.h | 6 ++++-- > kernel/bpf/core.c | 4 +++- > kernel/bpf/syscall.c | 7 ++++--- > kernel/bpf/verifier.c | 4 ++-- > net/core/filter.c | 6 +++--- > 5 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index 63624c619e371b..635311f57bf24f 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -413,7 +413,8 @@ struct bpf_prog { > locked:1, /* Program image locked? */ > gpl_compatible:1, /* Is filter GPL compatible? */ > cb_access:1, /* Is control block accessed? */ > - dst_needed:1; /* Do we need dst entry? */ > + dst_needed:1, /* Do we need dst entry? */ > + priv_cap_sys_admin:1; /* Where we loaded as sys_admin? */ This is no go. You didn't provide any explanation whatsoever why you want to see this boolean value.
Hi Hannes,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Hannes-Frederic-Sowa/bpf-list-all-loaded-ebpf-programs-in-proc-bpf-programs/20170427-090839
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
lib/test_bpf.c: In function 'generate_filter':
>> lib/test_bpf.c:5613:8: error: too few arguments to function 'bpf_prog_alloc'
fp = bpf_prog_alloc(bpf_prog_size(flen), 0);
^~~~~~~~~~~~~~
In file included from lib/test_bpf.c:20:0:
include/linux/filter.h:619:18: note: declared here
struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags,
^~~~~~~~~~~~~~
vim +/bpf_prog_alloc +5613 lib/test_bpf.c
10f18e0b Daniel Borkmann 2014-05-23 5607 *err, fprog.len);
10f18e0b Daniel Borkmann 2014-05-23 5608 return NULL;
64a8946b Alexei Starovoitov 2014-05-08 5609 }
10f18e0b Daniel Borkmann 2014-05-23 5610 break;
10f18e0b Daniel Borkmann 2014-05-23 5611
10f18e0b Daniel Borkmann 2014-05-23 5612 case INTERNAL:
60a3b225 Daniel Borkmann 2014-09-02 @5613 fp = bpf_prog_alloc(bpf_prog_size(flen), 0);
10f18e0b Daniel Borkmann 2014-05-23 5614 if (fp == NULL) {
10f18e0b Daniel Borkmann 2014-05-23 5615 pr_cont("UNEXPECTED_FAIL no memory left\n");
10f18e0b Daniel Borkmann 2014-05-23 5616 *err = -ENOMEM;
:::::: The code at line 5613 was first introduced by commit
:::::: 60a3b2253c413cf601783b070507d7dd6620c954 net: bpf: make eBPF interpreter images read-only
:::::: TO: Daniel Borkmann <dborkman@redhat.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Hannes,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Hannes-Frederic-Sowa/bpf-list-all-loaded-ebpf-programs-in-proc-bpf-programs/20170427-090839
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:264:8: sparse: attribute 'no_sanitize_address': unknown attribute
lib/test_bpf.c:407:29: sparse: subtraction of functions? Share your drugs
lib/test_bpf.c:418:29: sparse: subtraction of functions? Share your drugs
>> lib/test_bpf.c:5613:36: sparse: not enough arguments for function bpf_prog_alloc
lib/test_bpf.c: In function 'generate_filter':
lib/test_bpf.c:5613:8: error: too few arguments to function 'bpf_prog_alloc'
fp = bpf_prog_alloc(bpf_prog_size(flen), 0);
^~~~~~~~~~~~~~
In file included from lib/test_bpf.c:20:0:
include/linux/filter.h:619:18: note: declared here
struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags,
^~~~~~~~~~~~~~
vim +5613 lib/test_bpf.c
10f18e0ba Daniel Borkmann 2014-05-23 5597 /* Verifier didn't reject the test that's
10f18e0ba Daniel Borkmann 2014-05-23 5598 * bad enough, just return!
10f18e0ba Daniel Borkmann 2014-05-23 5599 */
10f18e0ba Daniel Borkmann 2014-05-23 5600 *err = -EINVAL;
10f18e0ba Daniel Borkmann 2014-05-23 5601 return NULL;
10f18e0ba Daniel Borkmann 2014-05-23 5602 }
10f18e0ba Daniel Borkmann 2014-05-23 5603 }
10f18e0ba Daniel Borkmann 2014-05-23 5604 /* We don't expect to fail. */
10f18e0ba Daniel Borkmann 2014-05-23 5605 if (*err) {
10f18e0ba Daniel Borkmann 2014-05-23 5606 pr_cont("FAIL to attach err=%d len=%d\n",
10f18e0ba Daniel Borkmann 2014-05-23 5607 *err, fprog.len);
10f18e0ba Daniel Borkmann 2014-05-23 5608 return NULL;
64a8946b4 Alexei Starovoitov 2014-05-08 5609 }
10f18e0ba Daniel Borkmann 2014-05-23 5610 break;
10f18e0ba Daniel Borkmann 2014-05-23 5611
10f18e0ba Daniel Borkmann 2014-05-23 5612 case INTERNAL:
60a3b2253 Daniel Borkmann 2014-09-02 @5613 fp = bpf_prog_alloc(bpf_prog_size(flen), 0);
10f18e0ba Daniel Borkmann 2014-05-23 5614 if (fp == NULL) {
10f18e0ba Daniel Borkmann 2014-05-23 5615 pr_cont("UNEXPECTED_FAIL no memory left\n");
10f18e0ba Daniel Borkmann 2014-05-23 5616 *err = -ENOMEM;
10f18e0ba Daniel Borkmann 2014-05-23 5617 return NULL;
10f18e0ba Daniel Borkmann 2014-05-23 5618 }
10f18e0ba Daniel Borkmann 2014-05-23 5619
10f18e0ba Daniel Borkmann 2014-05-23 5620 fp->len = flen;
4962fa10f Daniel Borkmann 2015-07-30 5621 /* Type doesn't really matter here as long as it's not unspec. */
:::::: The code at line 5613 was first introduced by commit
:::::: 60a3b2253c413cf601783b070507d7dd6620c954 net: bpf: make eBPF interpreter images read-only
:::::: TO: Daniel Borkmann <dborkman@redhat.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 26.04.2017 23:04, Daniel Borkmann wrote: > On 04/26/2017 08:24 PM, Hannes Frederic Sowa wrote: >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 9a37860a80fc78..dc020d40bb770a 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -1100,7 +1100,7 @@ int bpf_prog_create(struct bpf_prog **pfp, >> struct sock_fprog_kern *fprog) >> if (!bpf_check_basics_ok(fprog->filter, fprog->len)) >> return -EINVAL; >> >> - fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0); >> + fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0, false); >> if (!fp) >> return -ENOMEM; >> > > Did you check that transferring allow_ptr_leaks doesn't have a side > effect on the nfp JIT? I believe it can also do cbpf migrations to > a certain extend. Initially I grepped allow_ptr_leaks usages and didn't see it. I just looked through the code path and didn't see how it could have an impact. Also, cbpf programs shouldn't depend on allow_ptr_leak to the best of my knowledge, no? Thanks, Hannes
Hi, On 26.04.2017 23:08, Alexei Starovoitov wrote: > On Wed, Apr 26, 2017 at 08:24:17PM +0200, Hannes Frederic Sowa wrote: >> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> >> --- >> include/linux/filter.h | 6 ++++-- >> kernel/bpf/core.c | 4 +++- >> kernel/bpf/syscall.c | 7 ++++--- >> kernel/bpf/verifier.c | 4 ++-- >> net/core/filter.c | 6 +++--- >> 5 files changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/include/linux/filter.h b/include/linux/filter.h >> index 63624c619e371b..635311f57bf24f 100644 >> --- a/include/linux/filter.h >> +++ b/include/linux/filter.h >> @@ -413,7 +413,8 @@ struct bpf_prog { >> locked:1, /* Program image locked? */ >> gpl_compatible:1, /* Is filter GPL compatible? */ >> cb_access:1, /* Is control block accessed? */ >> - dst_needed:1; /* Do we need dst entry? */ >> + dst_needed:1, /* Do we need dst entry? */ >> + priv_cap_sys_admin:1; /* Where we loaded as sys_admin? */ > > This is no go. > You didn't provide any explanation whatsoever why you want to see this boolean value. Sorry, should be in the description and will be added if this patch series is considered to be worthwhile to pursue. cap_sys_admin influences the verifier a lot in terms which programs are accepted and which are not. So during investigations it might be even interesting if the bpf program required those special flags or if the same program could be loaded just as underprivileged. I also have to review if we can/should attach cap_sys_admin verified programs as unprivileged user. It should stop to install a ptr leaking bpf program as ordinary user, even if one got the file descriptor, no? Bye, Hannes
diff --git a/include/linux/filter.h b/include/linux/filter.h index 63624c619e371b..635311f57bf24f 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -413,7 +413,8 @@ struct bpf_prog { locked:1, /* Program image locked? */ gpl_compatible:1, /* Is filter GPL compatible? */ cb_access:1, /* Is control block accessed? */ - dst_needed:1; /* Do we need dst entry? */ + dst_needed:1, /* Do we need dst entry? */ + priv_cap_sys_admin:1; /* Where we loaded as sys_admin? */ kmemcheck_bitfield_end(meta); enum bpf_prog_type type; /* Type of BPF program */ u32 len; /* Number of filter blocks */ @@ -615,7 +616,8 @@ static inline int sk_filter(struct sock *sk, struct sk_buff *skb) struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err); void bpf_prog_free(struct bpf_prog *fp); -struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags); +struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags, + bool cap_sys_admin); struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, gfp_t gfp_extra_flags); void __bpf_prog_free(struct bpf_prog *fp); diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 2139118258cdf8..048e2d79718a16 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -74,7 +74,8 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns return NULL; } -struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags) +struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags, + bool cap_sys_admin) { gfp_t gfp_flags = GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO | gfp_extra_flags; @@ -94,6 +95,7 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags) return NULL; } + fp->priv_cap_sys_admin = cap_sys_admin; fp->pages = size / PAGE_SIZE; fp->aux = aux; fp->aux->prog = fp; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index d61d1bd3e6fee6..ed698c17578a49 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -786,7 +786,7 @@ EXPORT_SYMBOL_GPL(bpf_prog_get_type); /* last field in 'union bpf_attr' used by this command */ #define BPF_PROG_LOAD_LAST_FIELD kern_version -static int bpf_prog_load(union bpf_attr *attr) +static int bpf_prog_load(union bpf_attr *attr, bool cap_sys_admin) { enum bpf_prog_type type = attr->prog_type; struct bpf_prog *prog; @@ -817,7 +817,8 @@ static int bpf_prog_load(union bpf_attr *attr) return -EPERM; /* plain bpf_prog allocation */ - prog = bpf_prog_alloc(bpf_prog_size(attr->insn_cnt), GFP_USER); + prog = bpf_prog_alloc(bpf_prog_size(attr->insn_cnt), GFP_USER, + cap_sys_admin); if (!prog) return -ENOMEM; @@ -1053,7 +1054,7 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz err = map_get_next_key(&attr); break; case BPF_PROG_LOAD: - err = bpf_prog_load(&attr); + err = bpf_prog_load(&attr, capable(CAP_SYS_ADMIN)); break; case BPF_OBJ_PIN: err = bpf_obj_pin(&attr); diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6f8b6ed690be93..24c9dac374770f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3488,7 +3488,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr) if (ret < 0) goto skip_full_check; - env->allow_ptr_leaks = capable(CAP_SYS_ADMIN); + env->allow_ptr_leaks = env->prog->priv_cap_sys_admin; ret = do_check(env); @@ -3589,7 +3589,7 @@ int bpf_analyzer(struct bpf_prog *prog, const struct bpf_ext_analyzer_ops *ops, if (ret < 0) goto skip_full_check; - env->allow_ptr_leaks = capable(CAP_SYS_ADMIN); + env->allow_ptr_leaks = prog->priv_cap_sys_admin; ret = do_check(env); diff --git a/net/core/filter.c b/net/core/filter.c index 9a37860a80fc78..dc020d40bb770a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1100,7 +1100,7 @@ int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog) if (!bpf_check_basics_ok(fprog->filter, fprog->len)) return -EINVAL; - fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0); + fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0, false); if (!fp) return -ENOMEM; @@ -1147,7 +1147,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog, if (!bpf_check_basics_ok(fprog->filter, fprog->len)) return -EINVAL; - fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0); + fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0, false); if (!fp) return -ENOMEM; @@ -1249,7 +1249,7 @@ struct bpf_prog *__get_filter(struct sock_fprog *fprog, struct sock *sk) if (!bpf_check_basics_ok(fprog->filter, fprog->len)) return ERR_PTR(-EINVAL); - prog = bpf_prog_alloc(bpf_prog_size(fprog->len), 0); + prog = bpf_prog_alloc(bpf_prog_size(fprog->len), 0, false); if (!prog) return ERR_PTR(-ENOMEM);
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- include/linux/filter.h | 6 ++++-- kernel/bpf/core.c | 4 +++- kernel/bpf/syscall.c | 7 ++++--- kernel/bpf/verifier.c | 4 ++-- net/core/filter.c | 6 +++--- 5 files changed, 16 insertions(+), 11 deletions(-)