Re: [PATCH 4.4] ipv6: check skb->protocol before lookup for nexthop

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Aug 20, 2020 at 10:31:57AM +0100, Alessio Balsini wrote:
> On Thu, Aug 20, 2020 at 11:06:20AM +0200, Greg KH wrote:
> > On Thu, Aug 20, 2020 at 09:55:11AM +0100, Alessio Balsini wrote:
> > > On Thu, Aug 20, 2020 at 10:09:02AM +0200, Greg KH wrote:
> > > > On Wed, Aug 19, 2020 at 09:11:17PM +0100, Alessio Balsini wrote:
> > > > > From: WANG Cong <xiyou.wangcong@gmail.com>
> > > > > 
> > > > > [ Upstream commit 199ab00f3cdb6f154ea93fa76fd80192861a821d ]
> > > > > 
> > > > > Andrey reported a out-of-bound access in ip6_tnl_xmit(), this
> > > > > is because we use an ipv4 dst in ip6_tnl_xmit() and cast an IPv4
> > > > > neigh key as an IPv6 address:
> > > > > 
> > > > >         neigh = dst_neigh_lookup(skb_dst(skb),
> > > > >                                  &ipv6_hdr(skb)->daddr);
> > > > >         if (!neigh)
> > > > >                 goto tx_err_link_failure;
> > > > > 
> > > > >         addr6 = (struct in6_addr *)&neigh->primary_key; // <=== HERE
> > > > >         addr_type = ipv6_addr_type(addr6);
> > > > > 
> > > > >         if (addr_type == IPV6_ADDR_ANY)
> > > > >                 addr6 = &ipv6_hdr(skb)->daddr;
> > > > > 
> > > > >         memcpy(&fl6->daddr, addr6, sizeof(fl6->daddr));
> > > > > 
> > > > > Also the network header of the skb at this point should be still IPv4
> > > > > for 4in6 tunnels, we shold not just use it as IPv6 header.
> > > > > 
> > > > > This patch fixes it by checking if skb->protocol is ETH_P_IPV6: if it
> > > > > is, we are safe to do the nexthop lookup using skb_dst() and
> > > > > ipv6_hdr(skb)->daddr; if not (aka IPv4), we have no clue about which
> > > > > dest address we can pick here, we have to rely on callers to fill it
> > > > > from tunnel config, so just fall to ip6_route_output() to make the
> > > > > decision.
> > > > > 
> > > > > Fixes: ea3dc9601bda ("ip6_tunnel: Add support for wildcard tunnel endpoints.")
> > > > > Reported-by: Andrey Konovalov <andreyknvl@google.com>
> > > > > Reported-by: syzbot+01400f5fc51cf4747bec@syzkaller.appspotmail.com
> > > > > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> > > > > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Signed-off-by: Alessio Balsini <balsini@android.com>
> > > > > ---
> > > > >  net/ipv6/ip6_tunnel.c | 32 +++++++++++++++++---------------
> > > > >  1 file changed, 17 insertions(+), 15 deletions(-)
> > > > 
> > > > This was already applied to the 4.4.66 kernel release
> > > > 
> > > > But this patch applies to the 4.4.y tree.  Which is really really odd,
> > > > what is going on here?
> > > > 
> > > > confused,
> > > > 
> > > > greg k-h
> > > 
> > > Totally odd... Now that you gave me this heads up, I can see that the
> > > patch was applied to v4.4.66 and for some reason dropped since v4.4.118.
> > > 
> > > Can you please take a look? Thanks!
> > 
> > What dropped it?  Fixes that resolved other things?  Are you sure this
> > is still needed?
> > 
> > That's all I can tell, you can see the kernel branch as well as I can :)
> > 
> > greg k-h
> 
> It looks like upstream 607f725f6f7d (net: replace dst_cache ip6_tunnel
> implementation with the generic one), had some extra code removed when
> backported as b8c7f80cbdcd in v4.4.118, resulting in a complete, hidden
> revert of the patch proposed in this thread.

Odd, ok, I've queued this up now, thanks!

greg k-h