Re: [PATCH net] ipv6: validate extension header length before copying to cmsg

[Paolo Abeni <pabeni@redhat.com>]

On 4/19/26 5:03 PM, Qi Tang wrote:
> ip6_datagram_recv_specific_ctl() builds IPV6_{HOPOPTS,DSTOPTS,RTHDR}
> cmsgs (and their IPV6_2292* legacy counterparts) by trusting the
> on-wire hdrlen byte (ptr[1]) when computing the put_cmsg() length.
> The length was validated only at parse time (ipv6_parse_hopopts(),
> etc.). An nftables payload-write expression can rewrite hdrlen after
> parsing and before the skb reaches recvmsg; the write itself is
> in-bounds but put_cmsg() then reads up to ((hdrlen+1) << 3) = 2040
> bytes from an 8-byte header. nftables is reachable from an unprivi-
> leged user namespace, so this is an unprivileged slab-out-of-bounds
> read:
> 
>   BUG: KASAN: slab-out-of-bounds in put_cmsg+0x3ac/0x540
>    put_cmsg+0x3ac/0x540
>    udpv6_recvmsg+0xca0/0x1250
>    sock_recvmsg+0xdf/0x190
>    ____sys_recvmsg+0x1b1/0x620
> 
> Clamp each cmsg length against skb_tail_pointer(skb) before calling
> put_cmsg(). Extension headers are kept in the linear skb area by
> pskb_may_pull() during input, so skb_tail_pointer() is the correct
> bound. The check is replicated at each call site (one HbH, four
> RFC2292 sites, and four switch cases in the DSTOPTS/RTHDR/AH walk)
> rather than hoisted out of the switch, to keep the fix minimal and
> backportable; a follow-up cleanup can factor it out. In the walk
> loop a failed check also aborts the walk, since subsequent offsets
> depend on the tampered length.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Qi Tang <tpluszz77@gmail.com>
> ---
>  net/ipv6/datagram.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index ca3605acb..a7b9f5a24 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -643,7 +643,10 @@ void ip6_datagram_recv_specific_ctl(struct sock *sk, struct msghdr *msg,
>  	/* HbH is allowed only once */
>  	if (np->rxopt.bits.hopopts && (opt->flags & IP6SKB_HOPBYHOP)) {
>  		u8 *ptr = nh + sizeof(struct ipv6hdr);
> -		put_cmsg(msg, SOL_IPV6, IPV6_HOPOPTS, (ptr[1]+1)<<3, ptr);
> +		u16 hbhlen = (ptr[1] + 1) << 3;
> +
> +		if (ptr + hbhlen <= skb_tail_pointer(skb))
> +			put_cmsg(msg, SOL_IPV6, IPV6_HOPOPTS, hbhlen, ptr);

The patch looks functionally correct to me, but the above 3 statements
are repeated multiple times. You can put them in a local helper and
avoud a lot of duplicate code.

>  	}
>  
>  	if (opt->lastopt &&
> @@ -668,27 +671,37 @@ void ip6_datagram_recv_specific_ctl(struct sock *sk, struct msghdr *msg,
>  			case IPPROTO_DSTOPTS:
>  				nexthdr = ptr[0];
>  				len = (ptr[1] + 1) << 3;
> +				if (ptr + len > skb_tail_pointer(skb))
> +					goto ext_hdr_done;

The packet is corrupted, allowing processing of later rxopt requires the
IMHO not nice empty label. I think it would be better just returning
from this function.

/P