linux-kernel - Re: [Resend] arch: mips: Fix initrd_start and initrd

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: <20190419205456.uylahdin2jlkeyyy@pburton-laptop>
Date:   Fri, 19 Apr 2019 20:55:06 +0000
From:   Paul Burton <paul.burton@...s.com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
CC:     "ralf@...ux-mips.org" <ralf@...ux-mips.org>,
        "jhogan@...nel.org" <jhogan@...nel.org>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [Resend] arch: mips: Fix initrd_start and initrd_end when read
 from DT

Hi Horatiu,

On Tue, Apr 16, 2019 at 12:18:20PM +0200, Horatiu Vultur wrote:
> When the bootloader passes arguments to linux kernel through device tree,
> it passes the address of initrd_start and initrd_stop, which are in kseg0.
> But when linux kernel reads these addresses from device tree, it converts
> them to virtual addresses inside the function
> __early_init_dt_declare_initrd.

I'm not sure I follow - if the bootloader provides an address in kseg0
then it's already a virtual address.

It looks like __early_init_dt_declare_initrd expects the DT to provide
physical addresses, which fits in well with the fact that DTs generally
use physical addresses for everything else.

__early_init_dt_declare_initrd calling __va on a virtual address will
give you something bogus, and it looks like you're just cancelling this
out below. In practice for a typical system where PAGE_OFFSET is the
start of kseg0 (0x80000000) the bogus address you get will happen to be
the same as the physical address, but that's not guaranteed.

> At a later point then in the function init_initrd, it is checking for
> initrd_start to be lower than PAGE_OFFSET, which for a 32 CPU it is not,
> therefore it would disable the initrd by setting 0 to initrd_start and
> initrd_stop.

The check you mention here is to make sure initrd_start looks like a
virtual address - if it's lower than PAGE_OFFSET (typically 0x80000000)
then it looks bad & initrd is disabled. I think your comment is
backwards - what you have is a physical address, entirely by accident,
and you're converting it back to a virtual address again by accident
which keeps the check happy.

> The fix consists of checking if linux kernel received a device tree and not
> having enable extended virtual address and in that case convert them back
> to physical addresses that point in kseg0 as expected.

Can you instead just have your bootloader provide physical addresses in
the DT?

Even if we were to have this code try to sanitize the value with
something like __va(__pa(initrd_start)), it only covers systems using
the UHI boot protocol which isn't the only way we can obtain a DT. If a
system builds in its DTB for example it'll get different behaviour to if
it's passed via the UHI protocol by the bootloader.

Thanks,
    Paul

> Signed-off-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
> ---
>  arch/mips/kernel/setup.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 8d1dc6c..774ee00 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -264,6 +264,17 @@ static unsigned long __init init_initrd(void)
>  		pr_err("initrd start must be page aligned\n");
>  		goto disable;
>  	}
> +
> +	/*
> +	 * In case the initrd_start and initrd_end are read from DT,
> +	 * then they are converted to virtual address, therefore convert
> +	 * them back to physical address.
> +	 */
> +	if (!IS_ENABLED(CONFIG_EVA) && fw_arg0 == -2) {
> +		initrd_start = initrd_start - PAGE_OFFSET + PHYS_OFFSET;
> +		initrd_end = initrd_end - PAGE_OFFSET + PHYS_OFFSET;
> +	}
> +
>  	if (initrd_start < PAGE_OFFSET) {
>  		pr_err("initrd start < PAGE_OFFSET\n");
>  		goto disable;
> -- 
> 2.7.4
>