diff mbox

Probably a flaw in Linux rtl8139 driver FYI

Message ID 528f811a0904180540t15489826n99cf8143fe105463@mail.gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Tzungder Lin April 18, 2009, 12:40 p.m. UTC 
Dear Sirs,

Hello, I am jon from Taiwan.
First I want to thank you for your great contributions of open source.
Thank you for your 8139 driver to make our world better.

Here is what happens:
While I am debugging our Embedded Linux SoC I found a flaw in Linux
8139 driver (8139too.c) ,probably.
If I attack (interval < 10ms) the driver with broadcast packets
(mac.destaddr == ff:ff:ff:ff:ff:ff) when network interface up (ifconfig
eth0 up) at the first time, the kernel space memory will be corrupted
(overwrited with packet content) start from 0xc03e8800.
Then kernel panics.

Here is what I discovered:
While ifconfig eth0 up kernel calls open() of 8139 driver(8139too.c).
In rtl8139_hw_start() of rtl8139_open(), 8139 driver enable RX before
setting up the DMA buffer
address.
Therefore in this interval where RX was enabled and DMA buffer address
is not yet set up, any incoming broadcast packet would be send to a strange
physical address: 0x003e8800 which is the default value of DMA buffer address.
Unfortunately, this address is in Linux kernel used by kmem_cache functions.
So, kernel panics. I have tried to fix the driver by setting up DMA
buffer address
before RX enabled and everything is fine.

I have checked 8139too.c in both 2.4.x kernel and 2.6.x kernel, they
both have the same initial flow.
Here is a simple patch to show you what I found.


@@ -1405,8 +1409,6 @@
       /* Lock Config[01234] and BMCR register writes */
       RTL_W8 (Cfg9346, Cfg9346_Lock);

-       /* init Rx ring buffer DMA address */
-       RTL_W32_F (RxBuf, tp->rx_ring_dma);

       /* init Tx buffer DMA addresses */
       for (i = 0; i < NUM_TX_DESC; i++)

Hope this can make the driver more robust.
FYR
Thanks a lot!

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

Comments

Eric Dumazet April 18, 2009, 1:01 p.m. UTC | #1 
Tzungder Lin a écrit :
> Dear Sirs,
> 
> Hello, I am jon from Taiwan.
> First I want to thank you for your great contributions of open source.
> Thank you for your 8139 driver to make our world better.
> 
> Here is what happens:
> While I am debugging our Embedded Linux SoC I found a flaw in Linux
> 8139 driver (8139too.c) ,probably.
> If I attack (interval < 10ms) the driver with broadcast packets
> (mac.destaddr == ff:ff:ff:ff:ff:ff) when network interface up (ifconfig
> eth0 up) at the first time, the kernel space memory will be corrupted
> (overwrited with packet content) start from 0xc03e8800.
> Then kernel panics.
> 
> Here is what I discovered:
> While ifconfig eth0 up kernel calls open() of 8139 driver(8139too.c).
> In rtl8139_hw_start() of rtl8139_open(), 8139 driver enable RX before
> setting up the DMA buffer
> address.
> Therefore in this interval where RX was enabled and DMA buffer address
> is not yet set up, any incoming broadcast packet would be send to a strange
> physical address: 0x003e8800 which is the default value of DMA buffer address.
> Unfortunately, this address is in Linux kernel used by kmem_cache functions.
> So, kernel panics. I have tried to fix the driver by setting up DMA
> buffer address
> before RX enabled and everything is fine.
> 
> I have checked 8139too.c in both 2.4.x kernel and 2.6.x kernel, they
> both have the same initial flow.
> Here is a simple patch to show you what I found.
> 
> --- 8139too.c   2007-12-13 15:54:26.000000000 +0800
> +++ 8139too_patch.c     2009-04-17 14:56:27.000000000 +0800
> @@ -1382,6 +1382,10 @@
>        RTL_W32_F (MAC0 + 0, cpu_to_le32 (*(u32 *) (dev->dev_addr + 0)));
>        RTL_W32_F (MAC0 + 4, cpu_to_le32 (*(u32 *) (dev->dev_addr + 4)));
> 
> +       /* init Rx ring buffer DMA address */
> +       /* init before Rx enabled to avoid broadcast packet attack in
> this interval */
> +       RTL_W32_F (RxBuf, tp->rx_ring_dma);
> +
>        /* Must enable Tx/Rx before setting transfer thresholds! */
>        RTL_W8 (ChipCmd, CmdRxEnb | CmdTxEnb);
> 
> @@ -1405,8 +1409,6 @@
>        /* Lock Config[01234] and BMCR register writes */
>        RTL_W8 (Cfg9346, Cfg9346_Lock);
> 
> -       /* init Rx ring buffer DMA address */
> -       RTL_W32_F (RxBuf, tp->rx_ring_dma);
> 
>        /* init Tx buffer DMA addresses */
>        for (i = 0; i < NUM_TX_DESC; i++)
> 
> Hope this can make the driver more robust.
> FYR
> Thanks a lot!
> 
> Regards
>  Jonathan Lin @Taiwan
>  2009.4.18

Hello Jonathan

This seems a nice catch !

What about also initializing tp->cur_rx = 0 *before* enabling RX too ?

So after patch we should have :

	tp->cur_rx = 0;
	RTL_W32_F (RxBuf, tp->rx_ring_dma);
        /* Must enable Tx/Rx before setting transfer thresholds! */
        RTL_W8 (ChipCmd, CmdRxEnb | CmdTxEnb);
 
Everything should be setup before enable RX interrupts coming...


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tzungder Lin April 18, 2009, 3:28 p.m. UTC | #2 
Dear Eric,

I agree with you at first.
But after I checked the driver code, I think tp->cur_rx has been set
to zero in rtl8139_init_ring() which has been executed before
rtl8139_hw_start().
So, it should be safe already.
Thanks for your advice still.
FYR

Regards
  jonathan

On Sat, Apr 18, 2009 at 9:01 PM, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Tzungder Lin a écrit :
>> Dear Sirs,
>>
>> Hello, I am jon from Taiwan.
>> First I want to thank you for your great contributions of open source.
>> Thank you for your 8139 driver to make our world better.
>>
>> Here is what happens:
>> While I am debugging our Embedded Linux SoC I found a flaw in Linux
>> 8139 driver (8139too.c) ,probably.
>> If I attack (interval < 10ms) the driver with broadcast packets
>> (mac.destaddr == ff:ff:ff:ff:ff:ff) when network interface up (ifconfig
>> eth0 up) at the first time, the kernel space memory will be corrupted
>> (overwrited with packet content) start from 0xc03e8800.
>> Then kernel panics.
>>
>> Here is what I discovered:
>> While ifconfig eth0 up kernel calls open() of 8139 driver(8139too.c).
>> In rtl8139_hw_start() of rtl8139_open(), 8139 driver enable RX before
>> setting up the DMA buffer
>> address.
>> Therefore in this interval where RX was enabled and DMA buffer address
>> is not yet set up, any incoming broadcast packet would be send to a strange
>> physical address: 0x003e8800 which is the default value of DMA buffer address.
>> Unfortunately, this address is in Linux kernel used by kmem_cache functions.
>> So, kernel panics. I have tried to fix the driver by setting up DMA
>> buffer address
>> before RX enabled and everything is fine.
>>
>> I have checked 8139too.c in both 2.4.x kernel and 2.6.x kernel, they
>> both have the same initial flow.
>> Here is a simple patch to show you what I found.
>>
>> --- 8139too.c   2007-12-13 15:54:26.000000000 +0800
>> +++ 8139too_patch.c     2009-04-17 14:56:27.000000000 +0800
>> @@ -1382,6 +1382,10 @@
>>        RTL_W32_F (MAC0 + 0, cpu_to_le32 (*(u32 *) (dev->dev_addr + 0)));
>>        RTL_W32_F (MAC0 + 4, cpu_to_le32 (*(u32 *) (dev->dev_addr + 4)));
>>
>> +       /* init Rx ring buffer DMA address */
>> +       /* init before Rx enabled to avoid broadcast packet attack in
>> this interval */
>> +       RTL_W32_F (RxBuf, tp->rx_ring_dma);
>> +
>>        /* Must enable Tx/Rx before setting transfer thresholds! */
>>        RTL_W8 (ChipCmd, CmdRxEnb | CmdTxEnb);
>>
>> @@ -1405,8 +1409,6 @@
>>        /* Lock Config[01234] and BMCR register writes */
>>        RTL_W8 (Cfg9346, Cfg9346_Lock);
>>
>> -       /* init Rx ring buffer DMA address */
>> -       RTL_W32_F (RxBuf, tp->rx_ring_dma);
>>
>>        /* init Tx buffer DMA addresses */
>>        for (i = 0; i < NUM_TX_DESC; i++)
>>
>> Hope this can make the driver more robust.
>> FYR
>> Thanks a lot!
>>
>> Regards
>>  Jonathan Lin @Taiwan
>>  2009.4.18
>
> Hello Jonathan
>
> This seems a nice catch !
>
> What about also initializing tp->cur_rx = 0 *before* enabling RX too ?
>
> So after patch we should have :
>
>        tp->cur_rx = 0;
>        RTL_W32_F (RxBuf, tp->rx_ring_dma);
>        /* Must enable Tx/Rx before setting transfer thresholds! */
>        RTL_W8 (ChipCmd, CmdRxEnb | CmdTxEnb);
>
> Everything should be setup before enable RX interrupts coming...
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet April 18, 2009, 7:43 p.m. UTC | #3 
Tzungder Lin a écrit :
> Dear Eric,
> 
> I agree with you at first.
> But after I checked the driver code, I think tp->cur_rx has been set
> to zero in rtl8139_init_ring() which has been executed before
> rtl8139_hw_start().
> So, it should be safe already.
> Thanks for your advice still.
> FYR

Very good ! Could you formally post the patch with your Signoff,
and make it applicable with "patch -p1" ?

(Please read Documentation/SubmittingPatches if you need some info)

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tzungder Lin April 19, 2009, 6:52 a.m. UTC | #4 
Dear Sirs,

Thank you very much.
I will follow it tonight as soon as possible.

Regards
  jonathan


On 4/19/09, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Tzungder Lin a écrit :
> > Dear Eric,
> >
> > I agree with you at first.
> > But after I checked the driver code, I think tp->cur_rx has been set
> > to zero in rtl8139_init_ring() which has been executed before
> > rtl8139_hw_start().
> > So, it should be safe already.
> > Thanks for your advice still.
> > FYR
>
> Very good ! Could you formally post the patch with your Signoff,
> and make it applicable with "patch -p1" ?
>
> (Please read Documentation/SubmittingPatches if you need some info)
>
> Thanks
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- 8139too.c   2007-12-13 15:54:26.000000000 +0800
+++ 8139too_patch.c     2009-04-17 14:56:27.000000000 +0800
@@ -1382,6 +1382,10 @@ 
       RTL_W32_F (MAC0 + 0, cpu_to_le32 (*(u32 *) (dev->dev_addr + 0)));
       RTL_W32_F (MAC0 + 4, cpu_to_le32 (*(u32 *) (dev->dev_addr + 4)));

+       /* init Rx ring buffer DMA address */
+       /* init before Rx enabled to avoid broadcast packet attack in
this interval */
+       RTL_W32_F (RxBuf, tp->rx_ring_dma);
+
       /* Must enable Tx/Rx before setting transfer thresholds! */
       RTL_W8 (ChipCmd, CmdRxEnb | CmdTxEnb);