linux-kernel - Re: [PATCH] of/fdt: Ignore disabled memory nodes

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: <20220517153444.GA1057027-robh@kernel.org>
Date:   Tue, 17 May 2022 10:34:44 -0500
From:   Rob Herring <robh@...nel.org>
To:     Andre Przywara <andre.przywara@....com>
Cc:     Frank Rowand <frowand.list@...il.com>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Ross Burton <ross.burton@....com>,
        Peter Maydell <peter.maydell@...aro.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH] of/fdt: Ignore disabled memory nodes

On Tue, May 17, 2022 at 11:14:10AM +0100, Andre Przywara wrote:
> When we boot a machine using a devicetree, the generic DT code goes
> through all nodes with a 'device_type = "memory"' property, and collects
> all memory banks mentioned there. However it does not check for the
> status property, so any nodes which are explicitly "disabled" will still
> be added as a memblock.
> This ends up badly for QEMU, when booting with secure firmware on
> arm/arm64 machines, because QEMU adds a node describing secure-only
> memory:
> ===================
> 	secram@...0000 {

BTW, 'memory' is the correct node name.

> 		secure-status = "okay";
> 		status = "disabled";
> 		reg = <0x00 0xe000000 0x00 0x1000000>;
> 		device_type = "memory";
> 	};
> ===================
> 
> The kernel will eventually use that memory block (which is located below
> the main DRAM bank), but accesses to that will be answered with an
> SError:
> ===================
> [    0.000000] Internal error: synchronous external abort: 96000050 [#1] PREEMPT SMP
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc6-00014-g10c8acb8b679 #524
> [    0.000000] Hardware name: linux,dummy-virt (DT)
> [    0.000000] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    0.000000] pc : new_slab+0x190/0x340
> [    0.000000] lr : new_slab+0x184/0x340
> [    0.000000] sp : ffff80000a4b3d10
> ....
> ==================
> The actual crash location and call stack will be somewhat random, and
> depend on the specific allocation of that physical memory range.
> 
> As the DT spec[1] explicitly mentions standard properties, add a simple
> check to skip over disabled memory nodes, so that we only use memory
> that is meant for non-secure code to use.
> 
> That fixes booting a QEMU arm64 VM with EL3 enabled ("secure=on"), when
> not using UEFI. In this case the QEMU generated DT will be handed on
> to the kernel, which will see the secram node.
> This issue is reproducible when using TF-A together with U-Boot as
> firmware, then booting with the "booti" command.
> 
> When using U-Boot as an UEFI provider, the code there [2] explicitly
> filters for disabled nodes when generating the UEFI memory map, so we
> are safe.
> EDK/2 only reads the first bank of the first DT memory node [3] to learn
> about memory, so we got lucky there.
> 
> [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst#memory-node (after the table)
> [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/fdtdec.c#L1061-1063
> [3] https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/FdtParser.c
> 
> Reported-by: Ross Burton <ross.burton@....com>
> Signed-off-by: Andre Przywara <andre.przywara@....com>
> ---
>  drivers/of/fdt.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks!