[PATCH V2] arm64: Fix early pointer print plus improve comment

[Robin Murphy]

On 2021-12-21 14:49, Guilherme G. Piccoli wrote:
> When facing a really early issue on DT parsing we have currently
> a message that shows both the physical and virtual address of the
> FDT. The printk pointer modifier there is not right for the virtual
> address, due to the hashed address stuff, so hereby we fix that.

Strictly it *is* the right modifier, since users who want to see 
unhashed pointers should pass "no_hash_pointer" on the command line.

However, in this particular instance, the information leakage concern 
does not apply since we're facing such a catastrophic failure that the 
kernel can't even run - there's nothing for an attacker to attack! This 
is effectively a last-gasp panic message to help debug bootloader issues 
beyond the kernel's control, so it seems reasonable not to hamper it 
with kernel-debugging machinery.

It might be worth spelling out the rationale clearly, at least in the 
commit message, so it's there for easy future reference if someone comes 
along with a "%px is bad, change it back" patch. "Hashed address stuff" 
on its own isn't really a reason.

> Also, we tried to improve a bit the commenting on that function, given
> that if kernel fails there, it just hangs forever in a cpu_relax() loop.
> The reason we cannot BUG/panic is that is too early to do so; thanks to
> Mark Brown for pointing that on IRC.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at igalia.com>
> ---
> 
> V2: Fixing the right pointer here - it's the virtual one, not the
> physical! Thanks a bunch Robin Murphy for the review.
> 
>   arch/arm64/kernel/setup.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index be5f85b0a24d..172463ea6877 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -189,11 +189,15 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys)
>   
>   	if (!dt_virt || !early_init_dt_scan(dt_virt)) {
>   		pr_crit("\n"
> -			"Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
> +			"Error: invalid device tree blob at physical address %pa (virtual address 0x%px)\n"
>   			"The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
>   			"\nPlease check your bootloader.",
>   			&dt_phys, dt_virt);
>   

Nit: I think we prefer normal-style comments (i.e. "/*" on its own line 
to start) in arch code.

Otherwise, it all seems reasonable - thanks for clearing it up.

Robin.

> +		/* Note that in this _really_ early stage we cannot even BUG()
> +		 * or oops, so the least terrible thing to do is cpu_relax(),
> +		 * or else we could end-up printing non-initialized data, etc.
> +		 */
>   		while (true)
>   			cpu_relax();
>   	}