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