Re: [PATCH 1/2] net: xilinx_emaclite: fix receive buffer overflow

2017-02-15 Thread David Miller 
From: Anssi Hannula 
Date: Tue, 14 Feb 2017 19:11:44 +0200

> xilinx_emaclite looks at the received data to try to determine the
> Ethernet packet length but does not properly clamp it if
> proto_type == ETH_P_IP or 1500 < proto_type <= 1518, causing a buffer
> overflow and a panic via skb_panic() as the length exceeds the allocated
> skb size.
> 
> Fix those cases.
> 
> Also add an additional unconditional check with WARN_ON() at the end.
> 
> Signed-off-by: Anssi Hannula 
> Fixes: bb81b2ddfa19 ("net: add Xilinx emac lite device driver")

Applied.

RE: [PATCH 1/2] net: xilinx_emaclite: fix receive buffer overflow

2017-02-15 Thread David Laight 
From: Anssi Hannula
> Sent: 15 February 2017 08:29
...
> Looking through the product guide [1] I don't see the actual receive
> packet length provided anywhere, so I guess that is why the crazy stuff
> is done.

If the hardware doesn't provide the receive packet length then I suggest
you 'fix' the hardware with an angle grinder :-)
Not fit for purpose.

David

Re: [PATCH 1/2] net: xilinx_emaclite: fix receive buffer overflow

2017-02-15 Thread Anssi Hannula 
On 14.2.2017 22:12, David Miller wrote:
> From: Anssi Hannula 
> Date: Tue, 14 Feb 2017 19:11:44 +0200
>
>> xilinx_emaclite looks at the received data to try to determine the
>> Ethernet packet length but does not properly clamp it if
>> proto_type == ETH_P_IP or 1500 < proto_type <= 1518, causing a buffer
>> overflow and a panic via skb_panic() as the length exceeds the allocated
>> skb size.
>>
>> Fix those cases.
>>
>> Also add an additional unconditional check with WARN_ON() at the end.
>>
>> Signed-off-by: Anssi Hannula 
>> Fixes: bb81b2ddfa19 ("net: add Xilinx emac lite device driver")
> Why does this driver do all of this crazy stuff parsing the packet
> headers?
>
> It should be able to just read the length provided by the device
> at XEL_RPLR_OFFSET and just use that.

Looks like XEL_RPLR_OFFSET == XEL_HEADER_OFFSET + XEL_RXBUFF_OFFSET and
that is where the driver reads the on-wire Type/Length field.

Looking through the product guide [1] I don't see the actual receive
packet length provided anywhere, so I guess that is why the crazy stuff
is done.

[1]
https://www.xilinx.com/support/documentation/ip_documentation/axi_ethernetlite/v3_0/pg135-axi-ethernetlite.pdf

-- 
Anssi Hannula / Bitwise Oy
+358503803997

Re: [PATCH 1/2] net: xilinx_emaclite: fix receive buffer overflow

2017-02-14 Thread David Miller 
From: Anssi Hannula 
Date: Tue, 14 Feb 2017 19:11:44 +0200

> xilinx_emaclite looks at the received data to try to determine the
> Ethernet packet length but does not properly clamp it if
> proto_type == ETH_P_IP or 1500 < proto_type <= 1518, causing a buffer
> overflow and a panic via skb_panic() as the length exceeds the allocated
> skb size.
> 
> Fix those cases.
> 
> Also add an additional unconditional check with WARN_ON() at the end.
> 
> Signed-off-by: Anssi Hannula 
> Fixes: bb81b2ddfa19 ("net: add Xilinx emac lite device driver")

Why does this driver do all of this crazy stuff parsing the packet
headers?

It should be able to just read the length provided by the device
at XEL_RPLR_OFFSET and just use that.

[PATCH 1/2] net: xilinx_emaclite: fix receive buffer overflow

2017-02-14 Thread Anssi Hannula 
xilinx_emaclite looks at the received data to try to determine the
Ethernet packet length but does not properly clamp it if
proto_type == ETH_P_IP or 1500 < proto_type <= 1518, causing a buffer
overflow and a panic via skb_panic() as the length exceeds the allocated
skb size.

Fix those cases.

Also add an additional unconditional check with WARN_ON() at the end.

Signed-off-by: Anssi Hannula 
Fixes: bb81b2ddfa19 ("net: add Xilinx emac lite device driver")
---

 drivers/net/ethernet/xilinx/xilinx_emaclite.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c 
b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
index e3070fd8..bd6470f 100644
--- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
+++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
@@ -369,7 +369,7 @@ static int xemaclite_send_data(struct net_local *drvdata, 
u8 *data,
  *
  * Return: Total number of bytes received
  */
-static u16 xemaclite_recv_data(struct net_local *drvdata, u8 *data)
+static u16 xemaclite_recv_data(struct net_local *drvdata, u8 *data, int maxlen)
 {
void __iomem *addr;
u16 length, proto_type;
@@ -409,7 +409,7 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, 
u8 *data)
 
/* Check if received ethernet frame is a raw ethernet frame
 * or an IP packet or an ARP packet */
-   if (proto_type > (ETH_FRAME_LEN + ETH_FCS_LEN)) {
+   if (proto_type > ETH_DATA_LEN) {
 
if (proto_type == ETH_P_IP) {
length = ((ntohl(__raw_readl(addr +
@@ -417,6 +417,7 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, 
u8 *data)
XEL_RXBUFF_OFFSET)) >>
XEL_HEADER_SHIFT) &
XEL_RPLR_LENGTH_MASK);
+   length = min_t(u16, length, ETH_DATA_LEN);
length += ETH_HLEN + ETH_FCS_LEN;
 
} else if (proto_type == ETH_P_ARP)
@@ -429,6 +430,9 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, 
u8 *data)
/* Use the length in the frame, plus the header and trailer */
length = proto_type + ETH_HLEN + ETH_FCS_LEN;
 
+   if (WARN_ON(length > maxlen))
+   length = maxlen;
+
/* Read from the EmacLite device */
xemaclite_aligned_read((u32 __force *) (addr + XEL_RXBUFF_OFFSET),
data, length);
@@ -603,7 +607,7 @@ static void xemaclite_rx_handler(struct net_device *dev)
 
skb_reserve(skb, 2);
 
-   len = xemaclite_recv_data(lp, (u8 *) skb->data);
+   len = xemaclite_recv_data(lp, (u8 *) skb->data, len);
 
if (!len) {
dev->stats.rx_errors++;
-- 
2.8.3