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  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:	Wed, 07 Oct 2015 15:09:11 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Alexei Starovoitov <ast@...mgrid.com>,
	"David S. Miller" <davem@...emloft.net>
CC:	Andy Lutomirski <luto@...capital.net>,
	Ingo Molnar <mingo@...nel.org>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Kees Cook <keescook@...omium.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] bpf: fix cb access in socket filter programs

On 10/07/2015 11:39 AM, Daniel Borkmann wrote:
> On 10/07/2015 04:18 AM, Alexei Starovoitov wrote:
>> eBPF socket filter programs may see junk in 'u32 cb[5]' area,
>> since it could have been used by protocol layers earlier.
>>
>> On the receive path the af_packet sees clean skb->cb.
>> On the xmit the dev_queue_xmit_nit() delivers cloned skb, so we can
>> conditionally clean 20 bytes of skb->cb that could be used by the program.
>
> Having slept over this one night, I think this assumption is not
> always correct :/, more below ...
>
>> For programs attached to TCP/UDP sockets we need to save/restore
>> these 20 bytes, since it's used by protocol layers.
> ...
>> +static inline u32 bpf_prog_run_save_cb(const struct bpf_prog *prog,
>> +                       struct sk_buff *skb)
>> +{
>> +    u8 *cb_data = qdisc_skb_cb(skb)->data;
>> +    u8 saved_cb[QDISC_CB_PRIV_LEN];
>> +    u32 res;
>> +
>> +    BUILD_BUG_ON(FIELD_SIZEOF(struct __sk_buff, cb) !=
>> +             QDISC_CB_PRIV_LEN);
>> +
>> +    if (unlikely(prog->cb_access)) {
>> +        memcpy(saved_cb, cb_data, sizeof(saved_cb));
>> +        memset(cb_data, 0, sizeof(saved_cb));
>> +    }
>> +
>> +    res = BPF_PROG_RUN(prog, skb);
>> +
>> +    if (unlikely(prog->cb_access))
>> +        memcpy(cb_data, saved_cb, sizeof(saved_cb));
>> +
>> +    return res;
>> +}
>> +
>> +static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
>> +                    struct sk_buff *skb)
>> +{
>> +    u8 *cb_data = qdisc_skb_cb(skb)->data;
>> +
>> +    if (unlikely(prog->cb_access) && skb->pkt_type == PACKET_OUTGOING)
>> +        memset(cb_data, 0, QDISC_CB_PRIV_LEN);
>> +    return BPF_PROG_RUN(prog, skb);
>> +}
>> +
>>   static inline unsigned int bpf_prog_size(unsigned int proglen)
>>   {
>>       return max(sizeof(struct bpf_prog),
>
> bpf_prog_run_clear_cb() wouldn't work on dev_forward_skb() as
> skb->pkt_type is then being scrubbed to PACKET_HOST, so on the
> receive path, AF_PACKET might not always see clean skbs->cb[]
> as assumed ... I think that the skb->pkt_type part needs to be
> dropped, no?

Thinking a bit more about this part, which only accounts for
fanout_demux_bpf() and run_filter(), so AF_PACKET only, this
logic still needs to be slightly different:

You currently can have eBPF on packet fanout as a demux and behind
that eBPF on the actual packet socket. So, for some reason, fanout
could transfer some state to the socket along the way, which could
break when cleared as-is via bpf_prog_run_clear_cb().

So we need to make sure to only clear this once, either in front
of fanout, or when not present, in front of the socket filter.

Thanks,
Daniel
--
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

Powered by blists - more mailing lists