netdev - Re: [PATCH ipsec-next v12 12/16] xfrm: iptfs: handle received fragmented inner packets

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: <ZxYsWnWKPYyaoX79@gauss3.secunet.de>
Date: Mon, 21 Oct 2024 12:26:34 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Christian Hopps <chopps@...pps.org>
CC: <devel@...ux-ipsec.org>, <netdev@...r.kernel.org>, Florian Westphal
	<fw@...len.de>, Sabrina Dubroca <sd@...asysnail.net>, Simon Horman
	<horms@...nel.org>, Antony Antony <antony@...nome.org>, Christian Hopps
	<chopps@...n.net>
Subject: Re: [PATCH ipsec-next v12 12/16] xfrm: iptfs: handle received
 fragmented inner packets

On Mon, Oct 07, 2024 at 09:59:24AM -0400, Christian Hopps wrote:
> From: Christian Hopps <chopps@...n.net>
> 
> +
> +/**
> + * __iptfs_iphlen() - return the v4/v6 header length using packet data.
> + * @data: pointer at octet with version nibble
> + *
> + * The version data is expected to be valid (i.e., either 4 or 6).
> + *
> + * Return: the IP header size based on the IP version.
> + */
> +static u32 __iptfs_iphlen(u8 *data)
> +{
> +	struct iphdr *iph = (struct iphdr *)data;
> +
> +	if (iph->version == 0x4)
> +		return sizeof(*iph);
> +	WARN_ON_ONCE(iph->version != 0x6);
> +	return sizeof(struct ipv6hdr);

Better to return an error if this is not IPv6

> +}
> +
> +/**
> + * __iptfs_iplen() - return the v4/v6 length using packet data.
> + * @data: pointer to ip (v4/v6) packet header
> + *
> + * Grab the IPv4 or IPv6 length value in the start of the inner packet header
> + * pointed to by `data`. Assumes data len is enough for the length field only.
> + *
> + * The version data is expected to be valid (i.e., either 4 or 6).
> + *
> + * Return: the length value.
> + */
> +static u32 __iptfs_iplen(u8 *data)
> +{
> +	struct iphdr *iph = (struct iphdr *)data;
> +
> +	if (iph->version == 0x4)
> +		return ntohs(iph->tot_len);
> +	WARN_ON_ONCE(iph->version != 0x6);
> +	return ntohs(((struct ipv6hdr *)iph)->payload_len) +
> +	       sizeof(struct ipv6hdr);

Same here.

> +
> +		/* We have enough data to get the ip length value now,
> +		 * allocate an in progress skb
> +		 */
> +		ipremain = __iptfs_iplen(xtfs->ra_runt);
> +		if (ipremain < sizeof(xtfs->ra_runt)) {
> +			/* length has to be at least runtsize large */
> +			XFRM_INC_STATS(xs_net(xtfs->x),
> +				       LINUX_MIB_XFRMINIPTFSERROR);
> +			goto abandon;
> +		}
> +
> +		/* For the runt case we don't attempt sharing currently. NOTE:
> +		 * Currently, this IPTFS implementation will not create runts.
> +		 */
> +
> +		newskb = iptfs_alloc_skb(skb, ipremain, false);

As mentioned above, __iptfs_iplen needs error handling. Otherwise
you might alocate a random amount of data here.

> +		if (!newskb) {
> +			XFRM_INC_STATS(xs_net(xtfs->x), LINUX_MIB_XFRMINERROR);
> +			goto abandon;
> +		}
> +		xtfs->ra_newskb = newskb;
> +
> +		/* Copy the runt data into the buffer, but leave data
> +		 * pointers the same as normal non-runt case. The extra `rrem`
> +		 * recopied bytes are basically cacheline free. Allows using
> +		 * same logic below to complete.
> +		 */
> +		memcpy(skb_put(newskb, runtlen), xtfs->ra_runt,
> +		       sizeof(xtfs->ra_runt));
> +	}
> +
> +	/* Continue reassembling the packet */
> +	ipremain = __iptfs_iplen(newskb->data);
> +	iphlen = __iptfs_iphlen(newskb->data);
> +
> +	/* Sanity check, we created the newskb knowing the IP length so the IP
> +	 * length can't now be shorter.
> +	 */
> +	WARN_ON_ONCE(newskb->len > ipremain);
> +
> +	ipremain -= newskb->len;
> +	if (blkoff < ipremain) {
> +		/* Corrupt data, we don't have enough to complete the packet */
> +		XFRM_INC_STATS(xs_net(xtfs->x), LINUX_MIB_XFRMINIPTFSERROR);
> +		goto abandon;
> +	}
> +
> +	/* We want the IP header in linear space */
> +	if (newskb->len < iphlen) {
> +		iphremain = iphlen - newskb->len;
> +		if (blkoff < iphremain) {
> +			XFRM_INC_STATS(xs_net(xtfs->x),
> +				       LINUX_MIB_XFRMINIPTFSERROR);
> +			goto abandon;
> +		}
> +		fraglen = min(blkoff, remaining);
> +		copylen = min(fraglen, iphremain);
> +		WARN_ON_ONCE(skb_tailroom(newskb) < copylen);

This is also something that needs error handling. This WARN_ON_ONCE
does not make much sense, as the next line will crash the machine
anyway if this condition is true.

This is also a general thing, there are a lot of WARN_ON_ONCE
and you just continue after the warning. Whenever such a warn
condition can happen, it needs audit why it can happen. Usually
it can be either fixed or catched with an error. Warnings
should be used very rarely.

In this case you can either make sure to allocate the correct amount
of data or extend the tailroom with pskb_expand_head().

No need to crash the machine here :)

Please audit your WARN_ON_ONCE calls, I guess most are either not
needed or the condition can be handled otherwise somehow.

> +		if (skb_copy_seq_read(st, data, skb_put(newskb, copylen),
> +				      copylen)) {
> +			XFRM_INC_STATS(xs_net(xtfs->x),
> +				       LINUX_MIB_XFRMINBUFFERERROR);
> +			goto abandon;
> +		}

> @@ -1286,7 +1729,11 @@ static int iptfs_copy_to_user(struct xfrm_state *x, struct sk_buff *skb)
>  	int ret = 0;
>  	u64 q;
>  
> -	if (x->dir == XFRM_SA_DIR_OUT) {
> +	if (x->dir == XFRM_SA_DIR_IN) {
> +		q = xtfs->drop_time_ns;
> +		(void)do_div(q, NSECS_IN_USEC);

This cast is not needed.