netdev - Re: [PATCH net] ipv6: sr: fix destroy of seg6_hmac

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20250826192123.612cd0eabbbb57795a0bbdbc@uniroma2.it>
Date: Tue, 26 Aug 2025 19:21:23 +0200
From: Andrea Mayer <andrea.mayer@...roma2.it>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, David Ahern
 <dsahern@...nel.org>,
        Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Eric Biggers
 <ebiggers@...nel.org>,
        David Lebrun <dlebrun@...gle.com>,
        Stefano Salsano
 <stefano.salsano@...roma2.it>,
        Paolo Lungaroni
 <paolo.lungaroni@...roma2.it>,
        Ahmed Abdelsalam <ahabdels.dev@...il.com>, stable@...r.kernel.org,
        Andrea Mayer <andrea.mayer@...roma2.it>
Subject: Re: [PATCH net] ipv6: sr: fix destroy of seg6_hmac_info to prevent
 HMAC data leak

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

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

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@...r.kernel.org
> > Signed-off-by: Andrea Mayer <andrea.mayer@...roma2.it>
> > ---
> >  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