netdev - Re: [PATCH] ipv6: mld: Prevent skb_over

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 20 Dec 2013 17:24:58 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Travis Brown <travisb@...stanetworks.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, kuznet@....inr.ac.ru,
	jmorris@...ei.org, yoshfuji@...ux-ipv6.org, kaber@...sh.net
Subject: Re: [PATCH] ipv6: mld: Prevent skb_over_panic with large MTUs

On Fri, 2013-12-20 at 17:00 -0800, Travis Brown wrote:
> MLD limits the maximum skb size to fit within a single page. Later
> code uses the device MTU, if available, to determine whether the skb
> has sufficient room for another field. On interfaces with MTUs larger
> than the page size, less the skb overhead, this can result in the skb
> being overrun and a skb_over_panic being generated.
> 
> Fix MLD in the same way IGMPv3 was fixed in the commit 57e1ab6e
> ("igmp: refine skb allocations"), by storing the requested packet size
> into skb->cb[0] and using that instead of the device MTU.
> 
> Signed-off-by: Travis Brown <travisb@...stanetworks.com>
> ---
>  net/ipv6/mcast.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index d18f9f9..3c6cbc3 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -1522,6 +1522,7 @@ static void ip6_mc_hdr(struct sock *sk, struct sk_buff *skb,
>  	hdr->daddr = *daddr;
>  }
>  
> +#define mld_skb_size(skb) (*(unsigned int *)((skb)->cb))
>  static struct sk_buff *mld_newpack(struct inet6_dev *idev, int size)
>  {
>  	struct net_device *dev = idev->dev;
> @@ -1531,6 +1532,7 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, int size)
>  	struct mld2_report *pmr;
>  	struct in6_addr addr_buf;
>  	const struct in6_addr *saddr;
> +	int skb_size;
>  	int hlen = LL_RESERVED_SPACE(dev);
>  	int tlen = dev->needed_tailroom;
>  	int err;
> @@ -1539,14 +1541,16 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, int size)
>  		     IPV6_TLV_PADN, 0 };
>  
>  	/* we assume size > sizeof(ra) here */
> -	size += hlen + tlen;
> +	skb_size = size + hlen + tlen;
>  	/* limit our allocations to order-0 page */
> -	size = min_t(int, size, SKB_MAX_ORDER(0, 0));
> -	skb = sock_alloc_send_skb(sk, size, 1, &err);
> +	skb_size = min_t(int, skb_size, SKB_MAX_ORDER(0, 0));
> +	skb = sock_alloc_send_skb(sk, skb_size, 1, &err);
>  
>  	if (!skb)
>  		return NULL;
>  
> +	mld_skb_size(skb) = size;
> +
>  	skb->priority = TC_PRIO_CONTROL;
>  	skb_reserve(skb, hlen);
>  
> @@ -1661,7 +1665,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
>  	return skb;
>  }
>  
> -#define AVAILABLE(skb) ((skb) ? ((skb)->dev ? (skb)->dev->mtu - (skb)->len : \
> +#define AVAILABLE(skb) ((skb) ? ((skb)->dev ? mld_skb_size(skb) - (skb)->len : \
>  	skb_tailroom(skb)) : 0)
>  

Not clear why this macro is like that....

It looks like it should really be 

#define AVAILABLE(skb) ((skb) ? (mld_skb_size(skb) - (skb)->len) : 0)

Or even better use skb_availroom() as we do in TCP stack...
(check sk_stream_alloc_skb())

Nice catch, thanks Travis !

Fixes: 72e09ad107e78 ("ipv6: avoid high order allocations")



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html