| Message ID | 20230412043300.360803-7-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | New BPF map and BTF security LSM hooks | expand |
On Tue, Apr 11, 2023 at 09:32:58PM -0700, Andrii Nakryiko wrote: > Seems like that extra bpf_capable() check in BPF_MAP_FREEZE handler was > unintentionally left when we switched to a model that all BPF map > operations should be allowed regardless of CAP_BPF (or any other > capabilities), as long as process got BPF map FD somehow. > > This patch replaces bpf_capable() check in BPF_MAP_FREEZE handler with > writeable access check, given conceptually freezing the map is modifying > it: map becomes unmodifiable for subsequent updates. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Is this patch stand-alone? It seems like this could be taken separately, or at least just be the first patch in the series? > --- > kernel/bpf/syscall.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 7d1165814efc..42d8473237ab 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2001,6 +2001,11 @@ static int map_freeze(const union bpf_attr *attr) > return -ENOTSUPP; > } > > + if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) { > + err = -EPERM; > + goto err_put; > + } > + > mutex_lock(&map->freeze_mutex); > if (bpf_map_write_active(map)) { > err = -EBUSY; > @@ -2010,10 +2015,6 @@ static int map_freeze(const union bpf_attr *attr) > err = -EBUSY; > goto err_put; > } > - if (!bpf_capable()) { > - err = -EPERM; > - goto err_put; > - } > > WRITE_ONCE(map->frozen, true); > err_put: > -- > 2.34.1 >
On Wed, Apr 12, 2023 at 11:24 AM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Apr 11, 2023 at 09:32:58PM -0700, Andrii Nakryiko wrote: > > Seems like that extra bpf_capable() check in BPF_MAP_FREEZE handler was > > unintentionally left when we switched to a model that all BPF map > > operations should be allowed regardless of CAP_BPF (or any other > > capabilities), as long as process got BPF map FD somehow. > > > > This patch replaces bpf_capable() check in BPF_MAP_FREEZE handler with > > writeable access check, given conceptually freezing the map is modifying > > it: map becomes unmodifiable for subsequent updates. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > Is this patch stand-alone? It seems like this could be taken separately, > or at least just be the first patch in the series? > yep, I'll send it separately, good point > > --- > > kernel/bpf/syscall.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 7d1165814efc..42d8473237ab 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -2001,6 +2001,11 @@ static int map_freeze(const union bpf_attr *attr) > > return -ENOTSUPP; > > } > > > > + if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) { > > + err = -EPERM; > > + goto err_put; > > + } > > + > > mutex_lock(&map->freeze_mutex); > > if (bpf_map_write_active(map)) { > > err = -EBUSY; > > @@ -2010,10 +2015,6 @@ static int map_freeze(const union bpf_attr *attr) > > err = -EBUSY; > > goto err_put; > > } > > - if (!bpf_capable()) { > > - err = -EPERM; > > - goto err_put; > > - } > > > > WRITE_ONCE(map->frozen, true); > > err_put: > > -- > > 2.34.1 > > > > -- > Kees Cook
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 7d1165814efc..42d8473237ab 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2001,6 +2001,11 @@ static int map_freeze(const union bpf_attr *attr) return -ENOTSUPP; } + if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) { + err = -EPERM; + goto err_put; + } + mutex_lock(&map->freeze_mutex); if (bpf_map_write_active(map)) { err = -EBUSY; @@ -2010,10 +2015,6 @@ static int map_freeze(const union bpf_attr *attr) err = -EBUSY; goto err_put; } - if (!bpf_capable()) { - err = -EPERM; - goto err_put; - } WRITE_ONCE(map->frozen, true); err_put:
Seems like that extra bpf_capable() check in BPF_MAP_FREEZE handler was unintentionally left when we switched to a model that all BPF map operations should be allowed regardless of CAP_BPF (or any other capabilities), as long as process got BPF map FD somehow. This patch replaces bpf_capable() check in BPF_MAP_FREEZE handler with writeable access check, given conceptually freezing the map is modifying it: map becomes unmodifiable for subsequent updates. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/bpf/syscall.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)