linux-kernel - RE: [PATCH] dmaengine: dw-edma: fix endianess confusion

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: <DM6PR12MB40101798EDD46EF4B8E0E0E1DAE70@DM6PR12MB4010.namprd12.prod.outlook.com>
Date:   Fri, 21 Jun 2019 08:42:32 +0000
From:   Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>,
        Vinod Koul <vkoul@...nel.org>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] dmaengine: dw-edma: fix endianess confusion

Hi,

On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <arnd@...db.de> wrote:

> When building with 'make C=1', sparse reports an endianess bug:

I didn't know that option.

> 
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:60:30: warning: cast removes address space of expression
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24:    expected void const volatile [noderef] <asn:2>*addr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24:    got void *[assigned] ptr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24:    expected void const volatile [noderef] <asn:2>*addr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24:    got void *[assigned] ptr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24:    expected void const volatile [noderef] <asn:2>*addr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24:    got void *[assigned] ptr
> 
> The current code is clearly wrong, as it passes an endian-swapped word
> into a register function where it gets swapped again. I assume that this

Sorry I didn't catch this, endianness-swapped word into a register 
function where it gets swapped again?
Where?

> was simply ported from a non-Linux OS, and the swap was done incorrectly.
> Replace it with a cast to uintptr_t.
> 
> Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  drivers/dma/dw-edma/dw-edma-v0-core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 97e3fd41c8a8..d670ebcc37b3 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -195,7 +195,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	struct dw_edma_v0_lli __iomem *lli;
>  	struct dw_edma_v0_llp __iomem *llp;
>  	u32 control = 0, i = 0;
> -	u64 sar, dar, addr;
> +	uintptr_t sar, dar, addr;

Will this type assure variables sar, dar and addr are 64 bits?

>  	int j;
>  
>  	lli = chunk->ll_region.vaddr;
> @@ -214,11 +214,11 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  		/* Transfer size */
>  		SET_LL(&lli[i].transfer_size, child->sz);
>  		/* SAR - low, high */
> -		sar = cpu_to_le64(child->sar);
> +		sar = (uintptr_t)child->sar;

Assuming the host is a big-endian machine and the eDMA on the endpoint 
strictly requires the address to be little endian.
By not using cpu_to_le64(), the address to be written on the eDMA will be 
in big-endian format, right? If so, that will break the driver.

>  		SET_LL(&lli[i].sar_low, lower_32_bits(sar));
>  		SET_LL(&lli[i].sar_high, upper_32_bits(sar));
>  		/* DAR - low, high */
> -		dar = cpu_to_le64(child->dar);
> +		dar = (uintptr_t)child->dar;

Ditto.

>  		SET_LL(&lli[i].dar_low, lower_32_bits(dar));
>  		SET_LL(&lli[i].dar_high, upper_32_bits(dar));
>  		i++;
> @@ -232,7 +232,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	/* Channel control */
>  	SET_LL(&llp->control, control);
>  	/* Linked list  - low, high */
> -	addr = cpu_to_le64(chunk->ll_region.paddr);
> +	addr = (uintptr_t)chunk->ll_region.paddr;

Ditto.

>  	SET_LL(&llp->llp_low, lower_32_bits(addr));
>  	SET_LL(&llp->llp_high, upper_32_bits(addr));
>  }
> @@ -262,7 +262,7 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		SET_CH(dw, chan->dir, chan->id, ch_control1,
>  		       (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
>  		/* Linked list - low, high */
> -		llp = cpu_to_le64(chunk->ll_region.paddr);
> +		llp = (uintptr_t)chunk->ll_region.paddr;

Ditto.

>  		SET_CH(dw, chan->dir, chan->id, llp_low, lower_32_bits(llp));
>  		SET_CH(dw, chan->dir, chan->id, llp_high, upper_32_bits(llp));
>  	}
> -- 
> 2.20.0