Re: [PATCH net] ipv6: sr: fix destroy of seg6_hmac_info to prevent HMAC data leak

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Aug 25, 2025 at 12:33:26PM -0700, Eric Dumazet wrote:
> On Mon, Aug 25, 2025 at 12:08 PM Andrea Mayer <andrea.mayer@uniroma2.it> wrote:
> >
> > The seg6_hmac_info structure stores information related to SRv6 HMAC
> > configurations, including the secret key, HMAC ID, and hashing algorithm
> > used to authenticate and secure SRv6 packets.
> >
> > When a seg6_hmac_info object is no longer needed, it is destroyed via
> > seg6_hmac_info_del(), which eventually calls seg6_hinfo_release(). This
> > function uses kfree_rcu() to safely deallocate memory after an RCU grace
> > period has elapsed.
> > The kfree_rcu() releases memory without sanitization (e.g., zeroing out
> > the memory). Consequently, sensitive information such as the HMAC secret
> > and its length may remain in freed memory, potentially leading to data
> > leaks.
> >
> > To address this risk, we replaced kfree_rcu() with a custom RCU
> > callback, seg6_hinfo_free_callback_rcu(). Within this callback, we
> > explicitly sanitize the seg6_hmac_info object before deallocating it
> > safely using kfree_sensitive(). This approach ensures the memory is
> > securely freed and prevents potential HMAC info leaks.
> > Additionally, in the control path, we ensure proper cleanup of
> > seg6_hmac_info objects when seg6_hmac_info_add() fails: such objects are
> > freed using kfree_sensitive() instead of kfree().
> >
> > Fixes: 4f4853dc1c9c ("ipv6: sr: implement API to control SR HMAC structure")
> > Fixes: bf355b8d2c30 ("ipv6: sr: add core files for SR HMAC support")
> 
> Not sure if you are fixing a bug worth backports.

It can be considered a bug fix, or just hardening.  There are examples
of both ways for this same type of issue.  I think the patch is fine
as-is, though the commit message is a bit long.  Zeroizing crypto keys
is a best practice that the kernel tries to follow elsewhere for all
crypto keys, so this is nothing new.  The patch simply adds zeroization
before freeing for a struct that contains a key.

> >  static inline void seg6_hinfo_release(struct seg6_hmac_info *hinfo)
> >  {
> > -       kfree_rcu(hinfo, rcu);
> > +       call_rcu(&hinfo->rcu, seg6_hinfo_free_callback_rcu);
> >  }
> 
> If we worry a lot about sensitive data waiting too much in RCU land,
> perhaps use call_rcu_hurry() here ?

No, zeroization doesn't have stringent time constraints.  As long as it
happens eventually it is fine.

Reviewed-by: Eric Biggers <ebiggers@kernel.org>

- Eric