linux-kernel - Re: [PATCH 0/2] riscv: implement Zicbom-based CMO instructions + the t-head variant

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]

Date:   Sat, 16 Apr 2022 12:47:29 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Corentin Labbe <clabbe.montjoie@...il.com>
Cc:     Heiko Stuebner <heiko@...ech.de>, palmer@...belt.com,
        paul.walmsley@...ive.com, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, wefu@...hat.com, guoren@...nel.org,
        atishp@...shpatra.org, anup@...infault.org, mick@....forth.gr,
        cmuellner@...ux.com, philipp.tomsich@...ll.eu
Subject: Re: [PATCH 0/2] riscv: implement Zicbom-based CMO instructions + the
 t-head variant

On 4/16/22 2:35 AM, Corentin Labbe wrote:
> Le Fri, Apr 15, 2022 at 09:19:23PM -0500, Samuel Holland a écrit :
>> On 4/15/22 6:26 AM, Corentin Labbe wrote:
>>> Le Mon, Mar 07, 2022 at 11:46:18PM +0100, Heiko Stuebner a écrit :
>>>> This series is based on the alternatives changes done in my svpbmt series
>>>> and thus also depends on Atish's isa-extension parsing series.
>>>>
>>>> It implements using the cache-management instructions from the  Zicbom-
>>>> extension to handle cache flush, etc actions on platforms needing them.
>>>>
>>>> SoCs using cpu cores from T-Head like the Allwinne D1 implement a
>>>> different set of cache instructions. But while they are different,
>>>> instructions they provide the same functionality, so a variant can
>>>> easly hook into the existing alternatives mechanism on those.
>>>>
>>>>
>>>
>>> Hello
>>>
>>> I am testing https://github.com/smaeul/linux.git branch:origin/riscv/d1-wip which contain this serie.
>>>
>>> I am hitting a buffer corruption problem with DMA.
>>> The sun8i-ce crypto driver fail self tests due to "device overran destination buffer".
>>> In fact the buffer is not overran by device but by dma_map_single() operation.
>>>
>>> The following small code show the problem:
>>>
>>> dma_addr_t dma;
>>> u8 *buf;
>>> #define BSIZE 2048
>>> #define DMASIZE 16
>>>
>>> buf = kmalloc(BSIZE, GFP_KERNEL | GFP_DMA);
>>> for (i = 0; i < BSIZE; i++)
>>>     buf[i] = 0xFE;
>>> print_hex_dump(KERN_INFO, "DMATEST1:", DUMP_PREFIX_NONE, 16, 4, buf, 256, false);
>>> dma = dma_map_single(ce->dev, buf, DMASIZE, DMA_FROM_DEVICE);
>>
>> This function (through dma_direct_map_page()) ends up calling
>> arch_sync_dma_for_device(..., ..., DMA_FROM_DEVICE), which invalidates the CPU's
>> cache. This is the same thing other architectures do (at least arm, arm64,
>> openrisc, and powerpc). So this appears to be working as intended.
> 
> This behavour is not present at least on ARM and ARM64.
> The sample code I provided does not corrupt the buffer on them.

That can be explained by the 0xFE bytes having been flushed to DRAM already in
your ARM/ARM64 tests, whereas in your riscv64 case, the 0xFE bytes were still in
a dirty cache line. The cache topology and implementation is totally different
across the SoCs, so this is not too surprising.

Semantically, dma_map_single(..., DMA_FROM_DEVICE) means you are doing a
unidirectional DMA transfer from the device into that buffer. So the contents of
the buffer are "undefined" until the DMA transfer completes. If you are also
writing data into the buffer from the CPU side, then you need DMA_BIDIRECTIONAL.

Regards,
Samuel