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
| ||
|
Message-ID: <CAF=yD-JVDGmkrGDQ-k_m6KMO=kbB23MfHcg5ovQpyn4DbJ0YuA@mail.gmail.com> Date: Tue, 15 Jan 2019 20:28:57 -0500 From: Willem de Bruijn <willemdebruijn.kernel@...il.com> To: Daniel Borkmann <daniel@...earbox.net> Cc: Martin KaFai Lau <kafai@...com>, Network Development <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...com>, tgraf@...g.ch Subject: Re: [PATCH net 1/2] bpf: Fix bpf_redirect to an ipip/ip6tnl dev On Thu, Jan 10, 2019 at 5:44 PM Willem de Bruijn <willemdebruijn.kernel@...il.com> wrote: > > On Thu, Jan 10, 2019 at 5:22 PM Daniel Borkmann <daniel@...earbox.net> wrote: > > > > On 01/10/2019 09:41 PM, Willem de Bruijn wrote: > > > On Wed, Nov 9, 2016 at 6:57 PM Martin KaFai Lau <kafai@...com> wrote: > > >> > > >> If the bpf program calls bpf_redirect(dev, 0) and dev is > > >> an ipip/ip6tnl, it currently includes the mac header. > > >> e.g. If dev is ipip, the end result is IP-EthHdr-IP instead > > >> of IP-IP. > > >> > > >> The fix is to pull the mac header. At ingress, skb_postpull_rcsum() > > >> is not needed because the ethhdr should have been pulled once already > > >> and then got pushed back just before calling the bpf_prog. > > >> At egress, this patch calls skb_postpull_rcsum(). > > >> > > >> If bpf_redirect(dev, BPF_F_INGRESS) is called, > > >> it also fails now because it calls dev_forward_skb() which > > >> eventually calls eth_type_trans(skb, dev). The eth_type_trans() > > >> will set skb->type = PACKET_OTHERHOST because the mac address > > >> does not match the redirecting dev->dev_addr. The PACKET_OTHERHOST > > >> will eventually cause the ip_rcv() errors out. To fix this, > > >> ____dev_forward_skb() is added. > > >> > > >> Joint work with Daniel Borkmann. > > >> > > >> Fixes: cfc7381b3002 ("ip_tunnel: add collect_md mode to IPIP tunnel") > > >> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels") > > >> Acked-by: Daniel Borkmann <daniel@...earbox.net> > > >> Acked-by: Alexei Starovoitov <ast@...com> > > >> Signed-off-by: Martin KaFai Lau <kafai@...com> > > >> --- > > >> +static int __bpf_redirect_no_mac(struct sk_buff *skb, struct net_device *dev, > > >> + u32 flags) > > >> +{ > > >> + /* skb->mac_len is not set on normal egress */ > > >> + unsigned int mlen = skb->network_header - skb->mac_header; > > >> + > > >> + __skb_pull(skb, mlen); > > > > > > syzbot found an issue when redirecting an LWT_XMIT program using > > > BPF_PROG_TEST_RUN. > > > > > > That path produces packets with skb->data at skb->network_header, as > > > is_l2 is false in bpf_prog_test_run_skb. > > > > > > I think the correct fix here is to pull from skb->data up to > > > skb->network_header, instead of unconditionally from skb->mac_header: > > > > > > - unsigned int mlen = skb->network_header - skb->mac_header; > > > + unsigned int mlen = skb_network_offset(skb); > > > > > > - __skb_pull(skb, mlen); > > > - > > > - /* At ingress, the mac header has already been pulled once. > > > - * At egress, skb_pospull_rcsum has to be done in case that > > > - * the skb is originated from ingress (i.e. a forwarded skb) > > > - * to ensure that rcsum starts at net header. > > > - */ > > > - if (!skb_at_tc_ingress(skb)) > > > - skb_postpull_rcsum(skb, skb_mac_header(skb), mlen); > > > + if (mlen) { > > > + __skb_pull(skb, mlen); > > > + > > > + /* At ingress, the mac header has already been pulled once. > > > + * At egress, skb_pospull_rcsum has to be done in case that > > > + * the skb is originated from ingress (i.e. a forwarded skb) > > > + * to ensure that rcsum starts at net header. > > > + */ > > > + if (!skb_at_tc_ingress(skb)) > > > + skb_postpull_rcsum(skb, skb_mac_header(skb), mlen); > > > + } > > > > > > Comments, concerns? > > > > Hmm, thanks for the report! Do you have some more info on the root cause, > > > But then also the > > skb_postpull_rcsum() from above would be wrong as well, no? Given we have > > no actual LWT_* program under BPF kselftest, I'm actually wondering whether > > the BPF_PROG_TEST_RUN framework correctly reflects skb setup from lwt with > > all its assumptions, but if that is indeed correct and is the only such > > case that causes trouble then another option would be to fix up run_lwt_bpf() > > to correctly prep the skb before skb_do_redirect() to avoid potential > > breakage on others. Not sure if this would do it, so wild shot in the dark: > > > > diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c > > index 3e85437..a648568 100644 > > --- a/net/core/lwt_bpf.c > > +++ b/net/core/lwt_bpf.c > > @@ -63,6 +63,7 @@ static int run_lwt_bpf(struct sk_buff *skb, struct bpf_lwt_prog *lwt, > > lwt->name ? : "<unknown>"); > > ret = BPF_OK; > > } else { > > + skb_reset_mac_header(skb); > > ret = skb_do_redirect(skb); > > if (ret == 0) > > ret = BPF_REDIRECT; > > Sent http://patchwork.ozlabs.org/patch/1025584 including your suggestion. Thanks Daniel. mac_header is indeed not yet set at ip_finish_output2 and lwtunnel_xmit. This line is not strictly needed to fix the bad packet that syzkaller cooked through bpf_prog_test_run_skb. But we care more about fixing the real lightweight tunnel paths and both changes will be needed there.
Powered by blists - more mailing lists