Message-ID: <ae23ac0f-2ff9-4396-9033-617dc60221eb@openvpn.net>
Date: Wed, 4 Sep 2024 14:07:23 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.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

Hi,

On 02/09/2024 16:42, Sabrina Dubroca wrote:
> 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?

Right. The comment is not correct.

cs->secondary (old_secondary, that is the newest key) is what is 
probably being used by the other peer for sending traffic.
Therefore old_secondary is what is likely to be needed.

However, this is pure speculation and may not be accurate.

The fact that we could sleep before having completed the swap sounds 
like something we want to avoid.
Maybe I should convert this mutex to a spinlock. Its usage is fairly 
contained anyway.

> 
>> + * 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

ACK

> 
>> +	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)

good point - will clear it in both cases.

> 
>> +	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.

No, it is not correct. As you pointed out 'sg' is an actual array so 
should not be free'd.

This is a leftover of the first version where 'sg' was kmalloc'd.

FTR: this restructuring is the result of having tested 
encryption/decryption with pcrypt: sg, that is passed to the crypto 
code, was initially allocated on the stack, which was obviously not 
working for async crypto.
The solution was to make it part of the skb CB area, so that it can be 
carried around until crypto is done.

> 
>> +		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?

ouch. you're right.
I should better leave it alone and let ovpn_encrypt_post take care of 
free'ing it at the end.

> 
>> +		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.

yap

> 
>> +		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.

I agree, although I am pretty sure I moved it here in the past for a reason.
Maybe code changes eliminated that reason.

anyway, thanks for spotting this. I'll definitely move it to the _post 
functions.

> 
>> +	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

ACK

> 
> 
> [...]
>> -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.

right. we may have hit a return before kmalloc'ing it.

> 
>> +
>> +	/* 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.

Thanks. Will reworka bit the error path here.

> 
>>   	}
>>   	/* 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);
>>   }
> 

This patch was basically re-written after realizing that the async 
crypto path was not working as expected, therefore sorry if there were 
some "kinda obvious" mistakes.

Thanks a lot for your review.


-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists