[PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Hannes Frederic Sowa]

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? */
 	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);
 
-- 
2.9.3

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Daniel Borkmann]

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.

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Alexei Starovoitov]

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.

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38891 bytes --]

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[kbuild test robot]

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

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Hannes Frederic Sowa]

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

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Hannes Frederic Sowa]

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

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Alexei Starovoitov]

On Thu, Apr 27, 2017 at 6:17 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
>
> 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.

there are bunch of other things that influence verifier.
should we add all of them ?
there is prog->aux->user which is more than enough for
debugging.

Re: [PATCH net-next 4/6] bpf: track if the bpf program was loaded with SYS_ADMIN capabilities

[Jiri Benc]

On Thu, 27 Apr 2017 07:49:29 -0700, Alexei Starovoitov wrote:
> there are bunch of other things that influence verifier.
> should we add all of them ?

Sure, why not? It's always good to be able to see the complete input
conditions while debugging.

 Jiri