Re: [PATCH net-next 1/4] net: add ip4_parse()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The doc comment on ip4_parse() (include/linux/ip.h) promises more
  than the code checks.
- [Low] API hazard: nhoff is unsigned int and `nhoff + sizeof(struct
  iphdr)` is not checked for overflow.

--

net: add ip4_parse()

This commit adds an inline helper, ip4_parse(), to include/linux/ip.h.
It pulls the fixed IPv4 header at a given offset, checks for version 4
and IHL >= 5, and returns the header together with the transport header
offset. It is meant to replace several open-coded copies of the same
minimal validation in the net stack.

> diff --git a/include/linux/ip.h b/include/linux/ip.h
> index d11c25f5030a0..98012f6d0efb5 100644
> --- a/include/linux/ip.h
> +++ b/include/linux/ip.h
> @@ -56,4 +56,28 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
>  {
>  	iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
>  }
> +
> +/*
> + * Pull and minimally validate the IPv4 header at @nhoff.
> + * Returns the header and stores the transport header offset in
> + * @thoff, or NULL if truncated or malformed.  Only the fixed header
> + * is guaranteed linear; callers that need the options or the
> + * transport header must pull further.
> + */

[Severity: Low]
Is "NULL if truncated or malformed" accurate here? ip4_parse() only does
three checks: a pskb_may_pull() of the 20-byte fixed header, version == 4,
and ihl >= 5.

Two kinds of bad header still get a non-NULL iph back:

  - A header cut off inside its options, for example ihl = 15 with only
    20 bytes in the skb.
  - A header whose tot_len is smaller than ihl * 4.

In both cases thoff is stored without being checked against skb->len or
ntohs(iph->tot_len):

	*thoff = nhoff + iph->ihl * sizeof(u32);

The next sentence of the comment says the options may not be linear. It
does not say that thoff can point past the end of the packet or the
datagram.

The callers added later in this series do their own checks on thoff, so
none of them overruns today. Those callers are ip_rcv_core(),
br_validate_ipv4(), nf_reject_iphdr_validate() and
virtio_net_csum_partial_ok_at_offset().

Could the comment say that only the fixed header is validated, and that
callers must check thoff against what they can pull and against tot_len?

> +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);

[Severity: Low]
This isn't a bug in any current caller, but nhoff is an unsigned int and
the sum nhoff + sizeof(struct iphdr) is not checked for overflow.

Suppose a caller passed a negative offset, such as a negative
skb_network_offset(), or a very large one. Could the length passed to
pskb_may_pull() then wrap to a small value, while skb->data + nhoff still
uses the original offset?

No caller in the series passes such an offset:

  - ip_rcv_core(), br_validate_ipv4() and nf_reject_iphdr_validate()
    pass 0.
  - virtio_net_csum_partial_ok() passes ETH_HLEN, a VLAN depth, or a
    non-negative skb_network_offset().

The IPv6 branch next to the ip4_parse() call open-codes the same
pskb_may_pull(skb, nhoff + sizeof(...)) pattern.

Would it be worth documenting that nhoff must be in range, or rejecting
an nhoff that wraps?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791887415.git.mst%40redhat.com