[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zoz6FdiZ64bQhU0c@hog>
Date: Tue, 9 Jul 2024 10:51:33 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, ryazanov.s.a@...il.com,
pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v5 12/25] ovpn: implement packet processing
2024-06-27, 15:08:30 +0200, Antonio Quartulli wrote:
> +/* removes the primary key from the crypto context */
> +void ovpn_crypto_kill_primary(struct ovpn_crypto_state *cs)
> +{
> + struct ovpn_crypto_key_slot *ks;
> +
> + mutex_lock(&cs->mutex);
> + ks = rcu_replace_pointer(cs->primary, NULL,
> + lockdep_is_held(&cs->mutex));
Should there be a check that we're killing the key that has expired
and not some other key? I'm wondering if this could happen:
ovpn_encrypt_one
ovpn_aead_encrypt
ovpn_pktid_xmit_next
seq_num reaches threshold
returns -ERANGE
returns -ERANGE
ovpn_crypto_key_slots_swap
replaces cs->primary with cs->secondary
ovpn_encrypt_post
ret = -ERANGE
ovpn_crypto_kill_primary
kills the freshly installed primary key
> + ovpn_crypto_key_slot_put(ks);
> + mutex_unlock(&cs->mutex);
> +}
> +
[...]
> +static void ovpn_aead_encrypt_done(void *data, int ret)
> +{
> + struct sk_buff *skb = data;
> +
> + aead_request_free(ovpn_skb_cb(skb)->req);
> + ovpn_encrypt_post(skb, ret);
> +}
> +
> +int ovpn_aead_encrypt(struct ovpn_crypto_key_slot *ks, struct sk_buff *skb,
> + u32 peer_id)
> +{
> + const unsigned int tag_size = crypto_aead_authsize(ks->encrypt);
> + const unsigned int head_size = ovpn_aead_encap_overhead(ks);
> + struct scatterlist sg[MAX_SKB_FRAGS + 2];
> + DECLARE_CRYPTO_WAIT(wait);
unused? (also in _decrypt)
[...]
> +
> + req = aead_request_alloc(ks->encrypt, GFP_ATOMIC);
> + if (unlikely(!req))
> + return -ENOMEM;
> +
> + /* setup async crypto operation */
> + aead_request_set_tfm(req, ks->encrypt);
> + aead_request_set_callback(req, 0, ovpn_aead_encrypt_done, NULL);
NULL? That should be skb, ovpn_aead_encrypt_done needs it (same for
decrypt).
I suspect you haven't triggered the async path in testing. For that,
you can use crconf:
git clone https://git.code.sf.net/p/crconf/code
cd code && make
./src/crconf add driver 'pcrypt(generic-gcm-aesni)' type 3 priority 10000
Then all packets encrypted with gcm(aes) should go through the async
code.
> + aead_request_set_crypt(req, sg, sg, skb->len - head_size, iv);
> + aead_request_set_ad(req, OVPN_OP_SIZE_V2 + NONCE_WIRE_SIZE);
> +
> + ovpn_skb_cb(skb)->req = req;
> + ovpn_skb_cb(skb)->ks = ks;
> +
> + /* encrypt it */
> + return crypto_aead_encrypt(req);
> +}
[...]
> @@ -77,14 +133,45 @@ static void ovpn_decrypt_post(struct sk_buff *skb, int ret)
> /* pick next packet from RX queue, decrypt and forward it to the device */
> void ovpn_recv(struct ovpn_peer *peer, struct sk_buff *skb)
> {
> + struct ovpn_crypto_key_slot *ks;
> + 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);
This takes a reference on the keyslot (ovpn_crypto_key_slot_hold), but
I don't see it getting released in ovpn_decrypt_post. In
ovpn_encrypt_post you're adding a ovpn_crypto_key_slot_put (to match
ovpn_crypto_key_slot_primary), but nothing equivalent in
ovpn_decrypt_post?
> + 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);
> + dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
> + kfree_skb(skb);
> + return;
> + }
> +
> ovpn_skb_cb(skb)->peer = peer;
> - ovpn_decrypt_post(skb, 0);
> + ovpn_decrypt_post(skb, ovpn_aead_decrypt(ks, skb));
> }
--
Sabrina
Powered by blists - more mailing lists