| Message ID | 20170831205635.80256-3-chenbofeng.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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;