(just a few nits here)
2024-12-11, 22:15:13 +0100, Antonio Quartulli wrote:
> +static inline struct ovpn_crypto_key_slot *
> +ovpn_crypto_key_id_to_slot(const struct ovpn_crypto_state *cs, u8 key_id)
> +{
> + struct ovpn_crypto_key_slot *ks;
> + u8 idx;
> +
> + if (unlikely(!cs))
> + return NULL;
> +
> + rcu_read_lock();
> + idx = cs->primary_idx;
> + ks = rcu_dereference(cs->slots[idx]);
> + if (ks && ks->key_id == key_id) {
> + if (unlikely(!ovpn_crypto_key_slot_hold(ks)))
> + ks = NULL;
> + goto out;
> + }
> +
> + ks = rcu_dereference(cs->slots[idx ^ 1]);
nit: for consistency with the other uses of the secondary slot, I'd
switch that to cs->slots[!idx]
[...]
> +int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
> + struct sk_buff *skb)
> +{
[...]
> + /* add packet op as head of additional data */
> + op = ovpn_opcode_compose(OVPN_DATA_V2, ks->key_id, peer->id);
> + __skb_push(skb, OVPN_OPCODE_SIZE);
> + BUILD_BUG_ON(sizeof(op) != OVPN_OPCODE_SIZE);
> + *((__force __be32 *)skb->data) = htonl(op);
> +
> + /* AEAD Additional data */
> + sg_set_buf(sg, skb->data, OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE);
You could add
#define OVPN_AAD_SIZE (OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE)
and then use it in a few places in ovpn_aead_encrypt and
ovpn_aead_decrypt.
[...]
> +int ovpn_aead_decrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
> + struct sk_buff *skb)
> +{
> + const unsigned int tag_size = crypto_aead_authsize(ks->decrypt);
> + int ret, payload_len, nfrags;
> + unsigned int payload_offset;
> + struct aead_request *req;
> + struct sk_buff *trailer;
> + struct scatterlist *sg;
> + u8 iv[OVPN_NONCE_SIZE];
> + unsigned int sg_len;
> +
> + payload_offset = OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE + tag_size;
OVPN_AAD_SIZE + tag_size
[...]
> + sg_init_table(sg, nfrags + 2);
> +
> + /* packet op is head of additional data */
> + sg_len = OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE;
This variable can probably be dropped if you add OVPN_AAD_SIZE.
[...]
> + /* setup async crypto operation */
> + aead_request_set_tfm(req, ks->decrypt);
> + aead_request_set_callback(req, 0, ovpn_decrypt_post, skb);
> + aead_request_set_crypt(req, sg, sg, payload_len + tag_size, iv);
> +
> + aead_request_set_ad(req, OVPN_NONCE_WIRE_SIZE + OVPN_OPCODE_SIZE);
and this op is flipped but it's still OVPN_AAD_SIZE.
> +
> + /* decrypt it */
> + return crypto_aead_decrypt(req);
> +free_sg:
> + kfree(ovpn_skb_cb(skb)->sg);
> + ovpn_skb_cb(skb)->sg = NULL;
> + return ret;
> +}
> +
[...]
> @@ -80,7 +83,10 @@ static void ovpn_peer_release_rcu(struct rcu_head *head)
> */
> static void ovpn_peer_release(struct ovpn_peer *peer)
> {
> + ovpn_crypto_state_release(&peer->crypto);
> + spin_lock_bh(&peer->lock);
> ovpn_bind_reset(peer, NULL);
At this point in the series, ovpn_bind_reset also tries to take
peer->lock (gets fixed in the "peer floating" patch).
(but if you're tired of moving chunks around, I can live with it)
> + spin_unlock_bh(&peer->lock);
> netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
> call_rcu(&peer->rcu, ovpn_peer_release_rcu);
> }
--
Sabrina
