Re: [PATCH net-next v6 11/25] ovpn: implement basic RX path (UDP)

[Sabrina Dubroca <sd@queasysnail.net>]

2024-08-27, 14:07:51 +0200, Antonio Quartulli wrote:
> +static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
> +{
> +	/* we can't guarantee the packet wasn't corrupted before entering the
> +	 * VPN, therefore we give other layers a chance to check that
> +	 */
> +	skb->ip_summed = CHECKSUM_NONE;
> +
> +	/* skb hash for transport packet no longer valid after decapsulation */
> +	skb_clear_hash(skb);
> +
> +	/* post-decrypt scrub -- prepare to inject encapsulated packet onto the
> +	 * interface, based on __skb_tunnel_rx() in dst.h
> +	 */
> +	skb->dev = peer->ovpn->dev;
> +	skb_set_queue_mapping(skb, 0);
> +	skb_scrub_packet(skb, true);
> +
> +	skb_reset_network_header(skb);
> +	skb_reset_transport_header(skb);
> +	skb_probe_transport_header(skb);
> +	skb_reset_inner_headers(skb);
> +
> +	memset(skb->cb, 0, sizeof(skb->cb));
> +
> +	/* cause packet to be "received" by the interface */
> +	if (likely(gro_cells_receive(&peer->ovpn->gro_cells,
> +				     skb) == NET_RX_SUCCESS))
> +		/* update RX stats with the size of decrypted packet */
> +		dev_sw_netstats_rx_add(peer->ovpn->dev, skb->len);

I don't think accessing skb->len after passing the skb to
gro_cells_receive is safe, see
c7cc9200e9b4 ("macsec: avoid use-after-free in macsec_handle_frame()")


[...]
>  static void ovpn_struct_free(struct net_device *net)
>  {
> +	struct ovpn_struct *ovpn = netdev_priv(net);
> +
> +	gro_cells_destroy(&ovpn->gro_cells);
> +	rcu_barrier();

What's the purpose of this rcu_barrier? I expect it in module_exit,
not when removing one netdevice.


> diff --git a/drivers/net/ovpn/skb.h b/drivers/net/ovpn/skb.h
> index 7966a10d915f..e070fe6f448c 100644
> --- a/drivers/net/ovpn/skb.h
> +++ b/drivers/net/ovpn/skb.h
> @@ -18,10 +18,7 @@
>  #include <linux/types.h>
>  
>  struct ovpn_cb {
> -	struct aead_request *req;
>  	struct ovpn_peer *peer;
> -	struct ovpn_crypto_key_slot *ks;
> -	unsigned int payload_offset;

Squashed into the wrong patch?


[...]
> +struct ovpn_struct *ovpn_from_udp_sock(struct sock *sk)
> +{
> +	struct ovpn_socket *ovpn_sock;
> +
> +	if (unlikely(READ_ONCE(udp_sk(sk)->encap_type) != UDP_ENCAP_OVPNINUDP))
> +		return NULL;
> +
> +	ovpn_sock = rcu_dereference_sk_user_data(sk);

[1]

> +	if (unlikely(!ovpn_sock))
> +		return NULL;
> +
> +	/* make sure that sk matches our stored transport socket */
> +	if (unlikely(!ovpn_sock->sock || sk != ovpn_sock->sock->sk))
> +		return NULL;
> +
> +	return ovpn_sock->ovpn;
> +}


> +static int ovpn_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct ovpn_peer *peer = NULL;
> +	struct ovpn_struct *ovpn;
> +	u32 peer_id;
> +	u8 opcode;
> +
> +	ovpn = ovpn_from_udp_sock(sk);
> +	if (unlikely(!ovpn)) {
> +		net_err_ratelimited("%s: cannot obtain ovpn object from UDP socket\n",
> +				    __func__);
> +		goto drop;
> +	}
[...]
> +	/* pop off outer UDP header */
> +	__skb_pull(skb, sizeof(struct udphdr));
> +	ovpn_recv(peer, skb);
> +	return 0;
> +
> +drop:
> +	if (peer)
> +		ovpn_peer_put(peer);
> +	dev_core_stats_rx_dropped_inc(ovpn->dev);

If we get here from the first goto, ovpn is NULL. You could add a
drop_noovpn label right here to just do the free+return.

> +	kfree_skb(skb);
> +	return 0;
> +}
> +
>  /**
>   * ovpn_udp4_output - send IPv4 packet over udp socket
>   * @ovpn: the openvpn instance
> @@ -257,8 +342,13 @@ void ovpn_udp_send_skb(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
>   */
>  int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn)
>  {
> +	struct udp_tunnel_sock_cfg cfg = {
> +		.sk_user_data = ovpn,
> +		.encap_type = UDP_ENCAP_OVPNINUDP,
> +		.encap_rcv = ovpn_udp_encap_recv,
> +	};
>  	struct ovpn_socket *old_data;
> -	int ret = 0;
> +	int ret;
>  
>  	/* sanity check */
>  	if (sock->sk->sk_protocol != IPPROTO_UDP) {
> @@ -272,6 +362,7 @@ int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn)
>  	if (!old_data) {
>  		/* socket is currently unused - we can take it */
>  		rcu_read_unlock();
> +		setup_udp_tunnel_sock(sock_net(sock->sk), sock, &cfg);

This will set sk_user_data to the ovpn_struct, but ovpn_from_udp_sock
expects the ovpn_socket [1], which is stored into sk_user_data a
little bit later by ovpn_socket_new. If a packet reaches
ovpn_udp_encap_recv -> ovpn_from_udp_sock before ovpn_socket_new
overwrites sk_user_data, bad things probably happen.

-- 
Sabrina