Message-ID: <20091028185947.GA12675@hmsreliant.think-freely.org>
Date:	Wed, 28 Oct 2009 14:59:47 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com
Subject: Re: [PATCH] AF_RAW: Augment raw_send_hdrinc to expand skb to fit
	iphdr->ihl (v2)

On Wed, Oct 28, 2009 at 07:13:09PM +0100, Eric Dumazet wrote:
> Neil Horman a écrit :
> > Augment raw_send_hdrinc to correct for incorrect ip header length values
> > 
> > A series of oopses was reported to me recently.  Apparently when using AF_RAW
> > sockets to send data to peers that were reachable via ipsec encapsulation,
> > people could panic or BUG halt their systems.
> > 
> > I've tracked the problem down to user space sending an invalid ip header over an
> > AF_RAW socket with IP_HDRINCL set to 1.
> > 
> > Basically what happens is that userspace sends down an ip frame that includes
> > only the header (no data), but sets the ip header ihl value to a large number,
> > one that is larger than the total amount of data passed to the sendmsg call.  In
> > raw_send_hdrincl, we allocate an skb based on the size of the data in the msghdr
> > that was passed in, but assume the data is all valid.  Later during ipsec
> > encapsulation, xfrm4_tranport_output moves the entire frame back in the skbuff
> > to provide headroom for the ipsec headers.  During this operation, the
> > skb->transport_header is repointed to a spot computed by
> > skb->network_header + the ip header length (ihl).  Since so little data was
> > passed in relative to the value of ihl provided by the raw socket, we point
> > transport header to an unknown location, resulting in various crashes.
> > 
> > So, what to do about this?  My first thought was to simply return -EINVAL, and
> > let user space sort it out.  I'm still thinking that might be the best way, but
> > I thought I'd try this first, just in case someone has reason to try to
> > send such a bogus frame through the kernel.  This solution simply checks the
> > value of ihl in raw_send_hdrinc and expands the skb to fit, filling the new
> > space with  IPOPT_NOOP options.  I've confirmed that it fixes the crashes that
> > were reported.
> > 
> > Signed-off-by: Neil Horman <nhorman@...driver.com>
> > 
> 
> Thanks a lot for this detailed info, I wish everything could be explained like this !
> 
You're welcome, this was a fun one to track down :)

> I believe we should drop the request, since padding it is not what was expected by user.

Yeah, I had a feeling.  Ok, version 2, this time drop the invalid frame and
report it to user space, instead of expanding it:


    Augment raw_send_hdrinc to correct for incorrect ip header length values
    
    A series of oopses was reported to me recently.  Apparently when using AF_RAW
    sockets to send data to peers that were reachable via ipsec encapsulation,
    people could panic or BUG halt their systems.
    
    I've tracked the problem down to user space sending an invalid ip header over an
    AF_RAW socket with IP_HDRINCL set to 1.
    
    Basically what happens is that userspace sends down an ip frame that includes
    only the header (no data), but sets the ip header ihl value to a large number,
    one that is larger than the total amount of data passed to the sendmsg call.  In
    raw_send_hdrincl, we allocate an skb based on the size of the data in the msghdr
    that was passed in, but assume the data is all valid.  Later during ipsec
    encapsulation, xfrm4_tranport_output moves the entire frame back in the skbuff
    to provide headroom for the ipsec headers.  During this operation, the
    skb->transport_header is repointed to a spot computed by
    skb->network_header + the ip header length (ihl).  Since so little data was
    passed in relative to the value of ihl provided by the raw socket, we point
    transport header to an unknown location, resulting in various crashes.
    
    This fix for this is pretty straightforward, simply validate the value of of
    iph->ihl when sending over a raw socket.  If (iph->ihl*4U) > user data buffer
    size, drop the frame and return -EINVAL.  I just confirmed this fixes the
    reported crashes.
    
    Signed-off-by: Neil Horman <nhorman@...driver.com>

 raw.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)


diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 9ef8c08..4b15354 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -351,13 +351,24 @@ static int raw_send_hdrinc(struct sock *sk, void *from, size_t length,
 	skb->ip_summed = CHECKSUM_NONE;
 
 	skb->transport_header = skb->network_header;
-	err = memcpy_fromiovecend((void *)iph, from, 0, length);
-	if (err)
-		goto error_fault;
+	err = -EFAULT;
+	if (memcpy_fromiovecend((void *)iph, from, 0, length))
+		goto error_free;
 
-	/* We don't modify invalid header */
 	iphlen = iph->ihl * 4;
-	if (iphlen >= sizeof(*iph) && iphlen <= length) {
+
+	/*
+	 * We don't want to modify the ip header, but we do need to 
+	 * be sure that it won't cause problems later along the network
+	 * stack.  Specifically we want to make sure that iph->ihl is a 
+	 * sane value.  If ihl points beyond the length of the buffer passed
+	 * in, reject the frame as invalid
+	 */
+	err = -EINVAL;
+	if (iphlen > length)
+		goto error_free; 
+ 
+	if (iphlen >= sizeof(*iph)) {
 		if (!iph->saddr)
 			iph->saddr = rt->rt_src;
 		iph->check   = 0;
@@ -380,8 +391,7 @@ static int raw_send_hdrinc(struct sock *sk, void *from, size_t length,
 out:
 	return 0;
 
-error_fault:
-	err = -EFAULT;
+error_free:
 	kfree_skb(skb);
 error:
 	IP_INC_STATS(net, IPSTATS_MIB_OUTDISCARDS);
--
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

Powered by blists - more mailing lists