[RFC PATCH] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[RFC PATCH] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[Yauheni Kaliuta]

I'm wodering about the cap check against CAP_SYS_ADMIN. Is it
intentional to provide more security or oversight in commit
2c78ee898d8f ("bpf: Implement CAP_BPF")?

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
---
 include/linux/filter.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index a5f21dc3c432..3de96b1a736b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1100,7 +1100,7 @@ static inline bool bpf_jit_blinding_enabled(struct bpf_prog *prog)
 		return false;
 	if (!bpf_jit_harden)
 		return false;
-	if (bpf_jit_harden == 1 && capable(CAP_SYS_ADMIN))
+	if (bpf_jit_harden == 1 && bpf_capable())
 		return false;
 
 	return true;
-- 
2.34.1

[RFC PATCH v2] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[Yauheni Kaliuta]

The capability check can cause SELinux denial.

For example, in ptp4l, setsockopt() with the SO_ATTACH_FILTER option
raises sk_attach_filter() to run a bpf program. SELinux hooks into
capable() calls and performs an additional check if the task's
SELinux domain has permission to "use" the given capability. ptp4l_t
already has CAP_BPF granted by SELinux, so if the function used
bpf_capable() as most BPF code does, there would be no change needed
in selinux-policy.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
---

v2: put the reasoning in the commit message

---
 include/linux/filter.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index a5f21dc3c432..3de96b1a736b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1100,7 +1100,7 @@ static inline bool bpf_jit_blinding_enabled(struct bpf_prog *prog)
 		return false;
 	if (!bpf_jit_harden)
 		return false;
-	if (bpf_jit_harden == 1 && capable(CAP_SYS_ADMIN))
+	if (bpf_jit_harden == 1 && bpf_capable())
 		return false;
 
 	return true;
-- 
2.34.1

Re: [RFC PATCH v2] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[Serge E. Hallyn]

On Wed, Aug 31, 2022 at 06:24:14PM +0300, Yauheni Kaliuta wrote:
> The capability check can cause SELinux denial.
> 
> For example, in ptp4l, setsockopt() with the SO_ATTACH_FILTER option
> raises sk_attach_filter() to run a bpf program. SELinux hooks into
> capable() calls and performs an additional check if the task's
> SELinux domain has permission to "use" the given capability. ptp4l_t
> already has CAP_BPF granted by SELinux, so if the function used
> bpf_capable() as most BPF code does, there would be no change needed
> in selinux-policy.

The selinux mentions probably aren't really necessary.  The more
concise way to say it is that bpf_jit_blinding_enabled() should
be permitted with CAP_BPF, that full CAP_SYS_ADMIN is not needed.
(Assuming that that is the case)

> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
> ---
> 
> v2: put the reasoning in the commit message
> 
> ---
>  include/linux/filter.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index a5f21dc3c432..3de96b1a736b 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1100,7 +1100,7 @@ static inline bool bpf_jit_blinding_enabled(struct bpf_prog *prog)
>  		return false;
>  	if (!bpf_jit_harden)
>  		return false;
> -	if (bpf_jit_harden == 1 && capable(CAP_SYS_ADMIN))
> +	if (bpf_jit_harden == 1 && bpf_capable())
>  		return false;
>  
>  	return true;
> -- 
> 2.34.1

Re: [RFC PATCH v2] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[Daniel Borkmann]

On 8/31/22 8:50 PM, Serge E. Hallyn wrote:
> On Wed, Aug 31, 2022 at 06:24:14PM +0300, Yauheni Kaliuta wrote:
>> The capability check can cause SELinux denial.
>>
>> For example, in ptp4l, setsockopt() with the SO_ATTACH_FILTER option
>> raises sk_attach_filter() to run a bpf program. SELinux hooks into
>> capable() calls and performs an additional check if the task's
>> SELinux domain has permission to "use" the given capability. ptp4l_t
>> already has CAP_BPF granted by SELinux, so if the function used
>> bpf_capable() as most BPF code does, there would be no change needed
>> in selinux-policy.
> 
> The selinux mentions probably aren't really necessary.  The more
> concise way to say it is that bpf_jit_blinding_enabled() should
> be permitted with CAP_BPF, that full CAP_SYS_ADMIN is not needed.
> (Assuming that that is the case)
> 
>> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
>> ---
>>
>> v2: put the reasoning in the commit message
>>
>> ---
>>   include/linux/filter.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index a5f21dc3c432..3de96b1a736b 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -1100,7 +1100,7 @@ static inline bool bpf_jit_blinding_enabled(struct bpf_prog *prog)
>>   		return false;
>>   	if (!bpf_jit_harden)
>>   		return false;
>> -	if (bpf_jit_harden == 1 && capable(CAP_SYS_ADMIN))
>> +	if (bpf_jit_harden == 1 && bpf_capable())

I think lowering this requirement is fine here. These days given unpriv eBPF is
disabled by default, I see the main users for constant blinding coming from unpriv
in particular via cBPF -> eBPF migration (e.g. old-style socket filters).

>>   		return false;
>>   
>>   	return true;

Please also update Documentation/admin-guide/sysctl/net.rst to clarify cap details.

Thanks,
Daniel

[PATCH bpf-next] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[Yauheni Kaliuta]

The full CAP_SYS_ADMIN requirement for blining looks too strict
nowadays. These days given unpriv eBPF is disabled by default, the
main users for constant blinding coming from unpriv in particular
via cBPF -> eBPF migration (e.g. old-style socket filters).

Discussion: https://lore.kernel.org/bpf/20220831090655.156434-1-ykaliuta@redhat.com/

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
---
 Documentation/admin-guide/sysctl/net.rst | 3 +++
 include/linux/filter.h                   | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
index 805f2281e000..ff1e5b5acd28 100644
--- a/Documentation/admin-guide/sysctl/net.rst
+++ b/Documentation/admin-guide/sysctl/net.rst
@@ -101,6 +101,9 @@ Values:
 	- 1 - enable JIT hardening for unprivileged users only
 	- 2 - enable JIT hardening for all users
 
+where "privileged user" in this context means a process having
+CAP_BPF or CAP_SYS_ADMIN in the root user name space.
+
 bpf_jit_kallsyms
 ----------------
 
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 527ae1d64e27..75335432fcbc 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1099,7 +1099,7 @@ static inline bool bpf_jit_blinding_enabled(struct bpf_prog *prog)
 		return false;
 	if (!bpf_jit_harden)
 		return false;
-	if (bpf_jit_harden == 1 && capable(CAP_SYS_ADMIN))
+	if (bpf_jit_harden == 1 && bpf_capable())
 		return false;
 
 	return true;
-- 
2.34.1

Re: [PATCH bpf-next] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon,  5 Sep 2022 12:01:49 +0300 you wrote:
> The full CAP_SYS_ADMIN requirement for blining looks too strict
> nowadays. These days given unpriv eBPF is disabled by default, the
> main users for constant blinding coming from unpriv in particular
> via cBPF -> eBPF migration (e.g. old-style socket filters).
> 
> Discussion: https://lore.kernel.org/bpf/20220831090655.156434-1-ykaliuta@redhat.com/
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: use bpf_capable() instead of CAP_SYS_ADMIN for blinding decision
    https://git.kernel.org/bpf/bpf-next/c/bfeb7e399bac

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