linux-kernel - Re: [PATCH] net: arc_emac: fix arc_emac

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: <20171219.102254.1959679351283055093.davem@davemloft.net>
Date:   Tue, 19 Dec 2017 10:22:54 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     al.kochet@...il.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        f.fainelli@...il.com, edumazet@...gle.com
Subject: Re: [PATCH] net: arc_emac: fix arc_emac_rx() error paths

From: Alexander Kochetkov <al.kochet@...il.com>
Date: Fri, 15 Dec 2017 20:20:06 +0300

> arc_emac_rx() has some issues found by code review.
> 
> In case netdev_alloc_skb_ip_align() or dma_map_single() failure
> rx fifo entry will not be returned to EMAC.
> 
> In case dma_map_single() failure previously allocated skb became
> lost to driver. At the same time address of newly allocated skb
> will not be provided to EMAC.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@...il.com>

This patch adds quite a few bugs, here is one which shows this is not
functionally tested:

> -		/* receive_skb only if new skb was allocated to avoid holes */
> -		netif_receive_skb(skb);
> -
> -		addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data,
> +		addr = dma_map_single(&ndev->dev, (void *)skb->data,
>  				      EMAC_BUFFER_SIZE, DMA_FROM_DEVICE);

Map the new SKB.

>  		if (dma_mapping_error(&ndev->dev, addr)) {
>  			if (net_ratelimit())
> -				netdev_err(ndev, "cannot dma map\n");
> -			dev_kfree_skb(rx_buff->skb);
> +				netdev_err(ndev, "cannot map dma buffer\n");
> +			dev_kfree_skb(skb);
> +			/* Return ownership to EMAC */
> +			rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
>  			stats->rx_errors++;
> +			stats->rx_dropped++;
>  			continue;
>  		}
> +
> +		/* unmap previosly mapped skb */
> +		dma_unmap_single(&ndev->dev, dma_unmap_addr(rx_buff, addr),
> +				 dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);

And then you unmap it.  "addr" is the new DMA mapping, not the old one.