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: <9929d2390912071252u2de5cf73tf1a43d7959413ac4@mail.gmail.com>
Date:	Mon, 7 Dec 2009 12:52:57 -0800
From:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	Franco Fichtner <franco@...tsummer.de>, netdev@...r.kernel.org,
	e1000-devel@...ts.sourceforge.net, davem@...emloft.net,
	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 7, 2009 at 07:59, Neil Horman <nhorman@...driver.com> wrote:
> 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);
> --

I have added this patch to my queue of e1000e patches for
reveiw/testing.  Upon successful review/testing I will submit to
Dave/netdev.

-- 
Cheers,
Jeff
--
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