powerpc/prom_init: Check display props exist before enabling btext

It's possible to enable CONFIG_PPC_EARLY_DEBUG_BOOTX for a pseries
kernel (maybe it shouldn't be), which is then booted with qemu/slof.

But if you do that the kernel crashes in draw_byte(), with a DAR
pointing somewhere near INT_MAX.

Adding some debug to prom_init we see that we're not able to read the
"address" property from OF, so we're just using whatever junk value
was on the stack.

So check the properties can be read properly from OF, if not we bail
out before initialising btext, which avoids the crash.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/prom_init.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

On 21/08/2020 20:34, Michael Ellerman wrote:
> It's possible to enable CONFIG_PPC_EARLY_DEBUG_BOOTX for a pseries
> kernel (maybe it shouldn't be), which is then booted with qemu/slof.


CONFIG_BOOTX_TEXT=y
CONFIG_PPC_EARLY_DEBUG=y
CONFIG_PPC_EARLY_DEBUG_BOOTX=y

this does not crash my VM. The changed chunk is sitting under "if
(prom_getprop(node, "linux,boot-display", NULL, 0)" and I cannot find
what creates this property - it is neither slof/grub/qemu, unlikely that
it is phyp so it must be this one:

arch/powerpc/platforms/powermac/bootx_init.c|244|
bootx_dt_add_string("linux,boot-display", mem_end);


which is powermac and not pseries. Or may be that pmac firmware.

Where did you see this crash?


> But if you do that the kernel crashes in draw_byte(), with a DAR
> pointing somewhere near INT_MAX.
> 
> Adding some debug to prom_init we see that we're not able to read the
> "address" property from OF, so we're just using whatever junk value
> was on the stack.
> 
> So check the properties can be read properly from OF, if not we bail
> out before initialising btext, which avoids the crash.

This is a right thing any way, just the commit log is confusing.

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/prom_init.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index ae7ec9903191..5090a5ab54e5 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -2422,10 +2422,19 @@ static void __init prom_check_displays(void)
>  			u32 width, height, pitch, addr;
>  
>  			prom_printf("Setting btext !\n");
> -			prom_getprop(node, "width", &width, 4);
> -			prom_getprop(node, "height", &height, 4);
> -			prom_getprop(node, "linebytes", &pitch, 4);
> -			prom_getprop(node, "address", &addr, 4);
> +
> +			if (prom_getprop(node, "width", &width, 4) == PROM_ERROR)
> +				return;
> +
> +			if (prom_getprop(node, "height", &height, 4) == PROM_ERROR)
> +				return;
> +
> +			if (prom_getprop(node, "linebytes", &pitch, 4) == PROM_ERROR)
> +				return;
> +
> +			if (prom_getprop(node, "address", &addr, 4) == PROM_ERROR)
> +				return;
> +
>  			prom_printf("W=%d H=%d LB=%d addr=0x%x\n",
>  				    width, height, pitch, addr);
>  			btext_setup_display(width, height, 8, pitch, addr);
>

Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 21/08/2020 20:34, Michael Ellerman wrote:
>> It's possible to enable CONFIG_PPC_EARLY_DEBUG_BOOTX for a pseries
>> kernel (maybe it shouldn't be), which is then booted with qemu/slof.
>
>
> CONFIG_BOOTX_TEXT=y
> CONFIG_PPC_EARLY_DEBUG=y
> CONFIG_PPC_EARLY_DEBUG_BOOTX=y
>
> this does not crash my VM. The changed chunk is sitting under "if
> (prom_getprop(node, "linux,boot-display", NULL, 0)" and I cannot find
> what creates this property - it is neither slof/grub/qemu, unlikely that
> it is phyp so it must be this one:
>
> arch/powerpc/platforms/powermac/bootx_init.c|244|
> bootx_dt_add_string("linux,boot-display", mem_end);

It's in prom_init.c:

static void __init prom_init_stdout(void)
{
...
	stdout_node = call_prom("instance-to-package", 1, 1, prom.stdout);
	if (stdout_node != PROM_ERROR) {
		val = cpu_to_be32(stdout_node);

		/* If it's a display, note it */
		memset(type, 0, sizeof(type));
		prom_getprop(stdout_node, "device_type", type, sizeof(type));
		if (prom_strcmp(type, "display") == 0)
			prom_setprop(stdout_node, path, "linux,boot-display", NULL, 0);
	}
}

> which is powermac and not pseries. Or may be that pmac firmware.
>
> Where did you see this crash?

Qemu pseries either TCG or KVM with eg:

$ qemu-system-ppc64 -M pseries -cpu POWER8 -m 1G -kernel build~/vmlinux


>> But if you do that the kernel crashes in draw_byte(), with a DAR
>> pointing somewhere near INT_MAX.
>> 
>> Adding some debug to prom_init we see that we're not able to read the
>> "address" property from OF, so we're just using whatever junk value
>> was on the stack.
>> 
>> So check the properties can be read properly from OF, if not we bail
>> out before initialising btext, which avoids the crash.
>
> This is a right thing any way, just the commit log is confusing.
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks.

cheers

On Fri, 21 Aug 2020 20:34:07 +1000, Michael Ellerman wrote:
> It's possible to enable CONFIG_PPC_EARLY_DEBUG_BOOTX for a pseries
> kernel (maybe it shouldn't be), which is then booted with qemu/slof.
> 
> But if you do that the kernel crashes in draw_byte(), with a DAR
> pointing somewhere near INT_MAX.
> 
> Adding some debug to prom_init we see that we're not able to read the
> "address" property from OF, so we're just using whatever junk value
> was on the stack.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/prom_init: Check display props exist before enabling btext
      https://git.kernel.org/powerpc/c/6c71cfcc01685ef495ca7886471a76e73446424e

cheers