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

[Simon Horman <horms@kernel.org>]

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?

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?

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?

>  		struct in6_addr saddr;
>  		struct socket *sock = rcu_dereference(bareudp->sock);
>  
> +		if (!sock)
> +			return -ESHUTDOWN;
> +
>  		dst = udp_tunnel6_dst_lookup(skb, dev, bareudp->net, sock,
>  					     0, &saddr, &info->key,
>  					     sport, bareudp->port, info->key.tos,