netdev - Re: __sk_buff.data

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Apr 2017 16:28:47 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Johannes Berg <johannes@...solutions.net>,
        Alexei Starovoitov <ast@...nel.org>
CC:     netdev <netdev@...r.kernel.org>
Subject: Re: __sk_buff.data_end

On 04/20/2017 04:17 PM, Johannes Berg wrote:
> On Thu, 2017-04-20 at 16:10 +0200, Daniel Borkmann wrote:
>>
>> I think this would be a rather more complex operation on the BPF
>> side, it would need changes from LLVM (which assumes initial ctx sits
>> in r1), verifier for tracking this ctx2, all the way down to JITs
>> plus some way to handle 1 and 2 argument program calls generically.
>> Much easier to pass additional meta data for the program via cb[],
>> for example.
>
> Yeah, it did seem very complex :)
>
>>> Alternatively I can clear another pointer (u64) in the CB, store a
>>> pointer there, and always emit code following that pointer - should
>>> be possible right?
>>
>> What kind of pointer? If it's something like data_end as read-only,
>> then this needs to be tracked in the verifier in addition, of course.
>> Other option you could do (depending on what you want to achieve) is
>> to have a bpf_probe_read() version as a helper for your prog type
>> that would further walk that pointer/struct (similar to tracing)
>> where this comes w/o any backward compat guarantees, though.
>
> I meant something like this
>
> struct wifi_cb {
> 	struct wifi_data *wifi_data;
> 	...
> 	void *data_end; // with BUILD_BUG_ON to the right offset
> };
>
> Then struct wifi_data can contain extra data that doesn't fit into
> wifi_cb, like the stuff I evicted for *data_end and *wifi_data. Let's
> say one of those fields is "u64 boottime_ns;" (as I did in my patch
> now), so we have
>
> struct wifi_data {
> 	u64 boottime_ns;
> };
>
> then I can still have
>
> struct __wifi_sk_buff {
> 	u32 len;
> 	u32 data;
> 	u32 data_end;
> 	u32 boottime_ns; // this is strange but
> 			 // seems to be done this way?
> };
>
> And then when boottime_ns is accessed, I can have:
>
>          case offsetof(struct __wifi_sk_buff, boottime_ns):
>                  off  = si->off;
>                  off -= offsetof(struct __wifi_sk_buff, boottime_ns);
>                  off += offsetof(struct sk_buff, cb);
>                  off += offsetof(struct wifi_cb, wifi_data);
>                  *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
>                                        si->src_reg, off);
> 		off = offsetof(struct wifi_data, boottime_ns);
> 		*isns++ = BPF_LDX_MEM(BPF_SIZEOF(u64), si->dst_reg,
> 				      si->src_reg, off);
>                  break;
>
> no?
>
> It seems to me this should work, and essentially emit code to follow
> the pointer to inside struct wifi_data. Assuming

I see what you mean now. Yes, that's fine. We already do something
similar essentially with skb->ifindex access already (skb->dev +
dev->ifindex), f.e.:

[...]
	case offsetof(struct __sk_buff, ifindex):
		BUILD_BUG_ON(FIELD_SIZEOF(struct net_device, ifindex) != 4);

		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, dev),
				      si->dst_reg, si->src_reg,
				      offsetof(struct sk_buff, dev));
		*insn++ = BPF_JMP_IMM(BPF_JEQ, si->dst_reg, 0, 1);
		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
				      offsetof(struct net_device, ifindex));
		break;
[...]

Which is not too different from the above. You'd probably need to
populate the struct wifi_data each time if you place it onto the
stack, but perhaps could be optimized by storing that somewhere
else (e.g. somewhere via netdev, etc) and walking the pointer from
there, which would also spare you the cb[] save/restore.