linux-kernel - Re: [PATCH] net: ipv6: sanitize RPL SRH cmpre/cmpre fields to fix taint issue

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: <6831d5396ead5_1e734029446@willemb.c.googlers.com.notmuch>
Date: Sat, 24 May 2025 10:18:33 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Rafal Bilkowski <rafalbilkowski@...il.com>, 
 netdev@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, 
 dsahern@...nel.org, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 Rafal Bilkowski <rafalbilkowski@...il.com>, 
 alex.aring@...il.com
Subject: Re: [PATCH]    net: ipv6: sanitize RPL SRH cmpre/cmpre fields to fix
 taint issue

Rafal Bilkowski wrote:
>    Coverity flagged that the cmpre and cmpri fields in
>    struct ipv6_rpl_sr_hdr are used without proper bounds checking,
>    which may allow tainted values to be used as offsets or divisors,
>    potentially leading to out-of-bounds access or division by zero.
> 
>    This patch adds explicit range checks for cmpre and cmpri before
>    using them, ensuring they are within the valid range (0-15) and
>    cmpri is non-zero. Coverity was run loccaly

No indentation on comment.

Also in networking please add target in the subject, here [PATCH net]
 
>    Fixes:  ("Untrusted value as argument (TAINTED_SCALAR)")

Invalid Fixes tag. This should here be

  Fixes: 8610c7c6e3bd ("net: ipv6: add support for rpl sr exthdr")

> 
> Signed-off-by: Rafal Bilkowski <rafalbilkowski@...il.com>
> ---
>  net/ipv6/exthdrs.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 02e9ffb63af1..9646738cb872 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -504,6 +504,15 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
>  	}
>  
>  looped_back:
> +
> +	if (!pskb_may_pull(skb, skb_transport_offset(skb) + sizeof(struct ipv6_rpl_sr_hdr)))
> +		goto error;

This check likely is indeed needed before inspecting the header.

No need for a goto if only result is a return. Also the skb needs to
be freed, see other error paths.

> +	// Check if there is enough memory available for the header and hdrlen is in valid range
> +	if (skb_tailroom(skb) < ((hdr->hdrlen + 1) << 3) ||
> +	    hdr->hdrlen == 0 ||
> +	    hdr->hdrlen > U8_MAX)

How is ipv6_rpl_sr_hdr hdrlen defined. I don't immediately see it in
the struct definition comments or in RFC 6550

> +		goto error;
> +
>  	hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
>  
>  	if (hdr->segments_left == 0) {
> @@ -534,7 +543,18 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
>  		return 1;
>  	}
>  
> +	// Check if cmpri and cmpre are valid and do not exceed 15
> +	if (hdr->cmpri > 15 || hdr->cmpre > 15)
> +		goto error;

These are 4-bit fields. This cannot happen.

> +	// Check if pad value is valid and does not exceed 15
> +	if (hdr->pad > 15)
> +		goto error;
> +
>  	n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
> +	// Check if n is non-negative
> +	if (n <= 0)
> +		goto error;
> +
>  	r = do_div(n, (16 - hdr->cmpri));
>  	/* checks if calculation was without remainder and n fits into
>  	 * unsigned char which is segments_left field. Should not be
> @@ -638,6 +658,9 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
>  	dst_input(skb);
>  
>  	return -1;
> +
> +error:
> +	return -1;
>  }
>  
>  /********************************
> -- 
> 2.43.0
>