Re: [PATCH net-next v3 11/24] ovpn: implement packet processing

Sabrina Dubroca
2024-05-12T08:46:34Z

2024-05-06, 03:16:24 +0200, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c
> index c1f842c06e32..7240d1036fb7 100644
> --- a/drivers/net/ovpn/bind.c
> +++ b/drivers/net/ovpn/bind.c
> @@ -13,6 +13,7 @@
> #include "ovpnstruct.h"
> #include "io.h"
> #include "bind.h"
> +#include "packet.h"
> #include "peer.h"

You have a few hunks like that in this patch, adding an include to a
file that is otherwise not being modified. That's odd.

> diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
> new file mode 100644
> index 000000000000..98ef1ceb75e0
> --- /dev/null
> +++ b/drivers/net/ovpn/crypto.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* OpenVPN data channel offload
> + *
> + * Copyright (C) 2020-2024 OpenVPN, Inc.
> + *
> + * Author: James Yonan <james@openvpn.net>
> + * Antonio Quartulli <antonio@openvpn.net>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/net.h>
> +#include <linux/netdevice.h>
> +//#include <linux/skbuff.h>

That's also odd :)

[...]
> +/* Reset the ovpn_crypto_state object in a way that is atomic
> + * to RCU readers.
> + */
> +int ovpn_crypto_state_reset(struct ovpn_crypto_state *cs,
> + const struct ovpn_peer_key_reset *pkr)
> + __must_hold(cs->mutex)
> +{
> + struct ovpn_crypto_key_slot *old = NULL;
> + struct ovpn_crypto_key_slot *new;
> +
> + lockdep_assert_held(&cs->mutex);
> +
> + new = ovpn_aead_crypto_key_slot_new(&pkr->key);

This doesn't need the lock to be held, you could move the lock to a
smaller section (only around the pointer swap).

> + if (IS_ERR(new))
> + return PTR_ERR(new);
> +
> + switch (pkr->slot) {
> + case OVPN_KEY_SLOT_PRIMARY:
> + old = rcu_replace_pointer(cs->primary, new,
> + lockdep_is_held(&cs->mutex));
> + break;
> + case OVPN_KEY_SLOT_SECONDARY:
> + old = rcu_replace_pointer(cs->secondary, new,
> + lockdep_is_held(&cs->mutex));
> + break;
> + default:
> + goto free_key;

And validating pkr->slot before alloc could avoid a pointless
alloc/free (and simplify the code: once _new() has succeeded, no
failure can occur anymore).

> + }
> +
> + if (old)
> + ovpn_crypto_key_slot_put(old);
> +
> + return 0;
> +free_key:
> + ovpn_crypto_key_slot_put(new);
> + return -EINVAL;
> +}
> +
> +void ovpn_crypto_key_slot_delete(struct ovpn_crypto_state *cs,
> + enum ovpn_key_slot slot)
> +{
> + struct ovpn_crypto_key_slot *ks = NULL;
> +
> + mutex_lock(&cs->mutex);
> + switch (slot) {
> + case OVPN_KEY_SLOT_PRIMARY:
> + ks = rcu_replace_pointer(cs->primary, NULL,
> + lockdep_is_held(&cs->mutex));
> + break;
> + case OVPN_KEY_SLOT_SECONDARY:
> + ks = rcu_replace_pointer(cs->secondary, NULL,
> + lockdep_is_held(&cs->mutex));
> + break;
> + default:
> + pr_warn("Invalid slot to release: %u\n", slot);
> + break;
> + }
> + mutex_unlock(&cs->mutex);
> +
> + if (!ks) {
> + pr_debug("Key slot already released: %u\n", slot);

This will also be printed in case of an invalid argument, which would
be mildly confusing.

> + return;
> + }
> + pr_debug("deleting key slot %u, key_id=%u\n", slot, ks->key_id);
> +
> + ovpn_crypto_key_slot_put(ks);
> +}

> +static struct ovpn_crypto_key_slot *
> +ovpn_aead_crypto_key_slot_init(enum ovpn_cipher_alg alg,
> + const unsigned char *encrypt_key,
> + unsigned int encrypt_keylen,
> + const unsigned char *decrypt_key,
> + unsigned int decrypt_keylen,
> + const unsigned char *encrypt_nonce_tail,
> + unsigned int encrypt_nonce_tail_len,
> + const unsigned char *decrypt_nonce_tail,
> + unsigned int decrypt_nonce_tail_len,
> + u16 key_id)
> +{
[...]
> +
> + if (sizeof(struct ovpn_nonce_tail) != encrypt_nonce_tail_len ||
> + sizeof(struct ovpn_nonce_tail) != decrypt_nonce_tail_len) {
> + ret = -EINVAL;
> + goto destroy_ks;
> + }

Those checks could be done earlier, before bothering with any
allocations.

> +
> + memcpy(ks->nonce_tail_xmit.u8, encrypt_nonce_tail,
> + sizeof(struct ovpn_nonce_tail));
> + memcpy(ks->nonce_tail_recv.u8, decrypt_nonce_tail,
> + sizeof(struct ovpn_nonce_tail));
> +
> + /* init packet ID generation/validation */
> + ovpn_pktid_xmit_init(&ks->pid_xmit);
> + ovpn_pktid_recv_init(&ks->pid_recv);
> +
> + return ks;
> +
> +destroy_ks:
> + ovpn_aead_crypto_key_slot_destroy(ks);
> + return ERR_PTR(ret);
> +}
> +
> +struct ovpn_crypto_key_slot *
> +ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc)
> +{
> + return ovpn_aead_crypto_key_slot_init(kc->cipher_alg,
> + kc->encrypt.cipher_key,
> + kc->encrypt.cipher_key_size,
> + kc->decrypt.cipher_key,
> + kc->decrypt.cipher_key_size,
> + kc->encrypt.nonce_tail,
> + kc->encrypt.nonce_tail_size,
> + kc->decrypt.nonce_tail,
> + kc->decrypt.nonce_tail_size,
> + kc->key_id);
> +}

Why the wrapper? You could just call ovpn_aead_crypto_key_slot_init
directly.

> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index 9935a863bffe..66a4c551c191 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -110,6 +114,27 @@ int ovpn_napi_poll(struct napi_struct *napi, int budget)
> return work_done;
> }
>
> +/* Return IP protocol version from skb header.
> + * Return 0 if protocol is not IPv4/IPv6 or cannot be read.
> + */
> +static __be16 ovpn_ip_check_protocol(struct sk_buff *skb)

nit: if you put this function higher up in the patch that introduced
it, you wouldn't have to move it now

> +{
> + __be16 proto = 0;
> +
> + /* skb could be non-linear, make sure IP header is in non-fragmented
> + * part
> + */
> + if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
> + return 0;
> +
> + if (ip_hdr(skb)->version == 4)
> + proto = htons(ETH_P_IP);
> + else if (ip_hdr(skb)->version == 6)
> + proto = htons(ETH_P_IPV6);
> +
> + return proto;
> +}
> +
> /* Entry point for processing an incoming packet (in skb form)
> *
> * Enqueue the packet and schedule RX consumer.
> @@ -132,7 +157,81 @@ int ovpn_recv(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
>
> static int ovpn_decrypt_one(struct ovpn_peer *peer, struct sk_buff *skb)
> {
> - return true;

I missed that in the RX patch, true isn't an int :)
Were you intending this function to be bool like ovpn_encrypt_one?
Since you're not actually using the returned value in the caller, it
would be reasonable, but you'd have to convert all the <0 error values
to bool.

> + struct ovpn_peer *allowed_peer = NULL;
> + struct ovpn_crypto_key_slot *ks;
> + __be16 proto;
> + int ret = -1;
> + u8 key_id;
> +
> + /* get the key slot matching the key Id in the received packet */
> + key_id = ovpn_key_id_from_skb(skb);
> + ks = ovpn_crypto_key_id_to_slot(&peer->crypto, key_id);
> + if (unlikely(!ks)) {
> + net_info_ratelimited("%s: no available key for peer %u, key-id: %u\n",
> + peer->ovpn->dev->name, peer->id, key_id);
> + goto drop;
> + }
> +
> + /* decrypt */
> + ret = ovpn_aead_decrypt(ks, skb);
> +
> + ovpn_crypto_key_slot_put(ks);
> +
> + if (unlikely(ret < 0)) {
> + net_err_ratelimited("%s: error during decryption for peer %u, key-id %u: %d\n",
> + peer->ovpn->dev->name, peer->id, key_id,
> + ret);
> + goto drop;
> + }
> +
> + /* check if this is a valid datapacket that has to be delivered to the
> + * tun interface

s/tun/ovpn/ ?

> + */
> + skb_reset_network_header(skb);
> + proto = ovpn_ip_check_protocol(skb);
> + if (unlikely(!proto)) {
> + /* check if null packet */
> + if (unlikely(!pskb_may_pull(skb, 1))) {
> + netdev_dbg(peer->ovpn->dev,
> + "NULL packet received from peer %u\n",
> + peer->id);
> + ret = -EINVAL;
> + goto drop;
> + }
> +
> + netdev_dbg(peer->ovpn->dev,
> + "unsupported protocol received from peer %u\n",
> + peer->id);
> +
> + ret = -EPROTONOSUPPORT;
> + goto drop;
> + }
> + skb->protocol = proto;
> +
> + /* perform Reverse Path Filtering (RPF) */
> + allowed_peer = ovpn_peer_get_by_src(peer->ovpn, skb);
> + if (unlikely(allowed_peer != peer)) {
> + if (skb_protocol_to_family(skb) == AF_INET6)
> + net_warn_ratelimited("%s: RPF dropped packet from peer %u, src: %pI6c\n",
> + peer->ovpn->dev->name, peer->id,
> + &ipv6_hdr(skb)->saddr);
> + else
> + net_warn_ratelimited("%s: RPF dropped packet from peer %u, src: %pI4\n",
> + peer->ovpn->dev->name, peer->id,
> + &ip_hdr(skb)->saddr);
> + ret = -EPERM;
> + goto drop;
> + }

Have you considered holding rcu_read_lock around this whole RPF check?
It would avoid taking a reference on the peer just to release it 3
lines later. And the same could likely be done for some of the other
ovpn_peer_get_* lookups too.

> + ret = ptr_ring_produce_bh(&peer->netif_rx_ring, skb);
> +drop:
> + if (likely(allowed_peer))
> + ovpn_peer_put(allowed_peer);
> +
> + if (unlikely(ret < 0))
> + kfree_skb(skb);
> +
> + return ret;

Mixing the drop/success returns looks kind of strange. This would be a
bit simpler:

ovpn_peer_put(allowed_peer);
return ptr_ring_produce_bh(&peer->netif_rx_ring, skb);

drop:
if (allowed_peer)
ovpn_peer_put(allowed_peer);
kfree_skb(skb);
return ret;

> diff --git a/drivers/net/ovpn/packet.h b/drivers/net/ovpn/packet.h
> index 7ed146f5932a..e14c9bf464f7 100644
> --- a/drivers/net/ovpn/packet.h
> +++ b/drivers/net/ovpn/packet.h
> @@ -10,7 +10,7 @@
> #ifndef _NET_OVPN_PACKET_H_
> #define _NET_OVPN_PACKET_H_
>
> -/* When the OpenVPN protocol is ran in AEAD mode, use
> +/* When the OpenVPN protocol is run in AEAD mode, use

nit: that typo came in earlier

--
Sabrina