On 02/09/2024 13:22, Sabrina Dubroca wrote:

2024-08-27, 14:07:51 +0200, Antonio Quartulli wrote:

quoted

+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()")

Thanks for spotting this! It's basically the same issue (but symmetric) 
as patch 10/25.

[...]

quoted

  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.

Good question.
I presume it's a leftover from previous tests.
I think it's harmless, but it should not be needed at all.

I will remove it.

quoted

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?

Darn yes. Sorry for this.

[...]

quoted

+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]

quoted

+	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;
+}

quoted

+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;
+	}

[...]

quoted

+	/* 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.

Right.
Weird though that no static analysis tool complained about ovpn possibly 
being NULL.

Will add the extra label.

quoted

+	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.

Wow - this is a very nice catch.

I think this wrong cfg.sk_user_data initialization was there this since 
the first prototype, but it just passed unnoticed.

I will drop the field initialization, so that the sk_user_data stays 
NULL until it gets assigned the ovpn_sock.


Thanks a lot!

Cheers,


-- 
Antonio Quartulli
OpenVPN Inc.