netdev - Re: [PATCH 1/3] e1000: increase skb size to prevent dma over skb boundary (v2)

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: <20091207155953.GE8073@hmsreliant.think-freely.org>
Date:	Mon, 7 Dec 2009 10:59:53 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Franco Fichtner <franco@...tsummer.de>
Cc:	netdev@...r.kernel.org, e1000-devel@...ts.sourceforge.net,
	davem@...emloft.net, jeffrey.t.kirsher@...el.com,
	jesse.brandeburg@...el.com, bruce.w.allan@...el.com,
	peter.p.waskiewicz.jr@...el.com, john.ronciak@...el.com
Subject: Re: [PATCH 1/3] e1000: increase skb size to prevent dma over skb
	boundary (v2)

On Mon, Dec 07, 2009 at 04:24:02PM +0100, Franco Fichtner wrote:
> Hi Neil,
>
> Neil Horman wrote:
>> Update e1000 driver to not allow dma beyond the end of the allocated skb
>>     Signed-off-by: Neil Horman <nhorman@...driver.com>
>>
>>
>> e1000_main.c |   34 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 33 insertions(+), 1 deletion(-)
>>
>>
>> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
>> index 7e855f9..7600deb 100644
>> --- a/drivers/net/e1000/e1000_main.c
>> +++ b/drivers/net/e1000/e1000_main.c
>> @@ -1667,6 +1667,19 @@ int e1000_setup_all_rx_resources(struct e1000_adapter *adapter)
>>  	return err;
>>  }
>>  +static inline u32 normalize_rx_len(u32 len)
>> +{
>> +        u32 match, last_match;
>> +
>>   
> Skip newline and get rid of last_match. Also, there is a whitespace error...
I'll just wash this all out, based on your comments, ben's and Michals, its not
needed..
>> <snip>  
> If you modify rx_buffer_len anyway, then get rid of normed_rx_len and
> do a quick
>
>    adapter->rx_buffer_len = normalize_rx_len(adapter->rx_buffer_len);
>
> instead. With the modification above, it never fails, so no need to check
> for !normed_rx_len.
>
Agreed, by using roundup_pow_of_two as was previously suggested the extra
variable is no longer needed.

> But I don't really know the context of this change. Is it okay to shorten
> rx_buffer_len here? Why was it not set appropriately as the driver
> expects?
>
We're not shortening, we're rounding up.  And yes its both ok, and necessecary.
The problem (from the intial email I sent), was that this driver tells the
hardware that potentially it can dma up to X bytes to a given rx buffer, but it
is possible and likely that we only provide Y bytes to dma to, where X > Y.
This leads to corruption of the skb_shared_info structure.

> Oh, BTW, the default case in the switch statement is stupid and should
> be removed.
>
I agree, its silly, but with the above changes, it becomes needed (or beneficial
again), to catch cases in which roundup_pow_of_two returns a value beyond what
the hardware can handle.  I think in this case we should just panic, as
rx_buffer_len should never be larger than 16384.


Thanks all for the comments, heres version two of this patch, taking all
comments into account:


Update e1000 driver to not allow dma beyond the end of the allocated skb
    
Signed-off-by: Neil Horman <nhorman@...driver.com>


e1000_main.c |   15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)


diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 7e855f9..cb16615 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1697,6 +1697,17 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 	/* Setup buffer sizes */
 	rctl &= ~E1000_RCTL_SZ_4096;
 	rctl |= E1000_RCTL_BSEX;
+
+	/*
+	 * We need to normalize the rx_buffer_len here
+	 * since the hardware only knows about 7 discrete
+	 * frame lengths here.  To accomodate that we need
+	 * to set the rx length in the hardware to the next highest
+	 * size over the rx_buffer_len, then increase rx_buffer_len
+	 * to match it, so that we can get a full mtu sized frame
+	 */
+	adapter->rx_buffer_len = roundup_pow_of_two(adapter->rx_buffer_len);
+
 	switch (adapter->rx_buffer_len) {
 		case E1000_RXBUFFER_256:
 			rctl |= E1000_RCTL_SZ_256;
@@ -1711,7 +1722,6 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 			rctl &= ~E1000_RCTL_BSEX;
 			break;
 		case E1000_RXBUFFER_2048:
-		default:
 			rctl |= E1000_RCTL_SZ_2048;
 			rctl &= ~E1000_RCTL_BSEX;
 			break;
@@ -1724,6 +1734,9 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 		case E1000_RXBUFFER_16384:
 			rctl |= E1000_RCTL_SZ_16384;
 			break;
+		default:
+			panic("Bad rx_buffer_len size\n");
+			break;
 	}
 
 	ew32(RCTL, rctl);
--
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