[PATCH] uboot-lantiq: fix out of bounds cache invalidate

[Mathias Kresin]

11/3/21 2:43 PM, Daniel Schwierzeck:
> Am Dienstag, dem 02.11.2021 um 23:35 +0100 schrieb Mathias Kresin:
>> If u-boot is compiled with gcc 10+ the relevant part of the memory is
>> used as following:
>>
>> variable BootFile is at address: 0x83ffe4ec
>> variable data is at address:     0x83ffdee2
>> variable data has len:           0x600
>>
>> Within invalidate_dcache_range() the cache covering the data variable
>> should be invalidated (0x83ffdee2 till 0x83ffdef2).
>>
>> Due to dcache line size of 32 and some alignment correction, the
>> actual
>> memory range for which the cache is invalidated is from 0x83ffdee0
>> till
>> 0x83ffe500, which includes the location of the BootFile variable as
>> well.
>>
>> Afterwards the BootFile variable is 0. The issue can be observed by
>> using tftpboot, which always uses the default/generated filename
>> instead
>> of the specified one.
>>
>> To fix the issue, align the memory addresses properly before passing
>> them to invalidate_dcache_range(). The misalignment can be observed
>> with
>> older gcc versions as well but the BootFile variable is placed more
>> than
>> 0x1000 bytes after the data variable and doesn't get null'ed.
>>
>> Fixes: FS#4113
>>
>> Signed-off-by: Mathias Kresin <dev at kresin.me>
>> ---
>>   ...q-fix-out-of-bounds-cache-invalidate.patch | 48
>> +++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>   create mode 100644 package/boot/uboot-lantiq/patches/0031-dma-
>> lantiq-fix-out-of-bounds-cache-invalidate.patch
>>
>> diff --git a/package/boot/uboot-lantiq/patches/0031-dma-lantiq-fix-
>> out-of-bounds-cache-invalidate.patch b/package/boot/uboot-
>> lantiq/patches/0031-dma-lantiq-fix-out-of-bounds-cache-
>> invalidate.patch
>> new file mode 100644
>> index 0000000000..5a2bf39441
>> --- /dev/null
>> +++ b/package/boot/uboot-lantiq/patches/0031-dma-lantiq-fix-out-of-
>> bounds-cache-invalidate.patch
>> @@ -0,0 +1,48 @@
>> +From f10ffb65865d5bfa5679293b13e027e0474df1fe Mon Sep 17 00:00:00
>> 2001
>> +From: Mathias Kresin <dev at kresin.me>
>> +Date: Tue, 2 Nov 2021 21:24:29 +0100
>> +Subject: [PATCH] dma: lantiq: fix out of bounds cache invalidate
>> +
>> +If u-boot is compiled with gcc 10+ the relevant part of the memory
>> is
>> +used as following:
>> +
>> +variable BootFile is at address: 0x83ffe4ec
>> +variable data is at address:     0x83ffdee2
>> +variable data has len:           0x600
>> +
>> +Within invalidate_dcache_range() the cache covering the data
>> variable
>> +should be invalidated (0x83ffdee2 till 0x83ffdef2).
>> +
>> +Due to dcache line size of 32 and some alignment correction, the
>> actual
>> +memory range for which the cache is invalidated is from 0x83ffdee0
>> till
>> +0x83ffe500, which includes the location of the BootFile variable as
>> +well.
>> +
>> +Afterwards the BootFile variable is 0. The issue can be observed by
>> +using tftpboot, which always uses the default/generated filename
>> instead
>> +of the specified one.
>> +
>> +To fix the issue, align the memory addresses properly before passing
>> +them to invalidate_dcache_range(). The misalignment can be observed
>> with
>> +older gcc versions as well but the BootFile variable is placed more
>> than
>> +0x1000 bytes after the data variable and doesn't get null'ed.
>> +
>> +Signed-off-by: Mathias Kresin <dev at kresin.me>
>> +---
>> + drivers/dma/lantiq_dma.c | 4 ++--
>> + 1 file changed, 2 insertions(+), 2 deletions(-)
>> +
>> +--- a/drivers/dma/lantiq_dma.c
>> ++++ b/drivers/dma/lantiq_dma.c
>> +@@ -122,9 +122,9 @@ static inline void ltq_dma_dcache_wb_inv
>> +
>> + static inline void ltq_dma_dcache_inv(const void *ptr, size_t size)
>> + {
>> +-	unsigned long addr = (unsigned long) ptr;
>> ++	unsigned long addr = (unsigned long) ptr & ~(ARCH_DMA_MINALIGN
>> - 1);
>> +
>> +-	invalidate_dcache_range(addr, addr + size);
>> ++	invalidate_dcache_range(addr, ALIGN(addr + size,
>> ARCH_DMA_MINALIGN));
>> + }
> 
> actually invalidate_dcache_range() already aligns start and end address
> to CONFIG_SYS_CACHELINE_SIZE which is the same as ARCH_DMA_MINALIGN.
> 
> In mainline we fixed the cache functions to always issue a sync() after
> the cache ops to wait for all HW transactions to be complete. The
> Lantiq DMA driver only does this in ltq_dma_dcache_wb_inv() but not
> in ltq_dma_dcache_inv(). Maybe adding a ltq_dma_sync() there would also
> fix your problem?
> 
>> +
>> + void ltq_dma_init(void)

Hey Daniel,

a had an even closer look at what is going on here. It all starts in the 
soc netdev driver (here lantiq_danube_etop.c).

NetRxPackets[] points to cache line size aligned addresses. In 
ltq_eth_rx_packet_align() the address NetRxPackets[] points to is 
increased by LTQ_ETH_IP_ALIGN and the resulting not cache aligned 
address is used further on. While doing so, the len/size is never updated.

The "not cache aligned address" + len/size for a cache aligned address 
is passed to invalidate_dcache_range(). Hence, invalidate_dcache_range() 
invalidates the next 32 bit as well, which causes the issue.

   variable BootFile is at address: 0x83ffe12c
   NetRxPackets[] points to 0x83ffdb20 (len is 0x600)
   data points to: 0x83ffdb22 (len is 0x600)

   ltq_dma_dcache_inv: 0x83ffdb22 (for len 0x600)
   invalidate_dcache_range: 0x83ffdb20 to 0x83ffe120 (size: 32)
   invalidate_dcache_range: 0x83ffdb20 to 0x83ffdb40 (Bootfile: a.bin)
   ...
   invalidate_dcache_range: 0x83ffe100 to 0x83ffe120 (Bootfile: a.bin)
   invalidate_dcache_range: 0x83ffe120 to 0x83ffe140 (Bootfile: )

I see two possible approaches to fix the issue.

1. Do not move the pointer by LTQ_ETH_IP_ALIGN:

   To my own surprise Ethernet still works. But I've no idea why the
   pointer has to be moved by LTQ_ETH_IP_ALIGN nor why it still works if
   the pointer isn't moved. I'm quite sure the pointer is moved for a
   reason, which I do not get yet.

2. fix the alignment in ltq_dma_dcache_inv() as in my proposed change

   In the end, the pointer/startaddress gets corrected to the value it
   should be (0x83ffdb22 => 0x83ffdb20), the size matches again and only
   the expected cache range gets invalidated.

Any recommendation which approach is the more sane one? Since I don't 
understand the purpose of LTQ_ETH_IP_ALIGN, I tend to go with 2., but 
with an updated commit message.

Mathias