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

+/* this swap is not atomic, but there will be a very short time frame where the

Since we're under a mutex, I think we might get put to sleep for a
not-so-short time frame.

+ * old_secondary key won't be available. This should not be a big deal as most

I could be misreading the code, but isn't it old_primary that's
unavailable during the swap? rcu_replace_pointer overwrites
cs->primary, so before the final assign, both slots contain
old_secondary?

+ * likely both peers are already using the new primary at this point.
+ */
+void ovpn_crypto_key_slots_swap(struct ovpn_crypto_state *cs)
+{
+	const struct ovpn_crypto_key_slot *old_primary, *old_secondary;
+
+	mutex_lock(&cs->mutex);
+
+	old_secondary = rcu_dereference_protected(cs->secondary,
+						  lockdep_is_held(&cs->mutex));
+	old_primary = rcu_replace_pointer(cs->primary, old_secondary,
+					  lockdep_is_held(&cs->mutex));
+	rcu_assign_pointer(cs->secondary, old_primary);
+
+	pr_debug("key swapped: %u <-> %u\n",
+		 old_primary ? old_primary->key_id : 0,
+		 old_secondary ? old_secondary->key_id : 0);
+
+	mutex_unlock(&cs->mutex);
+}

[...]

+int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
+		      struct sk_buff *skb)
+{
+	const unsigned int tag_size = crypto_aead_authsize(ks->encrypt);
+	const unsigned int head_size = ovpn_aead_encap_overhead(ks);
+	DECLARE_CRYPTO_WAIT(wait);

nit: unused

+	struct aead_request *req;
+	struct sk_buff *trailer;
+	struct scatterlist *sg;
+	u8 iv[NONCE_SIZE];
+	int nfrags, ret;
+	u32 pktid, op;
+
+	/* Sample AEAD header format:
+	 * 48000001 00000005 7e7046bd 444a7e28 cc6387b1 64a4d6c1 380275a...
+	 * [ OP32 ] [seq # ] [             auth tag            ] [ payload ... ]
+	 *          [4-byte
+	 *          IV head]
+	 */
+
+	/* check that there's enough headroom in the skb for packet
+	 * encapsulation, after adding network header and encryption overhead
+	 */
+	if (unlikely(skb_cow_head(skb, OVPN_HEAD_ROOM + head_size)))
+		return -ENOBUFS;
+
+	/* get number of skb frags and ensure that packet data is writable */
+	nfrags = skb_cow_data(skb, 0, &trailer);
+	if (unlikely(nfrags < 0))
+		return nfrags;
+
+	if (unlikely(nfrags + 2 > (MAX_SKB_FRAGS + 2)))
+		return -ENOSPC;
+
+	ovpn_skb_cb(skb)->ctx = kmalloc(sizeof(*ovpn_skb_cb(skb)->ctx),
+					GFP_ATOMIC);
+	if (unlikely(!ovpn_skb_cb(skb)->ctx))
+		return -ENOMEM;

I think you should clear skb->cb (or at least ->ctx) at the start of
ovpn_aead_encrypt. I don't think it will be cleaned up by the previous
user, and if we fail before this alloc, we will possibly have bogus
values in ->ctx when we get to kfree(ovpn_skb_cb(skb)->ctx) at the end
of ovpn_encrypt_post.

(Similar comments around cb/ctx freeing and initialization apply to
ovpn_aead_decrypt and ovpn_decrypt_post)

+	sg = ovpn_skb_cb(skb)->ctx->sg;
+
+	/* sg table:
+	 * 0: op, wire nonce (AD, len=OVPN_OP_SIZE_V2+NONCE_WIRE_SIZE),
+	 * 1, 2, 3, ..., n: payload,
+	 * n+1: auth_tag (len=tag_size)
+	 */
+	sg_init_table(sg, nfrags + 2);
+
+	/* build scatterlist to encrypt packet payload */
+	ret = skb_to_sgvec_nomark(skb, sg + 1, 0, skb->len);
+	if (unlikely(nfrags != ret)) {
+		kfree(sg);

This is the only location in this function (and ovpn_encrypt_post)
that frees sg. Is that correct? sg points to an array contained within
->ctx, I don't think you want to free that directly.

+		return -EINVAL;
+	}
+
+	/* append auth_tag onto scatterlist */
+	__skb_push(skb, tag_size);
+	sg_set_buf(sg + nfrags + 1, skb->data, tag_size);
+
+	/* obtain packet ID, which is used both as a first
+	 * 4 bytes of nonce and last 4 bytes of associated data.
+	 */
+	ret = ovpn_pktid_xmit_next(&ks->pid_xmit, &pktid);
+	if (unlikely(ret < 0)) {
+		kfree(ovpn_skb_cb(skb)->ctx);

Isn't that going to cause a double-free when we get to the end of
ovpn_encrypt_post? Or even UAF when we try to get ks/peer at the
start?

+		return ret;
+	}
+
+	/* concat 4 bytes packet id and 8 bytes nonce tail into 12 bytes
+	 * nonce
+	 */
+	ovpn_pktid_aead_write(pktid, &ks->nonce_tail_xmit, iv);
+
+	/* make space for packet id and push it to the front */
+	__skb_push(skb, NONCE_WIRE_SIZE);
+	memcpy(skb->data, iv, NONCE_WIRE_SIZE);
+
+	/* add packet op as head of additional data */
+	op = ovpn_opcode_compose(OVPN_DATA_V2, ks->key_id, peer->id);
+	__skb_push(skb, OVPN_OP_SIZE_V2);
+	BUILD_BUG_ON(sizeof(op) != OVPN_OP_SIZE_V2);
+	*((__force __be32 *)skb->data) = htonl(op);
+
+	/* AEAD Additional data */
+	sg_set_buf(sg, skb->data, OVPN_OP_SIZE_V2 + NONCE_WIRE_SIZE);
+
+	req = aead_request_alloc(ks->encrypt, GFP_ATOMIC);
+	if (unlikely(!req)) {
+		kfree(ovpn_skb_cb(skb)->ctx);

Same here.

+		return -ENOMEM;
+	}
+
+	/* setup async crypto operation */
+	aead_request_set_tfm(req, ks->encrypt);
+	aead_request_set_callback(req, 0, ovpn_aead_encrypt_done, skb);
+	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)->ctx->peer = peer;
+	ovpn_skb_cb(skb)->ctx->req = req;
+	ovpn_skb_cb(skb)->ctx->ks = ks;
+
+	/* encrypt it */
+	return crypto_aead_encrypt(req);
+}
+
+static void ovpn_aead_decrypt_done(void *data, int ret)
+{
+	struct sk_buff *skb = data;
+
+	aead_request_free(ovpn_skb_cb(skb)->ctx->req);

This function only gets called in the async case. Where's the
corresponding aead_request_free in the sync case? (same for encrypt)
This should be moved into ovpn_decrypt_post, I think.

+	ovpn_decrypt_post(skb, ret);
+}
+
+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;
+	DECLARE_CRYPTO_WAIT(wait);

nit: unused


[...]

-static void ovpn_encrypt_post(struct sk_buff *skb, int ret)
+void ovpn_encrypt_post(struct sk_buff *skb, int ret)
 {
-	struct ovpn_peer *peer = ovpn_skb_cb(skb)->peer;
+	struct ovpn_crypto_key_slot *ks = ovpn_skb_cb(skb)->ctx->ks;
+	struct ovpn_peer *peer = ovpn_skb_cb(skb)->ctx->peer;

ovpn_skb_cb(skb)->ctx may not have been set by ovpn_aead_encrypt.

+
+	/* encryption is happening asynchronously. This function will be
+	 * called later by the crypto callback with a proper return value
+	 */
+	if (unlikely(ret == -EINPROGRESS))
+		return;
 
 	if (unlikely(ret < 0))
 		goto err;
 
 	skb_mark_not_on_list(skb);
 
+	kfree(ovpn_skb_cb(skb)->ctx);
+
 	switch (peer->sock->sock->sk->sk_protocol) {
 	case IPPROTO_UDP:
 		ovpn_udp_send_skb(peer->ovpn, peer, skb);
 		break;
 	default:
 		/* no transport configured yet */
 		goto err;

ovpn_skb_cb(skb)->ctx has just been freed before this switch, and here
we jump to err and free it again.

 	}
 	/* skb passed down the stack - don't free it */
 	skb = NULL;
 err:
 	if (unlikely(skb)) {
 		dev_core_stats_tx_dropped_inc(peer->ovpn->dev);
-		kfree_skb(skb);
+		kfree(ovpn_skb_cb(skb)->ctx);
 	}
+	kfree_skb(skb);
+	ovpn_crypto_key_slot_put(ks);
 	ovpn_peer_put(peer);
 }

-- 
Sabrina