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 <[email protected]>
> Reviewed-by: Björn Töpel <[email protected]>
> Reviewed-by: Phil Sutter <[email protected]>
> ---
> 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
>
