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

[Andrea Mayer]

On Mon, 25 Aug 2025 12:33:26 -0700
Eric Dumazet <edumazet@xxxxxxxxxx> wrote:

> On Mon, Aug 25, 2025 at 12:08 PM Andrea Mayer <andrea.mayer@xxxxxxxxxxx> 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.
> 

I believe failing to delete sensitive data, such as HMAC keys, from memory
before releasing it could pose security risks.
Therefore, I considered this a bug to be fixed in the stable versions.

> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Andrea Mayer <andrea.mayer@xxxxxxxxxxx>
> > ---
> >  net/ipv6/seg6.c      |  2 +-
> >  net/ipv6/seg6_hmac.c | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> > index 180da19c148c..88782bdab843 100644
> > --- a/net/ipv6/seg6.c
> > +++ b/net/ipv6/seg6.c
> > @@ -215,7 +215,7 @@ static int seg6_genl_sethmac(struct sk_buff *skb, struct genl_info *info)
> >
> >         err = seg6_hmac_info_add(net, hmackeyid, hinfo);
> >         if (err)
> > -               kfree(hinfo);
> > +               kfree_sensitive(hinfo);
> >
> >  out_unlock:
> >         mutex_unlock(&sdata->lock);
> > diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
> > index fd58426f222b..19cdf3791ebf 100644
> > --- a/net/ipv6/seg6_hmac.c
> > +++ b/net/ipv6/seg6_hmac.c
> > @@ -57,9 +57,17 @@ static int seg6_hmac_cmpfn(struct rhashtable_compare_arg *arg, const void *obj)
> >         return (hinfo->hmackeyid != *(__u32 *)arg->key);
> >  }
> >
> > +static void seg6_hinfo_free_callback_rcu(struct rcu_head *head)
> > +{
> > +       struct seg6_hmac_info *hinfo;
> > +
> > +       hinfo = container_of(head, struct seg6_hmac_info, rcu);
> > +       kfree_sensitive(hinfo);
> > +}
> > +
> >  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 ?

My concern is not so much about how long the sensitive data remains in RCU
land. Instead, I would like to ensure that the memory associated with the
seg6_hmac_info object is properly zeroed out before it is freed. I believe that
using call_rcu() (with seg6_hinfo_free_callback_rcu()) would be sufficient to
achieve this goal.

---

Aside from improving the commit message (thanks to Eric Bigger), what other
changes should we consider implementing for version 2?
Should we classify this patch as an enhancement rather than a bug fix?

Thank you all for your time,
Andrea