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

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

[Andrea Mayer]

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")
Cc: stable@vger.kernel.org
Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.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);
 }
 
 static void seg6_free_hi(void *ptr, void *arg)
-- 
2.20.1

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

[Eric Dumazet]

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.

> Cc: stable@vger.kernel.org
> Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.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 ?

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

[Eric Biggers]

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

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@google.com> 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.
> 

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