Re: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 3766 bytes --]

On Wed, 2014-01-15 at 13:04 -0800, Florian Fainelli wrote:
> Ever since this driver was merged the following code was included:
> 
> if (skb->len < MISR)
> 	skb->len = MISR;

The second 'skb->len' is currently 'descptr->len'.

> MISR is defined to 0x3C which is also equivalent to ETH_ZLEN, but use
> ETH_ZLEN directly which is exactly what we want to be checking for.
> 
> Reported-by: Marc Volovic <marcv@ezchip.com>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
>  drivers/net/ethernet/rdc/r6040.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c
> index ff4683a..eb15ebf 100644
> --- a/drivers/net/ethernet/rdc/r6040.c
> +++ b/drivers/net/ethernet/rdc/r6040.c
> @@ -836,8 +836,8 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
>  	/* Set TX descriptor & Transmit it */
>  	lp->tx_free_desc--;
>  	descptr = lp->tx_insert_ptr;
> -	if (skb->len < MISR)
> -		descptr->len = MISR;
> +	if (skb->len < ETH_ZLEN)
> +		descptr->len = ETH_ZLEN;

It looks like this is just increasing the TX descriptor length so the
packet is tail-padded with whatever happens to come next in the skb.
This is absolutely incorrect behaviour and may leak sensitive
information.

Presumably it is necessary to pad the frame because the MAC is too lame
to do it, and the correct way to do that is using skb_padto(skb,
ETH_ZLEN).  But this may fail as it might have to allocate memory

Additionally, the driver DMA-maps a length of skb->len but needs to map
a length of descptr->len.

Finally, it doesn't check for failure of DMA mapping.

Tidying up naming should be way down the list of priorities for
fixing this code.

I think this will fix those problems, though I need to split it up into
multiple patches:

--- a/drivers/net/ethernet/rdc/r6040.c
+++ b/drivers/net/ethernet/rdc/r6040.c
@@ -816,6 +816,7 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 	struct r6040_descriptor *descptr;
 	void __iomem *ioaddr = lp->base;
 	unsigned long flags;
+	dma_addr_t dma_addr;
 
 	/* Critical Section */
 	spin_lock_irqsave(&lp->lock, flags);
@@ -828,24 +829,35 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 		return NETDEV_TX_BUSY;
 	}
 
-	/* Statistic Counter */
-	dev->stats.tx_packets++;
-	dev->stats.tx_bytes += skb->len;
 	/* Set TX descriptor & Transmit it */
-	lp->tx_free_desc--;
 	descptr = lp->tx_insert_ptr;
-	if (skb->len < MISR)
-		descptr->len = MISR;
-	else
-		descptr->len = skb->len;
+
+	/* MAC doesn't pad frames so we have to */
+	if (skb_padto(skb, ETH_ZLEN)) {
+		kfree_skb(skb);
+		goto out;
+	}
+	descptr->len = max_t(int, skb->len, ETH_ZLEN);
+
+	dma_addr = pci_map_single(lp->pdev, skb->data, descptr->len,
+				  PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(lp->pdev, dma_addr)) {
+		kfree_skb(skb);
+		goto out;
+	}
+
+	lp->tx_free_desc--;
 
 	descptr->skb_ptr = skb;
-	descptr->buf = cpu_to_le32(pci_map_single(lp->pdev,
-		skb->data, skb->len, PCI_DMA_TODEVICE));
+	descptr->buf = cpu_to_le32(dma_addr);
 	descptr->status = DSC_OWNER_MAC;
 
 	skb_tx_timestamp(skb);
 
+	/* Statistic Counter */
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+
 	/* Trigger the MAC to check the TX descriptor */
 	iowrite16(TM2TX, ioaddr + MTPR);
 	lp->tx_insert_ptr = descptr->vndescp;
@@ -854,6 +866,7 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 	if (!lp->tx_free_desc)
 		netif_stop_queue(dev);
 
+out:
 	spin_unlock_irqrestore(&lp->lock, flags);
 
 	return NETDEV_TX_OK;
---

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]