Message-ID: <ZtXOw-NcL9lvwWa8@hog>
Date: Mon, 2 Sep 2024 16:42:11 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v6 12/25] ovpn: implement packet processing

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

Powered by blists - more mailing lists