diff mbox

[net-next,v2,2/2] bpf: Remove the capability check for cgroup skb eBPF program

Message ID 1496279760-20996-2-git-send-email-chenbofeng.kernel@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Chenbo Feng June 1, 2017, 1:16 a.m. UTC 
From: Chenbo Feng <fengc@google.com>

Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
capability while attaching the program to a cgroup only requires the
user have CAP_NET_ADMIN privilege. We can escape the capability
check when load the program just like socket filter program to make
the capability requirement consistent.

Change since v1:
Change the code style in order to be compliant with checkpatch.pl
preference

Signed-off-by: Chenbo Feng <fengc@google.com>
---
 kernel/bpf/syscall.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov June 1, 2017, 11:42 p.m. UTC | #1 
On Wed, May 31, 2017 at 06:16:00PM -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
> capability while attaching the program to a cgroup only requires the
> user have CAP_NET_ADMIN privilege. We can escape the capability
> check when load the program just like socket filter program to make
> the capability requirement consistent.
> 
> Change since v1:
> Change the code style in order to be compliant with checkpatch.pl
> preference
> 
> Signed-off-by: Chenbo Feng <fengc@google.com>

as far as I can see they're indeed the same as socket filters, so
Acked-by: Alexei Starovoitov <ast@kernel.org>

but I don't quite understand how it helps, since as you said
attaching such unpriv fd to cgroup still requires root.
Do you have more patches to follow?
Alexei Starovoitov June 2, 2017, 1:58 a.m. UTC | #2 
On Thu, Jun 01, 2017 at 06:55:09PM -0700, Chenbo Feng wrote:
> On Thu, Jun 1, 2017 at 4:42 PM, Alexei Starovoitov <
> alexei.starovoitov@gmail.com> wrote:
> 
> > On Wed, May 31, 2017 at 06:16:00PM -0700, Chenbo Feng wrote:
> > > From: Chenbo Feng <fengc@google.com>
> > >
> > > Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
> > > capability while attaching the program to a cgroup only requires the
> > > user have CAP_NET_ADMIN privilege. We can escape the capability
> > > check when load the program just like socket filter program to make
> > > the capability requirement consistent.
> > >
> > > Change since v1:
> > > Change the code style in order to be compliant with checkpatch.pl
> > > preference
> > >
> > > Signed-off-by: Chenbo Feng <fengc@google.com>
> >
> > as far as I can see they're indeed the same as socket filters, so
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
> >
> > but I don't quite understand how it helps, since as you said
> > attaching such unpriv fd to cgroup still requires root.
> > Do you have more patches to follow?
> >
> > Actually not, the purpose of this patch is only to make sure if a program
> have
> CAP_NET_ADMIN but not CAP_SYS_ADMIN it can still load and attach eBPF
> programs to cgroup.

got it. Thanks
David Miller June 2, 2017, 6:25 p.m. UTC | #3 
From: Chenbo Feng <chenbofeng.kernel@gmail.com>
Date: Wed, 31 May 2017 18:16:00 -0700

> From: Chenbo Feng <fengc@google.com>
> 
> Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
> capability while attaching the program to a cgroup only requires the
> user have CAP_NET_ADMIN privilege. We can escape the capability
> check when load the program just like socket filter program to make
> the capability requirement consistent.
> 
> Change since v1:
> Change the code style in order to be compliant with checkpatch.pl
> preference
> 
> Signed-off-by: Chenbo Feng <fengc@google.com>

Applied.
Daniel Borkmann June 6, 2017, 4:56 p.m. UTC | #4 
On 06/02/2017 01:42 AM, Alexei Starovoitov wrote:
> On Wed, May 31, 2017 at 06:16:00PM -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
>> capability while attaching the program to a cgroup only requires the
>> user have CAP_NET_ADMIN privilege. We can escape the capability
>> check when load the program just like socket filter program to make
>> the capability requirement consistent.
>>
>> Change since v1:
>> Change the code style in order to be compliant with checkpatch.pl
>> preference
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>
> as far as I can see they're indeed the same as socket filters, so
> Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> but I don't quite understand how it helps, since as you said
> attaching such unpriv fd to cgroup still requires root.
> Do you have more patches to follow?

Hmm, when we relax this from capable(CAP_SYS_ADMIN) to unprivileged,
then we must at least also zero out the not-yet-initialized memory
for the mac header for egress case in __cgroup_bpf_run_filter_skb().
Chenbo Feng June 6, 2017, 10:44 p.m. UTC | #5 
On 06/06/2017 09:56 AM, Daniel Borkmann wrote:
> On 06/02/2017 01:42 AM, Alexei Starovoitov wrote:
>> On Wed, May 31, 2017 at 06:16:00PM -0700, Chenbo Feng wrote:
>>> From: Chenbo Feng <fengc@google.com>
>>>
>>> Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
>>> capability while attaching the program to a cgroup only requires the
>>> user have CAP_NET_ADMIN privilege. We can escape the capability
>>> check when load the program just like socket filter program to make
>>> the capability requirement consistent.
>>>
>>> Change since v1:
>>> Change the code style in order to be compliant with checkpatch.pl
>>> preference
>>>
>>> Signed-off-by: Chenbo Feng <fengc@google.com>
>>
>> as far as I can see they're indeed the same as socket filters, so
>> Acked-by: Alexei Starovoitov <ast@kernel.org>
>>
>> but I don't quite understand how it helps, since as you said
>> attaching such unpriv fd to cgroup still requires root.
>> Do you have more patches to follow?
>
> Hmm, when we relax this from capable(CAP_SYS_ADMIN) to unprivileged,
> then we must at least also zero out the not-yet-initialized memory
> for the mac header for egress case in __cgroup_bpf_run_filter_skb().
>

Do you mean something like:

if (type == BPF_CGROUP_INET_EGRESS) {

         offset = skb_network_header(skb) - skb_mac_header(skb);

         memset(skb_mac_header(skb), 0, offset)

}

And could you explain more on why we need to do this if we remove the 
CAP_SYS_ADMIN check? I thought we still cannot directly access the 
sk_buff without using bpf_skb_load_bytes helper and we still need a 
CAP_NET_ADMIN in order to attach and run the program on egress side right?
Daniel Borkmann June 7, 2017, 3:57 p.m. UTC | #6 
On 06/07/2017 12:44 AM, Chenbo Feng wrote:
> On 06/06/2017 09:56 AM, Daniel Borkmann wrote:
>> On 06/02/2017 01:42 AM, Alexei Starovoitov wrote:
>>> On Wed, May 31, 2017 at 06:16:00PM -0700, Chenbo Feng wrote:
>>>> From: Chenbo Feng <fengc@google.com>
>>>>
>>>> Currently loading a cgroup skb eBPF program require a CAP_SYS_ADMIN
>>>> capability while attaching the program to a cgroup only requires the
>>>> user have CAP_NET_ADMIN privilege. We can escape the capability
>>>> check when load the program just like socket filter program to make
>>>> the capability requirement consistent.
>>>>
>>>> Change since v1:
>>>> Change the code style in order to be compliant with checkpatch.pl
>>>> preference
>>>>
>>>> Signed-off-by: Chenbo Feng <fengc@google.com>
>>>
>>> as far as I can see they're indeed the same as socket filters, so
>>> Acked-by: Alexei Starovoitov <ast@kernel.org>
>>>
>>> but I don't quite understand how it helps, since as you said
>>> attaching such unpriv fd to cgroup still requires root.
>>> Do you have more patches to follow?
>>
>> Hmm, when we relax this from capable(CAP_SYS_ADMIN) to unprivileged,
>> then we must at least also zero out the not-yet-initialized memory
>> for the mac header for egress case in __cgroup_bpf_run_filter_skb().
>
> Do you mean something like:
>
> if (type == BPF_CGROUP_INET_EGRESS) {
>
>          offset = skb_network_header(skb) - skb_mac_header(skb);
>
>          memset(skb_mac_header(skb), 0, offset)

That won't work, reason is the same as per 92046578ac88 ("bpf: cgroup
skb progs cannot access ld_abs/ind"), meaning that mac header is not
yet set at that point in time, but more below.

> }
>
> And could you explain more on why we need to do this if we remove the
 > CAP_SYS_ADMIN check? I thought we still cannot directly access the
 > sk_buff without using bpf_skb_load_bytes helper and we still need a
 > CAP_NET_ADMIN in order to attach and run the program on egress side right?

Ok, forget this comment of mine. The __cgroup_bpf_run_filter_skb() does
__skb_push()/__skb_pull(), but for egress case the offset is always 0, so
we don't start at mac header but at network header instead, meaning memset()
is not needed.

Thanks,
Daniel
diff mbox

Patch

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 265a0d8..59da103 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -815,7 +815,9 @@  static int bpf_prog_load(union bpf_attr *attr)
 	    attr->kern_version != LINUX_VERSION_CODE)
 		return -EINVAL;
 
-	if (type != BPF_PROG_TYPE_SOCKET_FILTER && !capable(CAP_SYS_ADMIN))
+	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
+	    type != BPF_PROG_TYPE_CGROUP_SKB &&
+	    !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
 	/* plain bpf_prog allocation */