Re: [PATCH net v5 6/7] net: add ip4_parse()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] net: use-after-free of iph in ip4_parse() after pskb_may_pull()
--

commit f0109d51e8b0adc3b479c25e0e4799b7ae07962f
Author: Michael S. Tsirkin <mst@redhat.com>

net: add ip4_parse()

This commit introduces the ip4_parse() helper in include/linux/ip.h to
pull and minimally validate an IPv4 header and its options, converting
several callers in virtio_net, bridge netfilter, ipv4 input, and
netfilter ipv4 reject.

> diff --git a/include/linux/ip.h b/include/linux/ip.h
> index d11c25f5030a0..7e047d18a61c9 100644
> --- a/include/linux/ip.h
> +++ b/include/linux/ip.h
> @@ -56,4 +56,41 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
>  {
>  	iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
>  }
> +
> +/**
> + * ip4_parse - pull and minimally validate an IPv4 header
> + * @skb: buffer to parse
> + * @nhoff: offset of the IPv4 header within the linear data
> + * @thoff: returns the transport header offset
> + *
> + * The whole header, options included, is guaranteed linear on
> + * return, so the transport header offset is within the linear
> + * data.  Callers that need to read the transport header must
> + * pull further.
> + *
> + * Return: the validated IPv4 header, or NULL if truncated or
> + * malformed.
> + */
> +static inline struct iphdr *ip4_parse(struct sk_buff *skb,
> +				      unsigned int nhoff,
> +				      unsigned int *thoff)
> +{
> +	struct iphdr *iph;
> +
> +	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
> +		return NULL;
> +
> +	iph = (struct iphdr *)(skb->data + nhoff);
> +	if (iph->version != 4 || iph->ihl < sizeof(struct iphdr) / sizeof(u32))
> +		return NULL;
> +
> +	/* Pull the options as well: the transport header offset
> +	 * returned below must be within the linear data.
> +	 */
> +	if (!pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)))
> +		return NULL;
> +
> +	*thoff = nhoff + iph->ihl * sizeof(u32);

[Severity: Critical]
Can this dereference of iph->ihl after the second pskb_may_pull() cause a
use-after-free?

In include/linux/ip.h:ip4_parse(), iph is initialized to point into
skb->data before the second pskb_may_pull() call pulls the IPv4 options.

When IPv4 options (ihl > 5) reside in non-linear skb data and either
skb_cloned(skb) is true or skb->head lacks tailroom (or when
skb_might_realloc() triggers), the call chain:

  pskb_may_pull()
    pskb_may_pull_reason()
      __pskb_pull_tail()
        pskb_expand_head()

allocates a new skb->head buffer, updates skb->data, and frees the old
buffer that iph points into. Reading iph->ihl again afterward to compute
*thoff accesses freed memory before skb->data + nhoff is recomputed for
the return value.

Would computing *thoff = nhoff + iph->ihl * sizeof(u32) before calling
pskb_may_pull(skb, *thoff) avoid dereferencing the stale iph pointer?

> +	return (struct iphdr *)(skb->data + nhoff);
> +}
>  #endif	/* _LINUX_IP_H */

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.v5.git.mst@redhat.com?part=6