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

[Jérémy Jean]

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