netdev - Re: [PATCH v5 bpf-next 2/2] bpf: add tests for direct packet access from CGROUP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <D1B874C2-0989-4B78-B74C-507464D78908@fb.com>
Date:   Fri, 19 Oct 2018 05:49:07 +0000
From:   Song Liu <songliubraving@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v5 bpf-next 2/2] bpf: add tests for direct packet access
 from CGROUP_SKB



> On Oct 18, 2018, at 10:22 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> 
> 
> 
> On 10/18/2018 10:01 PM, Song Liu wrote:
>> Tests are added to make sure CGROUP_SKB cannot access:
>>  tc_classid, data_meta, flow_keys
>> 
>> and can read and write:
>>  mark, prority, and cb[0-4]
>> 
>> and can read other fields.
>> 
>> To make selftest with skb->sk work, a dummy sk is added in
>> bpf_prog_test_run_skb().
>> 
>> Signed-off-by: Song Liu <songliubraving@...com>
>> ---
>> net/bpf/test_run.c                          |   7 +
>> tools/testing/selftests/bpf/test_verifier.c | 171 ++++++++++++++++++++
>> 2 files changed, 178 insertions(+)
>> 
>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> index 0c423b8cd75c..8dccac305268 100644
>> --- a/net/bpf/test_run.c
>> +++ b/net/bpf/test_run.c
>> @@ -10,6 +10,8 @@
>> #include <linux/etherdevice.h>
>> #include <linux/filter.h>
>> #include <linux/sched/signal.h>
>> +#include <net/sock.h>
>> +#include <net/tcp.h>
>> 
>> static __always_inline u32 bpf_test_run_one(struct bpf_prog *prog, void *ctx,
>> 		struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
>> @@ -115,6 +117,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>> 	u32 retval, duration;
>> 	int hh_len = ETH_HLEN;
>> 	struct sk_buff *skb;
>> +	struct sock sk = {0};
> 
> Arg another dummy :/
> 
>> 	void *data;
>> 	int ret;
>> 
>> @@ -137,11 +140,15 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>> 		break;
>> 	}
>> 
>> +	sock_net_set(&sk, &init_net);
> 
> A few lines later we use :
> 
> skb->protocol = eth_type_trans(skb, current->nsproxy->net_ns->loopback_dev);
> 
> So it looks like you want instead for consistency :
> 
> sock_net_set(&sk, current->nsproxy->net_ns);

Thanks! Will fix this in v6.

> 	
> 
>> +	sock_init_data(NULL, &sk);
>> +
>> 	skb = build_skb(data, 0);
>> 	if (!skb) {
>> 		kfree(data);
>> 		return -ENOMEM;
>> 	}
>> +	skb->sk = &sk;
>> 
> 
> Normally this would need a skb->destructor, but I guess nothing will call skb_orphan()
> from this point.

Yeah, I double checked, this should be OK. 

Song