linux-kernel - Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error

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: <20170422192020.GA10084@electric-eye.fr.zoreil.com>
Date:   Sat, 22 Apr 2017 21:20:20 +0200
From:   Francois Romieu <romieu@...zoreil.com>
To:     Alexey Khoroshilov <khoroshilov@...ras.ru>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, ldv-project@...uxtesting.org
Subject: Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping
 error

Alexey Khoroshilov <khoroshilov@...ras.ru> :
[...]
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> index 729095db3e08..dfc64e1e31f9 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
[...]
> @@ -1183,6 +1193,32 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
>  		netif_start_queue(ndev);
>  
>  	return NETDEV_TX_OK;
> +
> +dma_error:
> +	do {
> +		free_idx = (free_idx + NR_TX_DESC - 1) % NR_TX_DESC;
> +		desc = dev->tx_descs + (free_idx * DESC_SIZE);
> +		cmdsts = le32_to_cpu(desc[DESC_CMDSTS]);
> +		len = cmdsts & CMDSTS_LEN_MASK;
> +		buf = desc_addr_get(desc + DESC_BUFPTR);
> +		if (desc == first_desc)
> +			pci_unmap_single(dev->pci_dev,
> +					buf,
> +					len,
> +					PCI_DMA_TODEVICE);
> +		else
> +			pci_unmap_page(dev->pci_dev,
> +					buf,
> +					len,
> +					PCI_DMA_TODEVICE);

(use tabs + spaces to indent: code should line up right after the parenthesis)

(premature line breaks imho)

(nevermind, both can be avoided :o) )

> +		desc[DESC_CMDSTS] = cpu_to_le32(0);
> +		mb();
> +	} while (desc != first_desc);
> +
> +dma_error_first:
> +	dev_kfree_skb_any(skb);
> +	ndev->stats.tx_errors++;
                    ^^^^^^^^^ -> should be tx_dropped
> +	return NETDEV_TX_OK;
>  }

You only need a single test in the error loop if you mimic the map loop.
Something like:

diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 729095d..5e2dbc9 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -1160,9 +1160,11 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 
 		buf = skb_frag_dma_map(&dev->pci_dev->dev, frag, 0,
 				       skb_frag_size(frag), DMA_TO_DEVICE);
+		if (dma_mapping_error(&dev->pci_dev->dev, buf))
+			goto err_unmap_frags;
 		dprintk("frag: buf=%08Lx  page=%08lx offset=%08lx\n",
 			(long long)buf, (long) page_to_pfn(frag->page),
 			frag->page_offset);
 		len = skb_frag_size(frag);
 		frag++;
 		nr_frags--;
@@ -1181,8 +1184,27 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 	/* Check again: we may have raced with a tx done irq */
 	if (stopped && (dev->tx_done_idx != tx_done_idx) && start_tx_okay(dev))
 		netif_start_queue(ndev);
-
+out:
 	return NETDEV_TX_OK;
+
+err_unmap_frags:
+	while (1) {
+		buf = desc_addr_get(desc + DESC_BUFPTR);
+		if (!--nr_frags)
+			break;
+
+		pci_unmap_page(dev->pci_dev, buf, len, PCI_DMA_TODEVICE);
+
+		free_idx = (free_idx - 1) % NR_TX_DESC;
+		desc = dev->tx_descs + (free_idx * DESC_SIZE);
+		len = le32_to_cpu(desc + DESC_CMDSTS) & CMDSTS_LEN_MASK;
+	}
+	pci_unmap_single(dev->pci_dev, buf, len, PCI_DMA_TODEVICE);
+
+err_free_skb:
+	dev_kfree_skb_any(skb);
+	ndev->stats.tx_dropped++;
+	goto out;
 }
 
 static void ns83820_update_stats(struct ns83820 *dev)


Thinking more about it, the driver seems rather unsafe if a failing
start_xmit closely follows a succeeding one. The driver should imho
map frags first *then* plug the remaining hole in the descriptor ring.
Until it does, the implicit assumption about descriptor ownership that
the error unroll loop relies on may be wrong.

-- 
Ueimor