linux-kernel - Re: [PATCH] arm64: Remove d-cache clean operation at preserve_boot

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: <696a1a53-87b5-7517-3c05-82217c8eb190@arm.com>
Date:   Mon, 5 Sep 2022 11:46:50 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Jeungwoo Yoo <casionwoo@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>
Cc:     Hyeonggon Yoo <42.hyeyoo@...il.com>,
        Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>,
        Sangyun Kim <sangyun.kim@....ac.kr>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: Remove d-cache clean operation at
 preserve_boot_args().

On 2022-09-04 20:30, Jeungwoo Yoo wrote:
> Kernel expects only the clean operation as a booting requirement in
> arm64 architecture [1], therefore, the kernel has to invalidate any
> cache entries after accessing a memory in the booting time (before
> enabling D-cache and MMU) not to overwrite the memory with the stale
> cache entry.
> 
> Same applied in preserve_boot_args(), kernel saves boot arguments into
> 'boot_args' and invalidates the corresponding cache entry. However,
> according to the 'dcache_inval_poc()' implementation, the cache entry
> will be not only invalidated but also cleaned. That means if there is a
> stale cache entry corresponding to the address of the 'boot_args', the
> saved boot arguments in 'boot_args' will be overwritten by the stale
> cache entry. Therefore, it uses 'dv ivac' instruction directly instead
> of calling 'dcache_inval_poc()'.

You've already said in the first paragraph that we expect these 
locations to be clean. Clean lines are not written back, so your 
reasoning here is spurious. If boot_args has somehow become dirtied such 
that the clean operation *would* write back to memory, that can only 
mean one of two things: either the kernel image was not cleaned per the 
boot protocol, in which case there's every chance that other things will 
also go wrong elsewhere and there's not much we can do, or the prior 
stores hit in the cache (either unexpectedly or because the MMU was left 
on), in which case we almost certainly *would* want writeback here anyway.

> The address of the 'boot_args' is aligned to the cache line size and the
> size of 'boot_args' is 32 byte (8 byte * 4), therefore, a single
> invalidate operation is enough to invalidate the cache line belonging to
> the 'boot_args'.

The architecture allows the CWG to be as small as 2 words, so this is 
clearly untrue.

Thanks,
Robin.

> Sometimes clean operation is required not to lose any contents in the
> cache entry but not the target of the invalidation. However, in this
> case, there is no valid cache entries at a very early booting stage and
> preserve_boot_args() is not called by any other (non-primary) CPUs.
> Therefore, this invalidation operation will not introduce any problems.
> 
> [1] in Documentation/arm64/booting.rst:
> The address range corresponding to the loaded kernel image must be
> cleaned to the PoC.
> 
> Co-developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
> 
> Co-developed-by: Sangyun Kim <sangyun.kim@....ac.kr>
> Signed-off-by: Sangyun Kim <sangyun.kim@....ac.kr>
> 
> Signed-off-by: Jeungwoo Yoo <casionwoo@...il.com>
> ---
>   arch/arm64/kernel/head.S | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index cefe6a73ee54..916227666b07 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -121,9 +121,7 @@ SYM_CODE_START_LOCAL(preserve_boot_args)
>   
>   	dmb	sy				// needed before dc ivac with
>   						// MMU off
> -
> -	add	x1, x0, #0x20			// 4 x 8 bytes
> -	b	dcache_inval_poc		// tail call
> +	dc	ivac, x0			// Invalidate potentially stale cache line
>   SYM_CODE_END(preserve_boot_args)
>   
>   SYM_FUNC_START_LOCAL(clear_page_tables)