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

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

On 2026-10-02 14:05, Tung Quang Nguyen wrote:
>> Subject: Re: [PATCH net] tipc: protect received keys from concurrent 
>> flush
>> 
>> Thank you for your contribution! Sashiko AI review found 1 potential
>> issue(s) to consider.
>> 
>> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0
>> 
>> If you believe none of the issues are legitimate, please reply to each 
>> issue of
>> Medium severity or higher and explain why the AI is wrong.
>> If any of them are legitimate and you plan to respin, please let 
>> patchwork
>> know by including "pw-bot: cr" as a separate line at the end of your 
>> reply (one
>> such reply per series is enough).
>> 
>> - [Medium] The patch fixes the rx->skey UAF/double-free, but the flush 
>> it
>>  protects can now be silently undone.
>> 
>> Pre-existing issues:
>> - [High] This problem wasn't introduced by this patch, but an RX 
>> delayed
>>  work can re-arm after netns teardown has flushed it and then fire on 
>> a…
>> 
>> --
>> 
>> tipc: protect received keys from concurrent flush
>> 
>> This patch adds an skey_in_use flag to struct tipc_crypto. The RX 
>> worker sets it
>> under rx->lock while it attaches rx->skey, and
>> tipc_crypto_key_flush() skips freeing the key while the flag is set. 
>> The goal is to
>> close a KASAN-reported use-after-free and double-free that happens 
>> when key
>> exchange races with key flush.
>> 
>>> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c index
>>> 4409bdb70fe98..eab2a66e22333 100644
>>> --- a/net/tipc/crypto.c
>>> +++ b/net/tipc/crypto.c
>> 
>> [ ... ]
>> 
>>> @@ -1214,8 +1216,11 @@ void tipc_crypto_key_flush(struct tipc_crypto 
>>> *c)
>>>  		rx = c;
>>>  		tx = tipc_net(rx->net)->crypto_tx;
>>>  		if (cancel_delayed_work(&rx->work)) {
>>> -			kfree_sensitive(rx->skey);
>>> -			rx->skey = NULL;
>>> +			/* A previous invocation may still be using the key. */
>>> +			if (!rx->skey_in_use) {
>>> +				kfree_sensitive(rx->skey);
>>> +				rx->skey = NULL;
>>> +			}
>>>  			atomic_xchg(&rx->key_distr, 0);
>>>  			tipc_node_put(rx->node);
>>>  		}
>> 
>> [Severity: Medium]
>> With this skip, can the still-running worker undo the flush after it 
>> returns?
>> 
>> Nothing tells the in-flight tipc_crypto_work_rx() that a flush 
>> happened, so this
>> sequence looks possible:
>> 
>> CPU1 (tipc_crypto_work_rx)            CPU2
>> spin_lock_bh(&rx->lock);
>> rx->skey_in_use = true;
>> spin_unlock_bh(&rx->lock);
>>                                      tipc_crypto_key_synch()
>>                                        re-queues rx->work
>>                                      tipc_crypto_key_flush()
>>                                        cancel_delayed_work() == true
>>                                        skips kfree, rx->skey stays set
>>                                        c->flags = 0;
>>                                        tipc_crypto_key_set_state(c, 0, 
>> 0, 0);
>>                                        tipc_crypto_key_detach() on all 
>> keys
>>                                        spin_unlock_bh(&c->lock); 
>> tipc_crypto_key_init(rx, rx-
>>> skey, ...)
>>  tipc_crypto_key_attach()
>>    tipc_aead_rcu_replace(c->aead[new_key], aead, &c->lock);
>>    c->working = 1;
>>    c->nokey = 0;
>> 
>> In this case the TIPC_NL_KEY_FLUSH handler 
>> (__tipc_nl_node_flush_key())
>> returns 0. The peer's AEAD is still installed at key_next(0) and the 
>> RX crypto is
>> re-enabled.
>> 
>> There is also the retry path. Suppose tipc_crypto_key_attach() had 
>> already
>> returned -EBUSY, or tipc_aead_init() had returned -ENOMEM, before the 
>> flush.
>> The worker then keeps rx->skey, clears skey_in_use and re-queues 
>> itself.
>> queue_delayed_work() succeeds because the flush cancelled the pending
>> instance, so the pre-flush key gets attached about 5 seconds later.
>> 
>> Until that happens, tipc_crypto_key_rcv() rejects new keys from the 
>> peer
>> because the stale rx->skey is still non-NULL:
>> 
>> 	if (unlikely(rx->skey || (key_gen == rx->key_gen && rx->key.keys))) {
>> 		pr_err("%s: key existed <%p>, gen %d vs %d\n", rx->name,
>> 
>> tipc_crypto_rcv() also counts the stale rx->skey when it computes
>> rx->nokey:
>> 
>> 	rx->nokey = !(rx->skey ||
>> 
>> Before this patch, the cancel_delayed_work() == true case freed and 
>> cleared
>> rx->skey (with the use-after-free), so the retry had nothing to 
>> attach. With this
>> patch, that case behaves like the existing
>> cancel_delayed_work() == false case: the flush does not revoke the key 
>> that is
>> in flight. The commit message doesn't mention this.
>> 
>> Would a flushed or generation marker help? The worker would re-check 
>> it
>> under rx->lock before attaching or re-queueing, and free the key if a 
>> flush had
>> happened. That keeps the use-after-free fix and still honours the 
>> flush.
>> 
> Agree.

Me too, I'm working on a v2.

Jérémy