| Message ID | 20211221144958.1529612-1-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] arm64: Fix early pointer print plus improve comment | expand |
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@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(); > }
On 21/12/2021 12:27, Robin Murphy wrote: > [...] > 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. > OK Robin, thanks for the review again, I agree with you and will resubmit. >> [...] > > 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. Cool, will fix that also in the V3 =) Cheers, Guilherme
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); + /* 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(); }
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. 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@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(-)