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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Michael S. Tsirkin wrote:
> Multiple places in the net stack open-code the same minimal IPv4 header
> validation: pull the fixed header, check version 4 and IHL >= 5 (where 5
> is the fixed header size in 32-bit units) - and locate the transport
> header at IHL * 4 past the network header.
> 
> Add a helper for doing that: it returns the validated header and
> the transport header offset, or NULL if truncated or malformed.
> 
> As noted by Phil Sutter, there are more places that could use this
> helper, e.g. ip_mc_check_iphdr(). They will be converted in a follow-up
> patch.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Björn Töpel <bjorn@kernel.org>
> Reviewed-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/linux/ip.h                  | 30 +++++++++++++++++++++++++++++
>  include/linux/virtio_net.h          | 28 +--------------------------
>  net/bridge/br_netfilter_hooks.c     | 15 +++++----------
>  net/ipv4/ip_input.c                 | 13 +++++--------
>  net/ipv4/netfilter/nf_reject_ipv4.c | 13 +++++--------
>  5 files changed, 46 insertions(+), 53 deletions(-)
> 
> diff --git a/include/linux/ip.h b/include/linux/ip.h
> index d11c25f5030a..aba37cd60212 100644
> --- a/include/linux/ip.h
> +++ b/include/linux/ip.h
> @@ -56,4 +56,34 @@ 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

This only pulls sizeof(struct iphdr).

To match function comment and generally be robust: pull again
if ihl != 0.

Apparently virtio_net_hdr_tnl_to_skb will set 
skb->transport_header to the thoff reported here, which may be
beyond the end of the linear packet.

> + * @skb: buffer to parse
> + * @nhoff: offset of the IPv4 header within the linear data
> + * @thoff: returns the transport header offset
> + *
> + * Only the fixed header is guaranteed linear on return: callers
> + * that need the options or 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;
> +
> +	*thoff = nhoff + iph->ihl * sizeof(u32);
> +	return iph;
> +}
>  #endif	/* _LINUX_IP_H */
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b3bbecbd023c..724ab8659501 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -53,32 +53,6 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
>  	return 0;
>  }
>  
> -/* TODO: nothing virtio specific here, move to ip.h */
> -
> -/*
> - * 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.
> - */
> -static inline struct iphdr *virtio_net_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;
> -
> -	*thoff = nhoff + iph->ihl * sizeof(u32);
> -	return iph;
> -}
>  /* Defined in net/core/dev.c, enabled while any network namespace
>   * has net.core.virtio_net_hdr_validate set to 0.
>   */
> @@ -157,7 +131,7 @@ virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
>  		    unsigned int *thoff, int *ip_proto)
>  {
>  	if (proto == htons(ETH_P_IP)) {
> -		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
> +		const struct iphdr *iph = ip4_parse(skb, nhoff, thoff);
>  
>  		if (!iph || ip_is_fragment(iph))
>  			return false;
> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index 0a394e5f4391..e89da0e41542 100644
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c
> @@ -206,18 +206,13 @@ static inline void nf_bridge_pull_encap_header_rcsum(struct sk_buff *skb)
>  static int br_validate_ipv4(struct net *net, struct sk_buff *skb)
>  {
>  	const struct iphdr *iph;
> -	u32 len;
> +	u32 len, thoff;
>  
> -	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> +	iph = ip4_parse(skb, 0, &thoff);
> +	if (!iph)
>  		goto inhdr_error;
>  
> -	iph = ip_hdr(skb);
> -
> -	/* Basic sanity checks */
> -	if (iph->ihl < 5 || iph->version != 4)
> -		goto inhdr_error;
> -
> -	if (!pskb_may_pull(skb, iph->ihl*4))
> +	if (!pskb_may_pull(skb, thoff))
>  		goto inhdr_error;
>  
>  	iph = ip_hdr(skb);
> @@ -228,7 +223,7 @@ static int br_validate_ipv4(struct net *net, struct sk_buff *skb)
>  	if (skb->len < len) {
>  		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
>  		goto drop;
> -	} else if (len < (iph->ihl*4))
> +	} else if (len < thoff)
>  		goto inhdr_error;
>  
>  	if (pskb_trim_rcsum(skb, len)) {
> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> index 9860178752b8..5adf01763e85 100644
> --- a/net/ipv4/ip_input.c
> +++ b/net/ipv4/ip_input.c
> @@ -500,7 +500,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  {
>  	const struct iphdr *iph;
>  	int drop_reason;
> -	u32 len;
> +	u32 len, thoff;
>  
>  	/* When the interface is in promisc. mode, drop all the crap
>  	 * that it receives, do not try to analyse it.
> @@ -520,10 +520,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  	}
>  
>  	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> -	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> -		goto inhdr_error;
> -
> -	iph = ip_hdr(skb);
>  
>  	/*
>  	 *	RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
> @@ -536,7 +532,8 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  	 *	4.	Doesn't have a bogus length
>  	 */
>  
> -	if (iph->ihl < 5 || iph->version != 4)
> +	iph = ip4_parse(skb, 0, &thoff);
> +	if (!iph)
>  		goto inhdr_error;
>  
>  	BUILD_BUG_ON(IPSTATS_MIB_ECT1PKTS != IPSTATS_MIB_NOECTPKTS + INET_ECN_ECT_1);
> @@ -546,7 +543,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
>  		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
>  
> -	if (!pskb_may_pull(skb, iph->ihl*4))
> +	if (!pskb_may_pull(skb, thoff))
>  		goto inhdr_error;
>  
>  	iph = ip_hdr(skb);
> @@ -559,7 +556,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  		drop_reason = SKB_DROP_REASON_PKT_TOO_SMALL;
>  		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
>  		goto drop;
> -	} else if (len < (iph->ihl*4))
> +	} else if (len < thoff)
>  		goto inhdr_error;
>  
>  	/* Our transport medium may have padded the buffer out. Now we know it
> diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
> index 59ec465a9df9..42d639854ddf 100644
> --- a/net/ipv4/netfilter/nf_reject_ipv4.c
> +++ b/net/ipv4/netfilter/nf_reject_ipv4.c
> @@ -25,22 +25,19 @@ nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,
>  static int nf_reject_iphdr_validate(struct sk_buff *skb)
>  {
>  	struct iphdr *iph;
> -	u32 len;
> +	u32 len, thoff;
>  
> -	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> -		return 0;
> -
> -	iph = ip_hdr(skb);
> -	if (iph->ihl < 5 || iph->version != 4)
> +	iph = ip4_parse(skb, 0, &thoff);
> +	if (!iph)
>  		return 0;
>  
>  	len = ntohs(iph->tot_len);
>  	if (skb->len < len)
>  		return 0;
> -	else if (len < (iph->ihl*4))
> +	else if (len < thoff)
>  		return 0;
>  
> -	if (!pskb_may_pull(skb, iph->ihl*4))
> +	if (!pskb_may_pull(skb, thoff))
>  		return 0;
>  
>  	return 1;
> -- 
> MST
>