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

[Tung Quang Nguyen]

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

pw-bot: cr