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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250825205526.GA2130842@google.com>
Date: Mon, 25 Aug 2025 20:55:26 +0000
From: Eric Biggers <ebiggers@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Andrea Mayer <andrea.mayer@...roma2.it>,
	"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,
	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
Subject: Re: [PATCH net] ipv6: sr: fix destroy of seg6_hmac_info to prevent
 HMAC data leak

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

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@...nel.org>

- Eric