Re: [PATCH net] bareudp: fix NULL pointer dereference in bareudp_fill_metadata_dst()

[Kuniyuki Iwashima <kuniyu@google.com>]

From: Simon Horman <horms@kernel.org>
Date: Tue, 28 Apr 2026 17:04:46 +0100
> From: 'Simon Horman' <horms@kernel.org>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> bareudp: fix NULL pointer dereference in bareudp_fill_metadata_dst()
> 
> This commit fixes a NULL pointer dereference in bareudp_fill_metadata_dst()
> by adding a check for the socket in the IPv6 path. If the device is down
> and the socket is NULL, it now returns -ESHUTDOWN.
> 
> > Add a NULL check returning -ESHUTDOWN, consistent with the xmit paths
> > in the same driver.
> 
> Does the IPv4 path also need to be updated to remain consistent?

No, because udp_tunnel_dst_lookup() does not take struct
sock while udp_tunnel6_dst_lookup() does.

And all the other fast paths (bareudp_xmit_skb() and
bareudp6_xmit_skb()) have NULL checks properly.


> 
> While the IPv4 path lookup doesn't use the socket pointer directly, if the
> socket is NULL (meaning the device is down), shouldn't the IPv4 path also
> return -ESHUTDOWN instead of successfully performing a route lookup?
> 
> > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > index 0df3208783ad..da5866ba0699 100644
> > --- a/drivers/net/bareudp.c
> > +++ b/drivers/net/bareudp.c
> > @@ -529,6 +529,9 @@ static int bareudp_fill_metadata_dst(struct net_device *dev,
> 
> Is it possible for an IPv6 tunnel key to be incorrectly processed as IPv4
> earlier in this function?

I think this is orthogonal to this patch and can be a follow-up.

I have a series to remove all synchronize_rcu() from all UDP tunnel
users and udp_tunnel_sock_release(), but it conflict with this patch,
so I hope this land net-next this Thursday :)


> 
> Looking at the preceding condition in this function:
> 
>     if (!ipv6_mod_enabled() || ip_tunnel_info_af(info) == AF_INET) {
>         struct rtable *rt;
>         ...
>         info->key.u.ipv4.src = saddr;
>     }
> 
> If the IPv6 module is disabled at runtime, but an IPv6 tunnel key is provided
> (e.g., via OVS or TC), the code falls back to the IPv4 branch.
> 
> Since key->u.ipv4.dst aliases exactly with bytes 4-7 of key->u.ipv6.src due
> to the union layout in struct ip_tunnel_key, could this result in an invalid
> IPv4 route lookup using portions of the IPv6 source address?
> 
> Would it be safer to explicitly reject IPv6 tunnel keys when the IPv6 module
> is disabled?