netdev - Re: [PATCH net-next 04/10] ovpn: don't drop skb's dst when xmitting packet

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3a173ede-e2db-463e-a135-7dc9c7976cd7@redhat.com>
Date: Tue, 13 May 2025 09:45:13 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Antonio Quartulli <antonio@...nvpn.net>, netdev@...r.kernel.org
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Sabrina Dubroca <sd@...asysnail.net>, Gert Doering <gert@...enie.muc.de>
Subject: Re: [PATCH net-next 04/10] ovpn: don't drop skb's dst when xmitting
 packet

On 5/9/25 4:26 PM, Antonio Quartulli wrote:
> When routing a packet to a LAN behind a peer, ovpn needs to
> inspect the route entry that brought the packet there in the
> first place.
> 
> If this packet is truly routable, the route entry provides the
> GW to be used when looking up the VPN peer to send the packet to.
> 
> However, the route entry is currently dropped before entering
> the ovpn xmit function, because the IFF_XMIT_DST_RELEASE priv_flag
> is enabled by default.
> 
> Clear the IFF_XMIT_DST_RELEASE flag during interface setup to allow
> the route entry (skb's dst) to survive and thus be inspected
> by the ovpn routing logic.
> 
> Fixes: a3aaef8cd173 ("ovpn: implement peer lookup logic")
> Reported-by: Gert Doering <gert@...enie.muc.de>
> Tested-by: Gert Doering <gert@...enie.muc.de>
> Acked-by: Gert Doering <gert@...enie.muc.de> # as a primary user
> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>
> ---
>  drivers/net/ovpn/io.c   | 2 ++
>  drivers/net/ovpn/main.c | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index dd8a8055d967..7e4b89484c9d 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -398,6 +398,8 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  				    netdev_name(ovpn->dev));
>  		goto drop;
>  	}
> +	/* dst was needed for peer selection - it can now be dropped */
> +	skb_dst_drop(skb);
>  
>  	ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb->len);
>  	ovpn_send(ovpn, skb_list.next, peer);
> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
> index 0acb0934c1be..dcc094bf3ade 100644
> --- a/drivers/net/ovpn/main.c
> +++ b/drivers/net/ovpn/main.c
> @@ -157,6 +157,11 @@ static void ovpn_setup(struct net_device *dev)
>  	dev->type = ARPHRD_NONE;
>  	dev->flags = IFF_POINTOPOINT | IFF_NOARP;
>  	dev->priv_flags |= IFF_NO_QUEUE;
> +	/* when routing packets to a LAN behind a client, we rely on the
> +	 * route entry that originally brought the packet into ovpn, so
> +	 * don't release it
> +	 */
> +	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;

See commit 0287587884b15041203b3a362d485e1ab1f24445; the above should be

	netif_keep_dst(dev);

and no need to additional comment, as the helper nails it.

Thanks,

Paolo