linux-kernel - Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

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: <946f9db2-41a0-b58c-771a-5a82fdf28039@kernel.org>
Date:   Tue, 4 Jul 2023 08:05:21 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     "Isaac J. Manjarres" <isaacmanjarres@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Tony Luck <tony.luck@...el.com>,
        "Guilherme G. Piccoli" <gpiccoli@...lia.com>,
        Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>
Cc:     "Isaac J. Manjarres" <isaacm@...eaurora.org>,
        Mukesh Ojha <mojha@...eaurora.org>,
        Prasad Sodagudi <psodagud@...eaurora.org>,
        kernel-team@...roid.com, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops
 memory regions

On 22/06/2023 02:52, Isaac J. Manjarres wrote:
> From: "Isaac J. Manjarres" <isaacm@...eaurora.org>
> 
> The reserved memory region for ramoops is assumed to be at a fixed
> and known location when read from the devicetree. This is not desirable
> in environments where it is preferred for the region to be dynamically
> allocated early during boot (i.e. the memory region is defined with
> the "alloc-ranges" property instead of the "reg" property).
> 
> If the location of the ramoops region is not fixed via the "reg"
> devicetree property, the call to platform_get_resource() will fail
> because resources of type IORESOURCE_MEM must be described with the
> "reg" property.
> 
> Since ramoops regions are part of the reserved-memory devicetree
> node, they exist in the reserved_mem array. This means that the
> of_reserved_mem_lookup() function can be used to retrieve the
> reserved_mem structure for the ramoops region, and that structure
> contains the base and size of the region, even if it has been
> dynamically allocated.
> 
> Thus invoke of_reserved_mem_lookup() in case the call to
> platform_get_resource() fails in order to support dynamically
> allocated ramoops memory regions.

You missed change to Devicetree binding, so the reg is still required
for all DT-based platforms. For such, this patch is just half-baked.

You also did not CC DT maintainers, which would be nice considering some
improper advises for minidump like here:
https://lore.kernel.org/lkml/e25723bf-be85-b458-a84c-1a45392683bb@gmail.com/

DT is not for some dynamically allocated properties or dynamic system
configuration. You don't need DT for that. Therefore if you remove the
reg, I question the entire point of this binding.

See also:
https://lore.kernel.org/lkml/CAL_JsqJ_TTnGjjB2d8_FKHpWBRG5GHLoWnabCKjsdeZ4QFdNEg@mail.gmail.com/

https://lore.kernel.org/lkml/20230202235916.GA2931100-robh@kernel.org/


Best regards,
Krzysztof