netdev - Re: [PATCH net-next 3/6] samples: bpf: example of stateful socket filtering

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: <CAMEtUuw+z61FL81-_aZ0=D5XKm-_gh+Cs1uGL2th+0A+hcYPGQ@mail.gmail.com>
Date:	Sat, 29 Nov 2014 22:24:28 -0800
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	David Miller <davem@...emloft.net>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Daniel Borkmann <dborkman@...hat.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Linux API <linux-api@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 3/6] samples: bpf: example of stateful socket filtering

On Sat, Nov 29, 2014 at 9:01 PM, David Miller <davem@...emloft.net> wrote:
> From: Alexei Starovoitov <ast@...mgrid.com>
> Date: Wed, 26 Nov 2014 21:42:28 -0800
>
>> this socket filter example does:
>> - creates arraymap in kernel with key 4 bytes and value 8 bytes
>>
>> - loads eBPF program:
>>   r0 = skb[14 + 9]; // load one byte of ip->proto
>  ...
>> +             BPF_LD_ABS(BPF_B, 14 + 9 /* R0 = ip->proto */),
>
> I do not want anything having to do with fixed offsets from
> the skb.
>
> Nothing should know where things are in the SKB structure,
> especially user facing things.
>
> That's why we have explicit BPF operations for fetching
> specific SKB members, so that the layout is completely
> transparent to the entity generating BPF programs.
>
> Besides retaining the flexibility of changing the SKB
> layout arbitrarily without breaking bpf programs, there
> are also security considerations from allowing bpf
> programs to load arbitrary offsets.
>
> Sorry, I do not like this patch series at all.

sigh.
You misunderstood one wrong comment in the whole series.
r0 = skb[14 + 9]; // load one byte of ip->proto
is saying
r0 = skb->data[14 + 9]; // load one byte of ip->proto
it's loading one byte of packet payload and
not one byte of skb structure.

There are plenty of other comments where I mentioned that.
Including cover letter:
"Though native eBPF programs are way more powerful than classic filters
(attachable through similar setsockopt() call), they don't have skb field
accessors yet. Like skb->pkt_type, skb->dev->ifindex are not accessible.
There are sevaral ways to achieve that. That will be in the next set of patches.
So in this set native eBPF programs can only read data from packet and
access maps"

and in patch #2:
+static bool sock_filter_is_valid_access(int off, int size, enum
bpf_access_type type)
+{
+       /* skb fields cannot be accessed yet */
+       return false;
+}

In other words, there is no way to access skb fields yet.
There are several different ways I've implemented to
access skb fields, but they all had their cons and pros, so I
dropped them all to focus on basic things first.
Which is: read packet data and access maps.

Please let me know if you want me to fix this comment
in this patch of sample code.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html