Message-ID: <m2r07tipr1.fsf@chopps.org>
Date: Sat, 02 Nov 2024 16:01:42 +0000
From: Christian Hopps <chopps@...pps.org>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: Christian Hopps <chopps@...pps.org>, 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


Steffen Klassert <steffen.klassert@...unet.com> writes:

> 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

The version is checked prior to calling to only be v4 or v6. Removed the WARN call and made the comment above saying this more explicit.

>> +}
>> +
>> +/**
>> + * __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.

Same.

>> +
>> +		/* 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.

As we discussed offline, these uses were not where value can actually be wrong, they were all originally BUG_ON() and meant to document the code assumptions/assertions and to catch future coding/review bugs.

This is not a style that is used by/welcome in linux kernel code so I will remove it's use.

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

Removed.

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)

Powered by blists - more mailing lists