netdev - Re: [PATCH] ipv6: sr: add validity check for algorithm ID

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: <aJtHOuAc3BM3Aa9l@shredder>
Date: Tue, 12 Aug 2025 16:52:58 +0300
From: Ido Schimmel <idosch@...sch.org>
To: heminhong <heminhong@...inos.cn>
Cc: netdev@...r.kernel.org, kuba@...nel.org, kuniyu@...gle.com,
	edumazet@...gle.com, pabeni@...hat.com, dsahern@...nel.org
Subject: Re: [PATCH] ipv6: sr: add validity check for algorithm ID

On Tue, Aug 12, 2025 at 02:19:44PM +0800, heminhong wrote:
> From: Minhong He <heminhong@...inos.cn>
> 
> The seg6_genl_sethmac() directly uses the algid passed in by the user
> without checking whether it is an HMAC algorithm supported by the
> system. If the algid is invalid, unknown errors may occur during
> subsequent use of the HMAC information.

You should explain the user visible effects from this bug/fix rather
than saying "unknown errors". AFAICT, an invalid HMAC algorithm will
result in packet drops during encap / decap, but I might have missed a
more serious problem.

Fixes tag seems appropriate:

Fixes: 4f4853dc1c9c ("ipv6: sr: implement API to control SR HMAC structure")

And please read:

https://docs.kernel.org/process/maintainer-netdev.html

> 
> Signed-off-by: Minhong He <heminhong@...inos.cn>
> ---
>  include/net/seg6_hmac.h | 1 +
>  net/ipv6/seg6.c         | 5 +++++
>  net/ipv6/seg6_hmac.c    | 2 +-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/seg6_hmac.h b/include/net/seg6_hmac.h
> index 24f733b3e3fe..c34e86c99de3 100644
> --- a/include/net/seg6_hmac.h
> +++ b/include/net/seg6_hmac.h
> @@ -49,6 +49,7 @@ extern int seg6_hmac_info_del(struct net *net, u32 key);
>  extern int seg6_push_hmac(struct net *net, struct in6_addr *saddr,
>  			  struct ipv6_sr_hdr *srh);
>  extern bool seg6_hmac_validate_skb(struct sk_buff *skb);
> +extern struct seg6_hmac_algo *__hmac_get_algo(u8 alg_id);
>  #ifdef CONFIG_IPV6_SEG6_HMAC
>  extern int seg6_hmac_init(void);
>  extern void seg6_hmac_exit(void);
> diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> index 180da19c148c..33c1481ca50a 100644
> --- a/net/ipv6/seg6.c
> +++ b/net/ipv6/seg6.c
> @@ -152,6 +152,7 @@ static int seg6_genl_sethmac(struct sk_buff *skb, struct genl_info *info)
>  	struct net *net = genl_info_net(info);
>  	struct seg6_pernet_data *sdata;
>  	struct seg6_hmac_info *hinfo;
> +	struct seg6_hmac_algo *algo;
>  	u32 hmackeyid;
>  	char *secret;
>  	int err = 0;
> @@ -175,6 +176,10 @@ static int seg6_genl_sethmac(struct sk_buff *skb, struct genl_info *info)
>  	if (slen > SEG6_HMAC_SECRET_LEN)
>  		return -EINVAL;
>  
> +	algo = __hmac_get_algo(algid);
> +	if (!algo)
> +		return -EINVAL;

Another possibility is to keep the HMAC algorithm logic in seg6_hmac.c
and perform the check there. Something like:

diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
index f78ecb6ad838..d77b52523b6a 100644
--- a/net/ipv6/seg6_hmac.c
+++ b/net/ipv6/seg6_hmac.c
@@ -304,6 +304,9 @@ int seg6_hmac_info_add(struct net *net, u32 key, struct seg6_hmac_info *hinfo)
 	struct seg6_pernet_data *sdata = seg6_pernet(net);
 	int err;
 
+	if (!__hmac_get_algo(hinfo->alg_id))
+		return -EINVAL;
+
 	err = rhashtable_lookup_insert_fast(&sdata->hmac_infos, &hinfo->node,
 					    rht_params);

> +
>  	mutex_lock(&sdata->lock);
>  	hinfo = seg6_hmac_info_lookup(net, hmackeyid);
>  
> diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
> index f78ecb6ad838..1c4858195613 100644
> --- a/net/ipv6/seg6_hmac.c
> +++ b/net/ipv6/seg6_hmac.c
> @@ -107,7 +107,7 @@ static struct sr6_tlv_hmac *seg6_get_tlv_hmac(struct ipv6_sr_hdr *srh)
>  	return tlv;
>  }
>  
> -static struct seg6_hmac_algo *__hmac_get_algo(u8 alg_id)
> +struct seg6_hmac_algo *__hmac_get_algo(u8 alg_id)
>  {
>  	struct seg6_hmac_algo *algo;
>  	int i, alg_count;
> -- 
> 2.25.1
> 
>