Re: [PATCH net v2] tipc: protect received keys from concurrent flush

["Jérémy Jean" <jeremy.jean@oss.cyber.gouv.fr>]

Hello,

On 2026-10-05 04:14, Tung Quang Nguyen wrote:
>> 	/* Attach it to the crypto */
>> 	if (likely(!rc)) {
>> -		rc = tipc_crypto_key_attach(c, aead, 0, master_key);
>> +		spin_lock_bh(&c->lock);
>> +		if (ukey == c->skey_in_use && c->skey != ukey)
>> +			rc = -ECANCELED;
> 
> This check should be done earlier, before calling tipc_ahead_init() ?

Would moving the check before tipc_aead_init() be enough, given that a
flush can still happen while AEAD setup is running and before the key
is attached?

Indeed, the race goes something like:
1. Worker starts tipc_aead_init()
2. Flush clears rx->skey
3. Worker finishes AEAD setup

AFAIU, we still need the check under c->lock, immediately before
tipc_crypto_key_attach(), so that a revoked key cannot be published
after flush.

> 
>> +		else
>> +			rc = tipc_crypto_key_attach(c, aead, 0, master_key);
>> +		spin_unlock_bh(&c->lock);
>> 		if (rc < 0)
>> 			tipc_aead_free(&aead->rcu);
>> 	}
>> @@ -1151,6 +1158,8 @@ int tipc_crypto_key_init(struct tipc_crypto *c, 
>> struct
>> tipc_aead_key *ukey,
>>  * @pos: desired slot in the crypto key array, = 0 if any!
>>  * @master_key: specify this is a cluster master key
>>  *
>> + * The caller must hold c->lock.
>> + *
>>  * Return: new key id in case of success, otherwise: -EBUSY
>>  */
>> static int tipc_crypto_key_attach(struct tipc_crypto *c, @@ -1158,20 
>> +1167,19
>> @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
>> 				  bool master_key)
>> {
>> 	struct tipc_key key;
>> -	int rc = -EBUSY;
>> 	u8 new_key;
>> 
>> -	spin_lock_bh(&c->lock);
>> +	lockdep_assert_held(&c->lock);
>> 	key = c->key;
>> 	if (master_key) {
>> 		new_key = KEY_MASTER;
>> 		goto attach;
>> 	}
>> 	if (key.active && key.passive)
>> -		goto exit;
>> +		return -EBUSY;
>> 	if (key.pending) {
>> 		if (tipc_aead_users(c->aead[key.pending]) > 0)
>> -			goto exit;
>> +			return -EBUSY;
>> 		/* if (pos): ok with replacing, will be aligned when needed */
>> 		/* Replace it */
>> 		new_key = key.pending;
>> @@ -1201,11 +1209,7 @@ attach:
>> 	c->working = 1;
>> 	c->nokey = 0;
>> 	c->key_master |= master_key;
>> -	rc = new_key;
>> -
>> -exit:
>> -	spin_unlock_bh(&c->lock);
>> -	return rc;
>> +	return new_key;
> 
> Remove lock from tipc_crypto_key_attach() is enough. Why adding many 
> irrelevant changes that do not seem necessary ?

Those extra changes in tipc_crypto_key_attach() are more cleanup than
anything else; I preferred the direct returns since the function no
longer owns the lock, but I can reduce the patch and keep the exit
label.

Thanks,
Jérémy