netdev - Re: [PATCH] ixgbe: drop zero length frame segments during a packet split rx

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]

Message-ID: <20110902165523.GB27571@hmsreliant.think-freely.org>
Date:	Fri, 2 Sep 2011 12:55:23 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Alexander Duyck <alexander.h.duyck@...el.com>
Cc:	netdev@...r.kernel.org,
	Thadeu Lima de Souza Cascardo <cascardo@...ux.vnet.ibm.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] ixgbe: drop zero length frame segments during a packet
 split rx

On Fri, Sep 02, 2011 at 09:17:40AM -0700, Alexander Duyck wrote:
> This kind of fix just opens up a whole can of security related
> worms.  If you are going to discard a packet you should do it after
> we have reached the EOP in the series.  My advice would be to
> determine what traits identify this packet and add those to the
> check for the IXGBE_RXDADV_ERR_FRAME_ERR_MASK check further down in
> the code.  Likely what you are seeing is skb_headlen(skb) will be
> equal to 0.
> 
Well, the traits of the bogus descriptor are almost exactly as you describe
them, i.e. rx_buffer_info->dma is zero, which the driver takes to mean packet
split is enabled, and this is a buffer in the middle of that operation
(according to the comments in ixgbe_clean_rx_irq), and the upper_len value we
read from the rx_descriptior rx_dex->wb.upper.length is zero.  This implies we
have a frame which is in the middle of a packet split receive, and one of the
page long buffers has a length value of zero, which is non-sensical.  I suppose
we could wait until the next frame with EOP set to discard the whole thing, but
I'm not sure how that amounts to anything different than just skipping to the
next descriptor.

> I'm suspecting this is some sort of read corruption.  It looks like
> in order to trigger it you have to either be reading
> rx_buffer_info->dma as 0, or the header length is being read as 0.
Correct, which drops us into the else clause of the if(rx_buffer_info->dma)
conditional in ixgbe_clean_rx_irq.

> Do you know if you actually have header split enabled when this is
> occuring?  Are you running with jumbo frames enabled to see the
Yes, packet split is enabled. and no, Jumbo frames are not in use.

> issue?  If not then packet split wouldn't be enabled.
> 
> Is this occurring on net-next or on an older kernel?  I just want to
> be sure since we added a read memory barrier in 2.6.34 to address
> the fact that the length and descriptor DD bits were being read in
> the wrong order resulting in the length being corrupted on PowerPC
> systems.  The fact that we are now seeing another length error on
> PowerPC seems very odd.
> 
According to the bz:
https://bugzilla.redhat.com/show_bug.cgi?id=683611
This appears to be happening on RHEL, and on upstream kernels, as well as the
sourceforge driver.  Don't quote me on the SF driver though, because I never got
a clear answer on that.  Although, fwiw, the RHEL version of the driver in which
we were definately seeing this problem has a read memory barrrier at the top of
the loop in ixgbe_clean_rx_irq, pulled in from commit
3c945e5b3719bcc18c6ddd31bbcae8ef94f3d19a, so I think thats handled.


Regards
Neil
--
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