[PATCH bpf-next 0/3] Relax checks for unprivileged bpf() commands

[PATCH bpf-next 0/3] Relax checks for unprivileged bpf() commands

[Andrii Nakryiko]

During last relaxation of bpf syscall's capabilities checks ([0]), the model
of FD-based ownership was established: if process through whatever means got
FD for some BPF object (map, prog, etc), it should be able to perform
operations on this object without extra CAP_SYS_ADMIN or CAP_BPF capabilities.

It seems like we missed a few cases, though, in which we are still enforcing extra caps for no good reason, even though operations are not really unsafe and/or do not require any system-wide capabilities:
  - BPF_MAP_FREEZE command;
  - GET_NEXT_ID family of commands;
  - GET_INFO_BY_FD command has extra bpf_capable()-based sanitization.

This patch set is removing these unnecessary checks. MAP_FREEZE and INFO_BY_FD
are deviating from the "if you own FD, you can work with BPF object".

GET_NEXT_ID is a completely safe and unprivileged operation that returns just
IDs of BPF objects. One still needs to go through CAP_SYS_ADMIN-guarded
GET_FD_BY_ID command to get ahold of FD to do any operation on corresponding
BPF object. As such, it seems completely safe to drop CAP_SYS_ADMIN checks for
GET_NEXT_ID.

  [0] https://lore.kernel.org/all/1652970334-30510-2-git-send-email-alan.maguire@oracle.com/

Andrii Nakryiko (3):
  bpf: drop unnecessary bpf_capable() check in BPF_MAP_FREEZE command
  bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID
  bpf: don't require bpf_capable() for GET_INFO_BY_FD

 kernel/bpf/syscall.c                          | 23 ++++---------------
 .../bpf/prog_tests/unpriv_bpf_disabled.c      | 15 ++----------
 2 files changed, 7 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH bpf-next 1/3] bpf: drop unnecessary bpf_capable() check in BPF_MAP_FREEZE command

[Andrii Nakryiko]

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(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c7f6807215e6..c9a201e4c457 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1931,6 +1931,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;
@@ -1940,10 +1945,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

[PATCH bpf-next 2/3] bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID

[Andrii Nakryiko]

Getting ID of map/prog/btf/link doesn't give any access to underlying
BPF objects, so there is no point in requiring CAP_SYS_ADMIN for these
commands.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/syscall.c                              |  3 ---
 .../bpf/prog_tests/unpriv_bpf_disabled.c          | 15 ++-------------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c9a201e4c457..1d74c0a8d903 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3720,9 +3720,6 @@ static int bpf_obj_get_next_id(const union bpf_attr *attr,
 	if (CHECK_ATTR(BPF_OBJ_GET_NEXT_ID) || next_id >= INT_MAX)
 		return -EINVAL;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	next_id++;
 	spin_lock_bh(lock);
 	if (!idr_get_next(idr, &next_id))
diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
index 8383a99f610f..154a957154e8 100644
--- a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
+++ b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
@@ -156,7 +156,6 @@ static void test_unpriv_bpf_disabled_negative(struct test_unpriv_bpf_disabled *s
 	__u32 attach_flags = 0;
 	__u32 prog_ids[3] = {};
 	__u32 prog_cnt = 3;
-	__u32 next;
 	int i;
 
 	/* Negative tests for unprivileged BPF disabled.  Verify we cannot
@@ -176,25 +175,15 @@ static void test_unpriv_bpf_disabled_negative(struct test_unpriv_bpf_disabled *s
 			  -EPERM, "map_create_fails");
 
 	ASSERT_EQ(bpf_prog_get_fd_by_id(prog_id), -EPERM, "prog_get_fd_by_id_fails");
-	ASSERT_EQ(bpf_prog_get_next_id(prog_id, &next), -EPERM, "prog_get_next_id_fails");
-	ASSERT_EQ(bpf_prog_get_next_id(0, &next), -EPERM, "prog_get_next_id_fails");
 
 	if (ASSERT_OK(bpf_map_get_info_by_fd(map_fds[0], &map_info, &map_info_len),
-		      "obj_get_info_by_fd")) {
+		      "obj_get_info_by_fd"))
 		ASSERT_EQ(bpf_map_get_fd_by_id(map_info.id), -EPERM, "map_get_fd_by_id_fails");
-		ASSERT_EQ(bpf_map_get_next_id(map_info.id, &next), -EPERM,
-			  "map_get_next_id_fails");
-	}
-	ASSERT_EQ(bpf_map_get_next_id(0, &next), -EPERM, "map_get_next_id_fails");
 
 	if (ASSERT_OK(bpf_link_get_info_by_fd(bpf_link__fd(skel->links.sys_nanosleep_enter),
 					      &link_info, &link_info_len),
-		      "obj_get_info_by_fd")) {
+		      "obj_get_info_by_fd"))
 		ASSERT_EQ(bpf_link_get_fd_by_id(link_info.id), -EPERM, "link_get_fd_by_id_fails");
-		ASSERT_EQ(bpf_link_get_next_id(link_info.id, &next), -EPERM,
-			  "link_get_next_id_fails");
-	}
-	ASSERT_EQ(bpf_link_get_next_id(0, &next), -EPERM, "link_get_next_id_fails");
 
 	ASSERT_EQ(bpf_prog_query(prog_fd, BPF_TRACE_FENTRY, 0, &attach_flags, prog_ids,
 				 &prog_cnt), -EPERM, "prog_query_fails");
-- 
2.34.1

[PATCH bpf-next 3/3] bpf: don't require bpf_capable() for GET_INFO_BY_FD

[Andrii Nakryiko]

The rest of BPF subsystem follows the rule that if process managed to
get BPF object FD, then it has an ownership of this object, and thus can
query any information about it, or update it. Doing something special in
GET_INFO_BY_FD operation based on bpf_capable() goes against that
philosophy, so drop the check and unify the approach with the rest of
bpf() syscall.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/syscall.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 1d74c0a8d903..b07453ce10e7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4022,17 +4022,6 @@ static int bpf_prog_get_info_by_fd(struct file *file,
 
 	info.verified_insns = prog->aux->verified_insns;
 
-	if (!bpf_capable()) {
-		info.jited_prog_len = 0;
-		info.xlated_prog_len = 0;
-		info.nr_jited_ksyms = 0;
-		info.nr_jited_func_lens = 0;
-		info.nr_func_info = 0;
-		info.nr_line_info = 0;
-		info.nr_jited_line_info = 0;
-		goto done;
-	}
-
 	ulen = info.xlated_prog_len;
 	info.xlated_prog_len = bpf_prog_insn_size(prog);
 	if (info.xlated_prog_len && ulen) {
-- 
2.34.1

Re: [PATCH bpf-next 2/3] bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID

[Alexei Starovoitov]

On Wed, May 24, 2023 at 3:55 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Getting ID of map/prog/btf/link doesn't give any access to underlying
> BPF objects, so there is no point in requiring CAP_SYS_ADMIN for these
> commands.

I don't think it's a good idea to allow unpriv to figure out
all prog/map/btf/link IDs.
Since unpriv is typically disabled it's not a security issue,
but rather a concern over abuse of IDR logic and potential
for exploits in *get_next_id() code.
At least CAP_BPF is needed.

Re: [PATCH bpf-next 3/3] bpf: don't require bpf_capable() for GET_INFO_BY_FD

[Daniel Borkmann]

On 5/25/23 12:54 AM, Andrii Nakryiko wrote:
> The rest of BPF subsystem follows the rule that if process managed to
> get BPF object FD, then it has an ownership of this object, and thus can
> query any information about it, or update it. Doing something special in
> GET_INFO_BY_FD operation based on bpf_capable() goes against that
> philosophy, so drop the check and unify the approach with the rest of
> bpf() syscall.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>   kernel/bpf/syscall.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 1d74c0a8d903..b07453ce10e7 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4022,17 +4022,6 @@ static int bpf_prog_get_info_by_fd(struct file *file,
>   
>   	info.verified_insns = prog->aux->verified_insns;
>   
> -	if (!bpf_capable()) {
> -		info.jited_prog_len = 0;
> -		info.xlated_prog_len = 0;
> -		info.nr_jited_ksyms = 0;
> -		info.nr_jited_func_lens = 0;
> -		info.nr_func_info = 0;
> -		info.nr_line_info = 0;
> -		info.nr_jited_line_info = 0;
> -		goto done;
> -	}

Isn't this leaking raw kernel pointers from JIT image this way for unpriv? I think that
is the main reason why we guarded this (originally behind !capable(CAP_SYS_ADMIN)) back
then..

>   	ulen = info.xlated_prog_len;
>   	info.xlated_prog_len = bpf_prog_insn_size(prog);
>   	if (info.xlated_prog_len && ulen) {
> 

Re: [PATCH bpf-next 2/3] bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID

[Andrii Nakryiko]

On Wed, May 24, 2023 at 8:23 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, May 24, 2023 at 3:55 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > Getting ID of map/prog/btf/link doesn't give any access to underlying
> > BPF objects, so there is no point in requiring CAP_SYS_ADMIN for these
> > commands.
>
> I don't think it's a good idea to allow unpriv to figure out
> all prog/map/btf/link IDs.
> Since unpriv is typically disabled it's not a security issue,
> but rather a concern over abuse of IDR logic and potential
> for exploits in *get_next_id() code.
> At least CAP_BPF is needed.

Ok, sounds good. I was just trying to minimize the number of commands
that would need token_fd.

BPF_MAP_FREEZE is the one I care about the most, if that one looks
good, should we land that single patch?

Re: [PATCH bpf-next 2/3] bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID

[Alexei Starovoitov]

On Thu, May 25, 2023 at 10:05 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, May 24, 2023 at 8:23 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, May 24, 2023 at 3:55 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> > >
> > > Getting ID of map/prog/btf/link doesn't give any access to underlying
> > > BPF objects, so there is no point in requiring CAP_SYS_ADMIN for these
> > > commands.
> >
> > I don't think it's a good idea to allow unpriv to figure out
> > all prog/map/btf/link IDs.
> > Since unpriv is typically disabled it's not a security issue,
> > but rather a concern over abuse of IDR logic and potential
> > for exploits in *get_next_id() code.
> > At least CAP_BPF is needed.
>
> Ok, sounds good. I was just trying to minimize the number of commands
> that would need token_fd.
>
> BPF_MAP_FREEZE is the one I care about the most, if that one looks
> good, should we land that single patch?

Sure. Applied.

Re: [PATCH bpf-next 0/3] Relax checks for unprivileged bpf() commands

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed, 24 May 2023 15:54:18 -0700 you wrote:
> During last relaxation of bpf syscall's capabilities checks ([0]), the model
> of FD-based ownership was established: if process through whatever means got
> FD for some BPF object (map, prog, etc), it should be able to perform
> operations on this object without extra CAP_SYS_ADMIN or CAP_BPF capabilities.
> 
> It seems like we missed a few cases, though, in which we are still enforcing extra caps for no good reason, even though operations are not really unsafe and/or do not require any system-wide capabilities:
>   - BPF_MAP_FREEZE command;
>   - GET_NEXT_ID family of commands;
>   - GET_INFO_BY_FD command has extra bpf_capable()-based sanitization.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/3] bpf: drop unnecessary bpf_capable() check in BPF_MAP_FREEZE command
    https://git.kernel.org/bpf/bpf-next/c/c4c84f6fb2c4
  - [bpf-next,2/3] bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID
    (no matching commit)
  - [bpf-next,3/3] bpf: don't require bpf_capable() for GET_INFO_BY_FD
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next 3/3] bpf: don't require bpf_capable() for GET_INFO_BY_FD

[Andrii Nakryiko]

On Thu, May 25, 2023 at 6:14 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 5/25/23 12:54 AM, Andrii Nakryiko wrote:
> > The rest of BPF subsystem follows the rule that if process managed to
> > get BPF object FD, then it has an ownership of this object, and thus can
> > query any information about it, or update it. Doing something special in
> > GET_INFO_BY_FD operation based on bpf_capable() goes against that
> > philosophy, so drop the check and unify the approach with the rest of
> > bpf() syscall.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   kernel/bpf/syscall.c | 11 -----------
> >   1 file changed, 11 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 1d74c0a8d903..b07453ce10e7 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -4022,17 +4022,6 @@ static int bpf_prog_get_info_by_fd(struct file *file,
> >
> >       info.verified_insns = prog->aux->verified_insns;
> >
> > -     if (!bpf_capable()) {
> > -             info.jited_prog_len = 0;
> > -             info.xlated_prog_len = 0;
> > -             info.nr_jited_ksyms = 0;
> > -             info.nr_jited_func_lens = 0;
> > -             info.nr_func_info = 0;
> > -             info.nr_line_info = 0;
> > -             info.nr_jited_line_info = 0;
> > -             goto done;
> > -     }
>
> Isn't this leaking raw kernel pointers from JIT image this way for unpriv? I think that
> is the main reason why we guarded this (originally behind !capable(CAP_SYS_ADMIN)) back
> then..

Ah, ok, makes sense. We are protecting kernel from unpriv prog/user,
so the "if you have FD you can get info about object" rule doesn't
apply here.

>
> >       ulen = info.xlated_prog_len;
> >       info.xlated_prog_len = bpf_prog_insn_size(prog);
> >       if (info.xlated_prog_len && ulen) {
> >
>

Re: [PATCH bpf-next 2/3] bpf: don't require CAP_SYS_ADMIN for getting NEXT_ID

[Andrii Nakryiko]

On Thu, May 25, 2023 at 10:11 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 10:05 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, May 24, 2023 at 8:23 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Wed, May 24, 2023 at 3:55 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> > > >
> > > > Getting ID of map/prog/btf/link doesn't give any access to underlying
> > > > BPF objects, so there is no point in requiring CAP_SYS_ADMIN for these
> > > > commands.
> > >
> > > I don't think it's a good idea to allow unpriv to figure out
> > > all prog/map/btf/link IDs.
> > > Since unpriv is typically disabled it's not a security issue,
> > > but rather a concern over abuse of IDR logic and potential
> > > for exploits in *get_next_id() code.
> > > At least CAP_BPF is needed.
> >
> > Ok, sounds good. I was just trying to minimize the number of commands
> > that would need token_fd.
> >
> > BPF_MAP_FREEZE is the one I care about the most, if that one looks
> > good, should we land that single patch?
>
> Sure. Applied.

Great, thank you!

Re: [PATCH bpf-next 3/3] bpf: don't require bpf_capable() for GET_INFO_BY_FD

[Daniel Borkmann]

On 5/25/23 7:20 PM, Andrii Nakryiko wrote:
> On Thu, May 25, 2023 at 6:14 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 5/25/23 12:54 AM, Andrii Nakryiko wrote:
>>> The rest of BPF subsystem follows the rule that if process managed to
>>> get BPF object FD, then it has an ownership of this object, and thus can
>>> query any information about it, or update it. Doing something special in
>>> GET_INFO_BY_FD operation based on bpf_capable() goes against that
>>> philosophy, so drop the check and unify the approach with the rest of
>>> bpf() syscall.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>>    kernel/bpf/syscall.c | 11 -----------
>>>    1 file changed, 11 deletions(-)
>>>
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 1d74c0a8d903..b07453ce10e7 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -4022,17 +4022,6 @@ static int bpf_prog_get_info_by_fd(struct file *file,
>>>
>>>        info.verified_insns = prog->aux->verified_insns;
>>>
>>> -     if (!bpf_capable()) {
>>> -             info.jited_prog_len = 0;
>>> -             info.xlated_prog_len = 0;
>>> -             info.nr_jited_ksyms = 0;
>>> -             info.nr_jited_func_lens = 0;
>>> -             info.nr_func_info = 0;
>>> -             info.nr_line_info = 0;
>>> -             info.nr_jited_line_info = 0;
>>> -             goto done;
>>> -     }
>>
>> Isn't this leaking raw kernel pointers from JIT image this way for unpriv? I think that
>> is the main reason why we guarded this (originally behind !capable(CAP_SYS_ADMIN)) back
>> then..
> 
> Ah, ok, makes sense. We are protecting kernel from unpriv prog/user,
> so the "if you have FD you can get info about object" rule doesn't
> apply here.

Yeah that is correct.