[2/3] security: bpf: Add eBPF LSM hooks and security field to eBPF map
diff mbox

Message ID 20170831205635.80256-3-chenbofeng.kernel@gmail.com
State New
Headers show

Commit Message

Chenbo Feng Aug. 31, 2017, 8:56 p.m. UTC
From: Chenbo Feng <fengc@google.com>

Introduce a pointer into struct bpf_map to hold the security information
about the map. The actual security struct varies based on the security
models implemented. Place the LSM hooks before each of the unrestricted
eBPF operations, the map_update_elem and map_delete_elem operations are
checked by security_map_modify. The map_lookup_elem and map_get_next_key
operations are checked by securtiy_map_read.

Signed-off-by: Chenbo Feng <fengc@google.com>
---
 include/linux/bpf.h  |  3 +++
 kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

Mimi Zohar Aug. 31, 2017, 9:17 p.m. UTC | #1
On Thu, 2017-08-31 at 13:56 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> Introduce a pointer into struct bpf_map to hold the security information
> about the map. The actual security struct varies based on the security
> models implemented. Place the LSM hooks before each of the unrestricted
> eBPF operations, the map_update_elem and map_delete_elem operations are
> checked by security_map_modify. The map_lookup_elem and map_get_next_key
> operations are checked by securtiy_map_read.
> 
> Signed-off-by: Chenbo Feng <fengc@google.com>
> ---
>  include/linux/bpf.h  |  3 +++
>  kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b69e7a5869ff..ca3e6ff7091d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -53,6 +53,9 @@ struct bpf_map {
>  	struct work_struct work;
>  	atomic_t usercnt;
>  	struct bpf_map *inner_map_meta;
> +#ifdef CONFIG_SECURITY
> +	void *security;
> +#endif
>  };
>  
>  /* function argument constraints */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 045646da97cc..b15580bcf3b1 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -279,6 +279,10 @@ static int map_create(union bpf_attr *attr)
>  	if (err)
>  		return -EINVAL;
>  
> +	err = security_map_create();
> +	if (err)
> +		return -EACCES;

Any reason not to just return err?

Mimi

> +
>  	/* find map type and init map: hashtable vs rbtree vs bloom vs ... */
>  	map = find_and_alloc_map(attr);
>  	if (IS_ERR(map))
> @@ -291,6 +295,10 @@ static int map_create(union bpf_attr *attr)
>  	if (err)
>  		goto free_map_nouncharge;
>  
> +	err = security_post_create(map);
> +	if (err < 0)
> +		goto free_map;
> +
>  	err = bpf_map_alloc_id(map);
>  	if (err)
>  		goto free_map;
> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
>  
> +	err = security_map_read(map);
> +	if (err)
> +		return -EACCES;
> +
>  	key = memdup_user(ukey, map->key_size);
>  	if (IS_ERR(key)) {
>  		err = PTR_ERR(key);
> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
>  
> +	err = security_map_modify(map);
> +	if (err)
> +		return -EACCES;
> +
>  	key = memdup_user(ukey, map->key_size);
>  	if (IS_ERR(key)) {
>  		err = PTR_ERR(key);
> @@ -573,6 +589,10 @@ static int map_delete_elem(union bpf_attr *attr)
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
>  
> +	err = security_map_modify(map);
> +	if (err)
> +		return -EACCES;
> +
>  	key = memdup_user(ukey, map->key_size);
>  	if (IS_ERR(key)) {
>  		err = PTR_ERR(key);
> @@ -616,6 +636,10 @@ static int map_get_next_key(union bpf_attr *attr)
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
>  
> +	err = security_map_read(map);
> +	if (err)
> +		return -EACCES;
> +
>  	if (ukey) {
>  		key = memdup_user(ukey, map->key_size);
>  		if (IS_ERR(key)) {
> @@ -935,6 +959,10 @@ static int bpf_prog_load(union bpf_attr *attr)
>  	if (CHECK_ATTR(BPF_PROG_LOAD))
>  		return -EINVAL;
>  
> +	err = security_prog_load();
> +	if (err)
> +		return -EACCES;
> +
>  	if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT)
>  		return -EINVAL;
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chenbo Feng Aug. 31, 2017, 10:17 p.m. UTC | #2
On Thu, Aug 31, 2017 at 2:17 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Thu, 2017-08-31 at 13:56 -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> Introduce a pointer into struct bpf_map to hold the security information
>> about the map. The actual security struct varies based on the security
>> models implemented. Place the LSM hooks before each of the unrestricted
>> eBPF operations, the map_update_elem and map_delete_elem operations are
>> checked by security_map_modify. The map_lookup_elem and map_get_next_key
>> operations are checked by securtiy_map_read.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>> ---
>>  include/linux/bpf.h  |  3 +++
>>  kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index b69e7a5869ff..ca3e6ff7091d 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -53,6 +53,9 @@ struct bpf_map {
>>       struct work_struct work;
>>       atomic_t usercnt;
>>       struct bpf_map *inner_map_meta;
>> +#ifdef CONFIG_SECURITY
>> +     void *security;
>> +#endif
>>  };
>>
>>  /* function argument constraints */
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 045646da97cc..b15580bcf3b1 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -279,6 +279,10 @@ static int map_create(union bpf_attr *attr)
>>       if (err)
>>               return -EINVAL;
>>
>> +     err = security_map_create();
>> +     if (err)
>> +             return -EACCES;
>
> Any reason not to just return err?
>
> Mimi
>
Nope... return err might be better. I will fix this in next version.

Thanks
Chenbo
>> +
>>       /* find map type and init map: hashtable vs rbtree vs bloom vs ... */
>>       map = find_and_alloc_map(attr);
>>       if (IS_ERR(map))
>> @@ -291,6 +295,10 @@ static int map_create(union bpf_attr *attr)
>>       if (err)
>>               goto free_map_nouncharge;
>>
>> +     err = security_post_create(map);
>> +     if (err < 0)
>> +             goto free_map;
>> +
>>       err = bpf_map_alloc_id(map);
>>       if (err)
>>               goto free_map;
>> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_read(map);
>> +     if (err)
>> +             return -EACCES;
>> +
>>       key = memdup_user(ukey, map->key_size);
>>       if (IS_ERR(key)) {
>>               err = PTR_ERR(key);
>> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_modify(map);
>> +     if (err)
>> +             return -EACCES;
>> +
>>       key = memdup_user(ukey, map->key_size);
>>       if (IS_ERR(key)) {
>>               err = PTR_ERR(key);
>> @@ -573,6 +589,10 @@ static int map_delete_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_modify(map);
>> +     if (err)
>> +             return -EACCES;
>> +
>>       key = memdup_user(ukey, map->key_size);
>>       if (IS_ERR(key)) {
>>               err = PTR_ERR(key);
>> @@ -616,6 +636,10 @@ static int map_get_next_key(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_read(map);
>> +     if (err)
>> +             return -EACCES;
>> +
>>       if (ukey) {
>>               key = memdup_user(ukey, map->key_size);
>>               if (IS_ERR(key)) {
>> @@ -935,6 +959,10 @@ static int bpf_prog_load(union bpf_attr *attr)
>>       if (CHECK_ATTR(BPF_PROG_LOAD))
>>               return -EINVAL;
>>
>> +     err = security_prog_load();
>> +     if (err)
>> +             return -EACCES;
>> +
>>       if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT)
>>               return -EINVAL;
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann Aug. 31, 2017, 10:38 p.m. UTC | #3
On 08/31/2017 10:56 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> Introduce a pointer into struct bpf_map to hold the security information
> about the map. The actual security struct varies based on the security
> models implemented. Place the LSM hooks before each of the unrestricted
> eBPF operations, the map_update_elem and map_delete_elem operations are
> checked by security_map_modify. The map_lookup_elem and map_get_next_key
> operations are checked by securtiy_map_read.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>

Against which tree is this by the way, net-next? There are
changes here which require a rebase of your set.

> ---
>   include/linux/bpf.h  |  3 +++
>   kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b69e7a5869ff..ca3e6ff7091d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -53,6 +53,9 @@ struct bpf_map {
>   	struct work_struct work;
>   	atomic_t usercnt;
>   	struct bpf_map *inner_map_meta;
> +#ifdef CONFIG_SECURITY
> +	void *security;
> +#endif
>   };
>
>   /* function argument constraints */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 045646da97cc..b15580bcf3b1 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -279,6 +279,10 @@ static int map_create(union bpf_attr *attr)
>   	if (err)
>   		return -EINVAL;
>
> +	err = security_map_create();

Seems a bit limited to me, don't you want to be able to
also differentiate between different map types? Same goes
for security_prog_load() wrt prog types, no?

> +	if (err)
> +		return -EACCES;
> +
>   	/* find map type and init map: hashtable vs rbtree vs bloom vs ... */
>   	map = find_and_alloc_map(attr);
>   	if (IS_ERR(map))
> @@ -291,6 +295,10 @@ static int map_create(union bpf_attr *attr)
>   	if (err)
>   		goto free_map_nouncharge;
>
> +	err = security_post_create(map);
> +	if (err < 0)
> +		goto free_map;
> +

So the hook you implement in patch 3/3 does:

+static int selinux_bpf_post_create(struct bpf_map *map)
+{
+	struct bpf_security_struct *bpfsec;
+
+	bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
+	if (!bpfsec)
+		return -ENOMEM;
+
+	bpfsec->sid = current_sid();
+	map->security = bpfsec;
+
+	return 0;
+}

Where do you kfree() bpfsec when the map gets released
normally or in error path?

>   	err = bpf_map_alloc_id(map);
>   	if (err)
>   		goto free_map;
> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>   	if (IS_ERR(map))
>   		return PTR_ERR(map);
>
> +	err = security_map_read(map);
> +	if (err)
> +		return -EACCES;

How about actually dropping ref?

> +
>   	key = memdup_user(ukey, map->key_size);
>   	if (IS_ERR(key)) {
>   		err = PTR_ERR(key);
> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>   	if (IS_ERR(map))
>   		return PTR_ERR(map);
>
> +	err = security_map_modify(map);
> +	if (err)
> +		return -EACCES;

Ditto ...

>   	key = memdup_user(ukey, map->key_size);
>   	if (IS_ERR(key)) {
>   		err = PTR_ERR(key);
> @@ -573,6 +589,10 @@ static int map_delete_elem(union bpf_attr *attr)
>   	if (IS_ERR(map))
>   		return PTR_ERR(map);
>
> +	err = security_map_modify(map);
> +	if (err)
> +		return -EACCES;

Ditto ...

>   	key = memdup_user(ukey, map->key_size);
>   	if (IS_ERR(key)) {
>   		err = PTR_ERR(key);
> @@ -616,6 +636,10 @@ static int map_get_next_key(union bpf_attr *attr)
>   	if (IS_ERR(map))
>   		return PTR_ERR(map);
>
> +	err = security_map_read(map);
> +	if (err)
> +		return -EACCES;

And once again here ... :(

>   	if (ukey) {
>   		key = memdup_user(ukey, map->key_size);
>   		if (IS_ERR(key)) {
> @@ -935,6 +959,10 @@ static int bpf_prog_load(union bpf_attr *attr)
>   	if (CHECK_ATTR(BPF_PROG_LOAD))
>   		return -EINVAL;
>
> +	err = security_prog_load();
> +	if (err)
> +		return -EACCES;
> +
>   	if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT)
>   		return -EINVAL;
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chenbo Feng Sept. 1, 2017, 12:29 a.m. UTC | #4
On Thu, Aug 31, 2017 at 3:38 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 08/31/2017 10:56 PM, Chenbo Feng wrote:
>>
>> From: Chenbo Feng <fengc@google.com>
>>
>> Introduce a pointer into struct bpf_map to hold the security information
>> about the map. The actual security struct varies based on the security
>> models implemented. Place the LSM hooks before each of the unrestricted
>> eBPF operations, the map_update_elem and map_delete_elem operations are
>> checked by security_map_modify. The map_lookup_elem and map_get_next_key
>> operations are checked by securtiy_map_read.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>
>
> Against which tree is this by the way, net-next? There are
> changes here which require a rebase of your set.
>

This patch series is rebased on security subsystem since patch 1/3 is
a patch for
security system I assume. But I am not sure where this specific patch
should go in.
If I should submit this one to net-next, I will rebase it against
net-next and submit again.
>> ---
>>   include/linux/bpf.h  |  3 +++
>>   kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++++++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index b69e7a5869ff..ca3e6ff7091d 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -53,6 +53,9 @@ struct bpf_map {
>>         struct work_struct work;
>>         atomic_t usercnt;
>>         struct bpf_map *inner_map_meta;
>> +#ifdef CONFIG_SECURITY
>> +       void *security;
>> +#endif
>>   };
>>
>>   /* function argument constraints */
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 045646da97cc..b15580bcf3b1 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -279,6 +279,10 @@ static int map_create(union bpf_attr *attr)
>>         if (err)
>>                 return -EINVAL;
>>
>> +       err = security_map_create();
>
>
> Seems a bit limited to me, don't you want to be able to
> also differentiate between different map types? Same goes
> for security_prog_load() wrt prog types, no?
>
I don't want to introduce extra complexity into it if not necessary.
so I only included the
thing needed for the selinux implementation for now.  But I agree that the map
and program type information could be useful when people developing more complex
security checks. I will add a union bpf_attr *attr pointer into the
security hook functions
for future needs.
>> +       if (err)
>> +               return -EACCES;
>> +
>>         /* find map type and init map: hashtable vs rbtree vs bloom vs ...
>> */
>>         map = find_and_alloc_map(attr);
>>         if (IS_ERR(map))
>> @@ -291,6 +295,10 @@ static int map_create(union bpf_attr *attr)
>>         if (err)
>>                 goto free_map_nouncharge;
>>
>> +       err = security_post_create(map);
>> +       if (err < 0)
>> +               goto free_map;
>> +
>
>
> So the hook you implement in patch 3/3 does:
>
> +static int selinux_bpf_post_create(struct bpf_map *map)
> +{
> +       struct bpf_security_struct *bpfsec;
> +
> +       bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> +       if (!bpfsec)
> +               return -ENOMEM;
> +
> +       bpfsec->sid = current_sid();
> +       map->security = bpfsec;
> +
> +       return 0;
> +}
>
> Where do you kfree() bpfsec when the map gets released
> normally or in error path?
>
Will add it in next version. Thanks for point it out.
>>         err = bpf_map_alloc_id(map);
>>         if (err)
>>                 goto free_map;
>> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>>         if (IS_ERR(map))
>>                 return PTR_ERR(map);
>>
>> +       err = security_map_read(map);
>> +       if (err)
>> +               return -EACCES;
>
>
> How about actually dropping ref?
>
May bad, thanks again.
>> +
>>         key = memdup_user(ukey, map->key_size);
>>         if (IS_ERR(key)) {
>>                 err = PTR_ERR(key);
>> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>>         if (IS_ERR(map))
>>                 return PTR_ERR(map);
>>
>> +       err = security_map_modify(map);
>> +       if (err)
>> +               return -EACCES;
>
>
> Ditto ...
>
>>         key = memdup_user(ukey, map->key_size);
>>         if (IS_ERR(key)) {
>>                 err = PTR_ERR(key);
>> @@ -573,6 +589,10 @@ static int map_delete_elem(union bpf_attr *attr)
>>         if (IS_ERR(map))
>>                 return PTR_ERR(map);
>>
>> +       err = security_map_modify(map);
>> +       if (err)
>> +               return -EACCES;
>
>
> Ditto ...
>
>>         key = memdup_user(ukey, map->key_size);
>>         if (IS_ERR(key)) {
>>                 err = PTR_ERR(key);
>> @@ -616,6 +636,10 @@ static int map_get_next_key(union bpf_attr *attr)
>>         if (IS_ERR(map))
>>                 return PTR_ERR(map);
>>
>> +       err = security_map_read(map);
>> +       if (err)
>> +               return -EACCES;
>
>
> And once again here ... :(
>
>
>>         if (ukey) {
>>                 key = memdup_user(ukey, map->key_size);
>>                 if (IS_ERR(key)) {
>> @@ -935,6 +959,10 @@ static int bpf_prog_load(union bpf_attr *attr)
>>         if (CHECK_ATTR(BPF_PROG_LOAD))
>>                 return -EINVAL;
>>
>> +       err = security_prog_load();
>> +       if (err)
>> +               return -EACCES;
>> +
>>         if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT)
>>                 return -EINVAL;
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexei Starovoitov Sept. 1, 2017, 2:05 a.m. UTC | #5
On Thu, Aug 31, 2017 at 01:56:34PM -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> Introduce a pointer into struct bpf_map to hold the security information
> about the map. The actual security struct varies based on the security
> models implemented. Place the LSM hooks before each of the unrestricted
> eBPF operations, the map_update_elem and map_delete_elem operations are
> checked by security_map_modify. The map_lookup_elem and map_get_next_key
> operations are checked by securtiy_map_read.
> 
> Signed-off-by: Chenbo Feng <fengc@google.com>

...

> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
>  
> +	err = security_map_read(map);
> +	if (err)
> +		return -EACCES;
> +
>  	key = memdup_user(ukey, map->key_size);
>  	if (IS_ERR(key)) {
>  		err = PTR_ERR(key);
> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
>  
> +	err = security_map_modify(map);

I don't feel these extra hooks are really thought through.
With such hook you'll disallow map_update for given map. That's it.
The key/values etc won't be used in such security decision.
In such case you don't need such hooks in update/lookup at all.
Only in map_creation and object_get calls where FD can be received.
In other words I suggest to follow standard unix practices:
Do permissions checks in open() and allow read/write() if FD is valid.
Same here. Do permission checks in prog_load/map_create/obj_pin/get
and that will be enough to jail bpf subsystem.
bpf cmds that need to be fast (like lookup and update) should not
have security hooks.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeffrey Vander Stoep Sept. 1, 2017, 5:50 a.m. UTC | #6
On Thu, Aug 31, 2017 at 7:05 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Aug 31, 2017 at 01:56:34PM -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> Introduce a pointer into struct bpf_map to hold the security information
>> about the map. The actual security struct varies based on the security
>> models implemented. Place the LSM hooks before each of the unrestricted
>> eBPF operations, the map_update_elem and map_delete_elem operations are
>> checked by security_map_modify. The map_lookup_elem and map_get_next_key
>> operations are checked by securtiy_map_read.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>
> ...
>
>> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_read(map);
>> +     if (err)
>> +             return -EACCES;
>> +
>>       key = memdup_user(ukey, map->key_size);
>>       if (IS_ERR(key)) {
>>               err = PTR_ERR(key);
>> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_modify(map);
>
> I don't feel these extra hooks are really thought through.
> With such hook you'll disallow map_update for given map. That's it.
> The key/values etc won't be used in such security decision.
> In such case you don't need such hooks in update/lookup at all.
> Only in map_creation and object_get calls where FD can be received.
> In other words I suggest to follow standard unix practices:
> Do permissions checks in open() and allow read/write() if FD is valid.
> Same here. Do permission checks in prog_load/map_create/obj_pin/get
> and that will be enough to jail bpf subsystem.
> bpf cmds that need to be fast (like lookup and update) should not
> have security hooks.
>

I do think we want to distinguish between read/write (or read/modify)
for these objects. Essentially, we want to implement the example
described in patch 0/3 where eBPF objects can be passed to less
privileged processes which can read, but not modify the map. What
would be the best way to do this? Add a mode field to the bpf_map
struct?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chenbo Feng Sept. 5, 2017, 9:59 p.m. UTC | #7
On Thu, Aug 31, 2017 at 7:05 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Aug 31, 2017 at 01:56:34PM -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> Introduce a pointer into struct bpf_map to hold the security information
>> about the map. The actual security struct varies based on the security
>> models implemented. Place the LSM hooks before each of the unrestricted
>> eBPF operations, the map_update_elem and map_delete_elem operations are
>> checked by security_map_modify. The map_lookup_elem and map_get_next_key
>> operations are checked by securtiy_map_read.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>
> ...
>
>> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_read(map);
>> +     if (err)
>> +             return -EACCES;
>> +
>>       key = memdup_user(ukey, map->key_size);
>>       if (IS_ERR(key)) {
>>               err = PTR_ERR(key);
>> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>>       if (IS_ERR(map))
>>               return PTR_ERR(map);
>>
>> +     err = security_map_modify(map);
>
> I don't feel these extra hooks are really thought through.
> With such hook you'll disallow map_update for given map. That's it.
> The key/values etc won't be used in such security decision.
> In such case you don't need such hooks in update/lookup at all.
> Only in map_creation and object_get calls where FD can be received.
> In other words I suggest to follow standard unix practices:
> Do permissions checks in open() and allow read/write() if FD is valid.
> Same here. Do permission checks in prog_load/map_create/obj_pin/get
> and that will be enough to jail bpf subsystem.
> bpf cmds that need to be fast (like lookup and update) should not
> have security hooks.
>
Thanks for pointing out this. I agree we should only do checks on
creating and passing
the object from one processes to another. And if we can still
distinguish the read/write operation
when obtaining the file fd, that would be great. But that may require
us to add a new mode
field into bpf_map struct and change the attribute struct when doing
the bpf syscall. How do you
think about this approach? Or we can do simple checks for
bpf_obj_create and bpf_obj_use when
creating the object and passing the object respectively but this
solution cannot distinguish map modify and
read.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexei Starovoitov Sept. 6, 2017, 12:39 a.m. UTC | #8
On Tue, Sep 05, 2017 at 02:59:38PM -0700, Chenbo Feng wrote:
> On Thu, Aug 31, 2017 at 7:05 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Thu, Aug 31, 2017 at 01:56:34PM -0700, Chenbo Feng wrote:
> >> From: Chenbo Feng <fengc@google.com>
> >>
> >> Introduce a pointer into struct bpf_map to hold the security information
> >> about the map. The actual security struct varies based on the security
> >> models implemented. Place the LSM hooks before each of the unrestricted
> >> eBPF operations, the map_update_elem and map_delete_elem operations are
> >> checked by security_map_modify. The map_lookup_elem and map_get_next_key
> >> operations are checked by securtiy_map_read.
> >>
> >> Signed-off-by: Chenbo Feng <fengc@google.com>
> >
> > ...
> >
> >> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
> >>       if (IS_ERR(map))
> >>               return PTR_ERR(map);
> >>
> >> +     err = security_map_read(map);
> >> +     if (err)
> >> +             return -EACCES;
> >> +
> >>       key = memdup_user(ukey, map->key_size);
> >>       if (IS_ERR(key)) {
> >>               err = PTR_ERR(key);
> >> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
> >>       if (IS_ERR(map))
> >>               return PTR_ERR(map);
> >>
> >> +     err = security_map_modify(map);
> >
> > I don't feel these extra hooks are really thought through.
> > With such hook you'll disallow map_update for given map. That's it.
> > The key/values etc won't be used in such security decision.
> > In such case you don't need such hooks in update/lookup at all.
> > Only in map_creation and object_get calls where FD can be received.
> > In other words I suggest to follow standard unix practices:
> > Do permissions checks in open() and allow read/write() if FD is valid.
> > Same here. Do permission checks in prog_load/map_create/obj_pin/get
> > and that will be enough to jail bpf subsystem.
> > bpf cmds that need to be fast (like lookup and update) should not
> > have security hooks.
> >
> Thanks for pointing out this. I agree we should only do checks on
> creating and passing
> the object from one processes to another. And if we can still
> distinguish the read/write operation
> when obtaining the file fd, that would be great. But that may require
> us to add a new mode
> field into bpf_map struct and change the attribute struct when doing
> the bpf syscall. How do you
> think about this approach? Or we can do simple checks for
> bpf_obj_create and bpf_obj_use when
> creating the object and passing the object respectively but this
> solution cannot distinguish map modify and
> read.

iirc the idea to have read only maps was brought up in the past
(unfortunately no one took a stab at implementing it), but imo
that's better then relying on lsm to provide such restriction
and more secure, since even if you disable map_update via lsm,
the user can craft a program just to udpate the map from it.
For bpffs we already test for inode_permission(inode, MAY_WRITE);
during BPF_OBJ_GET command and we can extend this facility further.
Also we can allow dropping 'write' permissions from the map
(internally implemented via flag in struct bpf_map), so
update/delete operations won't be allowed.
This flag will be checked by syscall during map_update/delete
and by the verifier if such read-only map is used by the program
being loaded, so map_update/helpers won't be allowed in
such program.
Would also be good to support read-only programs as well
(progs that are read-only from kernel state point of view)
They won't be able to modify packets, maps, etc.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b69e7a5869ff..ca3e6ff7091d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -53,6 +53,9 @@  struct bpf_map {
 	struct work_struct work;
 	atomic_t usercnt;
 	struct bpf_map *inner_map_meta;
+#ifdef CONFIG_SECURITY
+	void *security;
+#endif
 };
 
 /* function argument constraints */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 045646da97cc..b15580bcf3b1 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -279,6 +279,10 @@  static int map_create(union bpf_attr *attr)
 	if (err)
 		return -EINVAL;
 
+	err = security_map_create();
+	if (err)
+		return -EACCES;
+
 	/* find map type and init map: hashtable vs rbtree vs bloom vs ... */
 	map = find_and_alloc_map(attr);
 	if (IS_ERR(map))
@@ -291,6 +295,10 @@  static int map_create(union bpf_attr *attr)
 	if (err)
 		goto free_map_nouncharge;
 
+	err = security_post_create(map);
+	if (err < 0)
+		goto free_map;
+
 	err = bpf_map_alloc_id(map);
 	if (err)
 		goto free_map;
@@ -410,6 +418,10 @@  static int map_lookup_elem(union bpf_attr *attr)
 	if (IS_ERR(map))
 		return PTR_ERR(map);
 
+	err = security_map_read(map);
+	if (err)
+		return -EACCES;
+
 	key = memdup_user(ukey, map->key_size);
 	if (IS_ERR(key)) {
 		err = PTR_ERR(key);
@@ -490,6 +502,10 @@  static int map_update_elem(union bpf_attr *attr)
 	if (IS_ERR(map))
 		return PTR_ERR(map);
 
+	err = security_map_modify(map);
+	if (err)
+		return -EACCES;
+
 	key = memdup_user(ukey, map->key_size);
 	if (IS_ERR(key)) {
 		err = PTR_ERR(key);
@@ -573,6 +589,10 @@  static int map_delete_elem(union bpf_attr *attr)
 	if (IS_ERR(map))
 		return PTR_ERR(map);
 
+	err = security_map_modify(map);
+	if (err)
+		return -EACCES;
+
 	key = memdup_user(ukey, map->key_size);
 	if (IS_ERR(key)) {
 		err = PTR_ERR(key);
@@ -616,6 +636,10 @@  static int map_get_next_key(union bpf_attr *attr)
 	if (IS_ERR(map))
 		return PTR_ERR(map);
 
+	err = security_map_read(map);
+	if (err)
+		return -EACCES;
+
 	if (ukey) {
 		key = memdup_user(ukey, map->key_size);
 		if (IS_ERR(key)) {
@@ -935,6 +959,10 @@  static int bpf_prog_load(union bpf_attr *attr)
 	if (CHECK_ATTR(BPF_PROG_LOAD))
 		return -EINVAL;
 
+	err = security_prog_load();
+	if (err)
+		return -EACCES;
+
 	if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT)
 		return -EINVAL;