Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] p54: avoid accessing the data mapped to streaming DMA
To:     Jia-Ju Bai <baijiaju@tsinghua.edu.cn>, kvalo@codeaurora.org,
        kuba@kernel.org
Cc:     linux-wireless@vger.kernel.org
References: <20200802132949.26788-1-baijiaju@tsinghua.edu.cn>
From:   Christian Lamparter <chunkeey@gmail.com>
Message-ID: <0b432282-8d52-3004-6648-d97c03cdae28@gmail.com>
Date:   Tue, 18 Aug 2020 19:27:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200802132949.26788-1-baijiaju@tsinghua.edu.cn>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hello,

sorry. Your mail got flagged by google mail as spam. :(

On 2020-08-02 15:29, Jia-Ju Bai wrote:
> In p54p_tx(), skb->data is mapped to streaming DMA on line 337:
>    mapping = pci_map_single(..., skb->data, ...);
> 
> Then skb->data is accessed on line 349:
>    desc->device_addr = ((struct p54_hdr *)skb->data)->req_id;
> 
> This access may cause data inconsistency between CPU cache and hardware.
> 
> To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in
> a local variable before DMA mapping, and then the driver accesses this
> local variable instead of skb->data.

Interesting. Please bear with me here. From my understanding, the 
streaming direction is set to PCI_DMA_TODEVICE. So is it really possible
for the hardware to interfere with the data without the IOMMU catching this?

(That said, patch looks be fine. I'll need to dust off a old PCI PC to 
check this with real hardware, if requested.)

Cheers,
Christian


> 
> Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
> ---
>   drivers/net/wireless/intersil/p54/p54pci.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c
> index 80ad0b7eaef4..f8c6027cab6b 100644
> --- a/drivers/net/wireless/intersil/p54/p54pci.c
> +++ b/drivers/net/wireless/intersil/p54/p54pci.c
> @@ -329,10 +329,12 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb)
>   	struct p54p_desc *desc;
>   	dma_addr_t mapping;
>   	u32 idx, i;
> +	__le32 device_addr;
>   
>   	spin_lock_irqsave(&priv->lock, flags);
>   	idx = le32_to_cpu(ring_control->host_idx[1]);
>   	i = idx % ARRAY_SIZE(ring_control->tx_data);
> +	device_addr = ((struct p54_hdr *)skb->data)->req_id;
>   
>   	mapping = pci_map_single(priv->pdev, skb->data, skb->len,
>   				 PCI_DMA_TODEVICE);
> @@ -346,7 +348,7 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb)
>   
>   	desc = &ring_control->tx_data[i];
>   	desc->host_addr = cpu_to_le32(mapping);
> -	desc->device_addr = ((struct p54_hdr *)skb->data)->req_id;
> +	desc->device_addr = device_addr;
>   	desc->len = cpu_to_le16(skb->len);
>   	desc->flags = 0;
>   
>