netdev - Re: [PATCH net-next] bpf: fix cb access in socket filter programs

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]

Message-ID: <5614E84E.2010806@iogearbox.net>
Date:	Wed, 07 Oct 2015 11:39:26 +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 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?

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