(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);
}