RE: [PATCH net 2/2] r8152: rx descriptor check

[Hayes Wang]

Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Tuesday, November 15, 2016 9:11 AM
[...]
> If it was possible to get it wrong once, it should be possible to
> get it wrong twice, especially if some part of the hardware design
> is recycled. I don't mean anything else.

I agree with you. However, I have to let it could be reproduced
for confirming it.

Besides, the behavior is different for PCIe and USB device. There
is no action of DMA for USB device. It is done by the USB host
controller. And, the USB host controller wouldn't allow the device
sends a data which is more than the size of the buffer.

Best Regards,
Hayes

Re: [PATCH net 2/2] r8152: rx descriptor check

[Francois Romieu]

Hayes Wang  :
> Francois Romieu [mailto:rom...@fr.zoreil.com]
> > Sent: Friday, November 11, 2016 8:13 PM
> [...]
> > Invalid packet size corrupted receive descriptors in Realtek's device
> > reminds of CVE-2009-4537.
> 
> Do you mean that the driver would get a packet exceed the size
> which is set to RxMaxSize ?

If it was possible to get it wrong once, it should be possible to
get it wrong twice, especially if some part of the hardware design
is recycled. I don't mean anything else.

I won't speculate about some cache consistency issue or some badly
aborted dma transaction to explain the memory corruption.

-- 
Ueimor

Re: [PATCH net 2/2] r8152: rx descriptor check

[David Miller]

From: Hayes Wang 
Date: Mon, 14 Nov 2016 07:23:51 +

> Mark Lord [mailto:ml...@pobox.com]
>> Sent: Monday, November 14, 2016 4:34 AM
> [...]
>> Perhaps the driver
>> is somehow accessing the buffer space again after doing usb_submit_urb()?
>> That would certainly produce this kind of behaviour.
> 
> I don't think so. First, the driver only read the received buffer.
> That is, the driver would not change (or write) the data. Second,
> The driver would lose the point address of the received buffer
> after submitting the urb to the USB host controller, until the
> transfer is completed by the USB host controller. That is, the
> driver doesn't how to access the buffer after calling usb_submit_urb().

This is why it's most likely some DMA implementation issue or similar.

RE: [PATCH net 2/2] r8152: rx descriptor check

[Hayes Wang]

Mark Lord [mailto:ml...@pobox.com]
> Sent: Monday, November 14, 2016 4:34 AM
[...]
> Perhaps the driver
> is somehow accessing the buffer space again after doing usb_submit_urb()?
> That would certainly produce this kind of behaviour.

I don't think so. First, the driver only read the received buffer.
That is, the driver would not change (or write) the data. Second,
The driver would lose the point address of the received buffer
after submitting the urb to the USB host controller, until the
transfer is completed by the USB host controller. That is, the
driver doesn't how to access the buffer after calling usb_submit_urb().

Best Regards,
Hayes

RE: [PATCH net 2/2] r8152: rx descriptor check

[Hayes Wang]

David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 14, 2016 1:40 AM
[...]
> If you add this patch now, there is a much smaller likelyhood that you
> will work with a high priority to figure out _why_ this is happening.
> 
> For all we know this could be a platform bug in the DMA API for the
> systems in question.
> 
> It could also be a bug elsewhere in the driver, either in setting up
> the descriptor DMA mappings or how the chip is programmed.
> 
> Either way the true cause must be found before we start throwing
> changes like this into the driver.

Our hw engineer could check our device, and I could check the
driver. However, for the other parts, such as the USB host
controller or memory, it is difficult for me to make sure whether
they are correct or not. I could only promise our devices and
driver work fine.

Best Regards,
Hayes

RE: [PATCH net 2/2] r8152: rx descriptor check

[Hayes Wang]

Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Friday, November 11, 2016 8:13 PM
[...]
> Invalid packet size corrupted receive descriptors in Realtek's device
> reminds of CVE-2009-4537.

Do you mean that the driver would get a packet exceed the size
which is set to RxMaxSize? I check it with our hw engineers.
They don't get any issue about RxMaxSize. And their test for
RxMaxSize register is fine.

> Is the silicium of both devices different enough to prevent the same
> exploit to happen ?

For this case, I don't think the device provide a invalid value
for the receive descriptors. However, the driver sees a different
value. That is why I say the memory is unbelievable.

Best Regards,
Hayes

Re: [PATCH net 2/2] r8152: rx descriptor check

[Mark Lord]

On 16-11-13 03:34 PM, Mark Lord wrote:
>
> The system I use it with is a 32-bit ppc476, with non-coherent RAM,
> and using 16KB page sizes.
> 
> The dongle instantly becomes a lot more reliable when r8152.c is updated
> to use usb_alloc_coherent() for URB buffers, rather than kmalloc().
> 
> Not sure why that would be though, as the USB stack normally would handle
> kmalloc'd buffers just fine.  It is calling the appropriate routines,
> which boil down to invalidating the dcache lines (for inbound bulk xfers)
> as part of usb_submit_urb(), and yet the problem there persists.
> 
> It could be caused by cache-line sharing with other allocations, but that 
> seems
> unlikely as the kmalloc() size is 16384 bytes per buffer.  Perhaps the driver
> is somehow accessing the buffer space again after doing usb_submit_urb()?
> That would certainly produce this kind of behaviour.
> 
> Or maybe there's just a memory barrier missing somewhere in path.
> 
> The really weird thing is that ASIX-based dongles (which use a different 
> driver)
> don't have this problem, and yet they also use kmalloc'd buffers.
> 
> I have access to the test system only for a day or two a week,
> and it takes a few hours to do a good test as to whether something helps or 
> not.
> I'll continue to poke at it as time and New Ideas permit.

Oh, and the problems did not exist with the 3.14.xx kernels and earlier.
They began to show up when we tried 3.16.xx and all newer kernels.

The difference there is that RX checksums were enabled in hardware as of 
3.16.xx,
and thus the network stack began accepting bad packets from the r8152 driver.

I don't know if the ASIX driver uses hardware checksums or just software 
checksums.
That might explain why it is more reliable here.
-- 
Mark Lord
Real-Time Remedies Inc.
ml...@pobox.com

Re: [PATCH net 2/2] r8152: rx descriptor check

[Mark Lord]

On 16-11-13 12:39 PM, David Miller wrote:
> From: Hayes Wang 
> Date: Fri, 11 Nov 2016 15:15:41 +0800
> 
>> For some platforms, the data in memory is not the same with the one
>> from the device. That is, the data of memory is unbelievable. The
>> check is used to find out this situation.
>>
>> Signed-off-by: Hayes Wang 
> 
> I'm all for adding consistency checks, but I disagree with proceeding
> in this manner for this.
> 
> If you add this patch now, there is a much smaller likelyhood that you
> will work with a high priority to figure out _why_ this is happening.
> 
> For all we know this could be a platform bug in the DMA API for the
> systems in question.
> 
> It could also be a bug elsewhere in the driver, either in setting up
> the descriptor DMA mappings or how the chip is programmed.
> 
> Either way the true cause must be found before we start throwing
> changes like this into the driver.

I agree.

The system I use it with is a 32-bit ppc476, with non-coherent RAM,
and using 16KB page sizes.

The dongle instantly becomes a lot more reliable when r8152.c is updated
to use usb_alloc_coherent() for URB buffers, rather than kmalloc().

Not sure why that would be though, as the USB stack normally would handle
kmalloc'd buffers just fine.  It is calling the appropriate routines,
which boil down to invalidating the dcache lines (for inbound bulk xfers)
as part of usb_submit_urb(), and yet the problem there persists.

It could be caused by cache-line sharing with other allocations, but that seems
unlikely as the kmalloc() size is 16384 bytes per buffer.  Perhaps the driver
is somehow accessing the buffer space again after doing usb_submit_urb()?
That would certainly produce this kind of behaviour.

Or maybe there's just a memory barrier missing somewhere in path.

The really weird thing is that ASIX-based dongles (which use a different driver)
don't have this problem, and yet they also use kmalloc'd buffers.

I have access to the test system only for a day or two a week,
and it takes a few hours to do a good test as to whether something helps or not.
I'll continue to poke at it as time and New Ideas permit.

New Ideas welcome!
-- 
Mark Lord
Real-Time Remedies Inc.
ml...@pobox.com

Re: [PATCH net 2/2] r8152: rx descriptor check

[David Miller]

From: Hayes Wang 
Date: Fri, 11 Nov 2016 15:15:41 +0800

> For some platforms, the data in memory is not the same with the one
> from the device. That is, the data of memory is unbelievable. The
> check is used to find out this situation.
> 
> Signed-off-by: Hayes Wang 

I'm all for adding consistency checks, but I disagree with proceeding
in this manner for this.

If you add this patch now, there is a much smaller likelyhood that you
will work with a high priority to figure out _why_ this is happening.

For all we know this could be a platform bug in the DMA API for the
systems in question.

It could also be a bug elsewhere in the driver, either in setting up
the descriptor DMA mappings or how the chip is programmed.

Either way the true cause must be found before we start throwing
changes like this into the driver.

I'm not applying this series, sorry.

Re: [PATCH net 2/2] r8152: rx descriptor check

[Mark Lord]

On 16-11-11 07:13 AM, Francois Romieu wrote:
> Hayes Wang  :
>> For some platforms, the data in memory is not the same with the one
>> from the device. That is, the data of memory is unbelievable. The
>> check is used to find out this situation.
> 
> Invalid packet size corrupted receive descriptors in Realtek's device
> reminds of CVE-2009-4537.
> 
> Is the silicium of both devices different enough to prevent the same
> exploit to happen ?

I don't know if the hardware can do it, but the existing Linux device
driver regularly attempts to process huge unreal packet sizes here.
I've had to patch it to reject "packets" larger than the configured MRU.
-- 
Mark Lord
Real-Time Remedies Inc.
ml...@pobox.com

Re: [PATCH net 2/2] r8152: rx descriptor check

[Francois Romieu]

Hayes Wang  :
> For some platforms, the data in memory is not the same with the one
> from the device. That is, the data of memory is unbelievable. The
> check is used to find out this situation.

Invalid packet size corrupted receive descriptors in Realtek's device
reminds of CVE-2009-4537.

Is the silicium of both devices different enough to prevent the same
exploit to happen ?

-- 
Ueimor

[PATCH net 2/2] r8152: rx descriptor check

[Hayes Wang]

For some platforms, the data in memory is not the same with the one
from the device. That is, the data of memory is unbelievable. The
check is used to find out this situation.

Signed-off-by: Hayes Wang 
---
 drivers/net/usb/r8152.c | 49 +
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0e42a78..e766121 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1756,6 +1756,43 @@ static u8 r8152_rx_csum(struct r8152 *tp, struct rx_desc 
*rx_desc)
return checksum;
 }
 
+static int invalid_rx_desc(struct r8152 *tp, struct rx_desc *rx_desc)
+{
+   u32 opts1 = le32_to_cpu(rx_desc->opts1);
+   u32 opts2 = le32_to_cpu(rx_desc->opts2);
+   unsigned int pkt_len = opts1 & RX_LEN_MASK;
+
+   switch (tp->version) {
+   case RTL_VER_01:
+   case RTL_VER_02:
+   if (pkt_len > RTL8152_RMS)
+   return -EIO;
+   break;
+   default:
+   if (pkt_len > RTL8153_RMS)
+   return -EIO;
+   break;
+   }
+
+   switch (opts2 & (RD_IPV4_CS | RD_IPV6_CS)) {
+   case (RD_IPV4_CS | RD_IPV6_CS):
+   return -EIO;
+   case RD_IPV4_CS:
+   case RD_IPV6_CS:
+   switch (opts2 & (RD_UDP_CS | RD_TCP_CS)) {
+   case (RD_UDP_CS | RD_TCP_CS):
+   return -EIO;
+   default:
+   break;
+   }
+   break;
+   default:
+   break;
+   }
+
+   return 0;
+}
+
 static int rx_bottom(struct r8152 *tp, int budget)
 {
unsigned long flags;
@@ -1812,6 +1849,18 @@ static int rx_bottom(struct r8152 *tp, int budget)
unsigned int pkt_len;
struct sk_buff *skb;
 
+   if (unlikely(invalid_rx_desc(tp, rx_desc))) {
+   if (net_ratelimit())
+   netif_err(tp, rx_err, netdev,
+ "Memory unbelievable\n");
+   if (tp->netdev->features & NETIF_F_RXCSUM) {
+   tp->netdev->features &= ~NETIF_F_RXCSUM;
+   netif_err(tp, rx_err, netdev,
+ "rx checksum off\n");
+   }
+   break;
+   }
+
pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
if (pkt_len < ETH_ZLEN)
break;
-- 
2.7.4