Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 12 Jan 2023 13:39:55 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     tony.luck@intel.com, gpiccoli@igalia.com,
        linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pstore/ram: Rework logic for detecting ramoops
Message-ID: <202301121337.B8CCCB6@keescook>
References: <1673428065-22356-1-git-send-email-quic_mojha@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1673428065-22356-1-git-send-email-quic_mojha@quicinc.com>
Precedence: bulk

On Wed, Jan 11, 2023 at 02:37:45PM +0530, Mukesh Ojha wrote:
> 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 the region to be dynamically
> allocated at runtime, as opposed to being fixed at compile time.
> 
> Also, Some of the platforms might be still expecting dedicated
> memory region for ramoops node where the region is known
> beforehand and platform_get_resource() is used in that case.
> 
> So, Add logic to detect the start and size of the ramoops memory
> region by looking up reserved memory region with
> of_reserved_mem_lookup() when platform_get_resource() failed.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

Thanks for the patch! Notes below...

> ---
>  fs/pstore/ram.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ade66db..e4bbba1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -20,6 +20,7 @@
>  #include <linux/compiler.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
>  
>  #include "internal.h"
>  #include "ram_internal.h"
> @@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  {
>  	struct device_node *of_node = pdev->dev.of_node;
>  	struct device_node *parent_node;
> +	struct reserved_mem *rmem;
>  	struct resource *res;
>  	u32 value;
>  	int ret;
> @@ -651,13 +653,20 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
> -		dev_err(&pdev->dev,
> -			"failed to locate DT /reserved-memory resource\n");
> -		return -EINVAL;
> +		rmem = of_reserved_mem_lookup(of_node);
> +		if (rmem) {
> +			pdata->mem_size = rmem->size;
> +			pdata->mem_address = rmem->base;
> +		} else {
> +			dev_err(&pdev->dev,
> +				"failed to locate DT /reserved-memory resource\n");
> +			return -EINVAL;
> +		}

Since the "else" case returns, the traditional code pattern is to leave
the other case "inline" (an indented), like so:

		if (!rmem) {
			dev_err(&pdev->dev,
				"failed to locate DT /reserved-memory resource\n");
			return -EINVAL;
		}
		pdata->mem_size = rmem->size;
		pdata->mem_address = rmem->base;


> +	} else {
> +		pdata->mem_size = resource_size(res);
> +		pdata->mem_address = res->start;
>  	}

Since this change the potential interface with DT, can you also update
the documentation in:

Documentation/devicetree/bindings/reserved-memory/ramoops.yaml

Or maybe my understanding of DT parsing is lacking and this change is
doing something slightly different?

-Kees

>  
> -	pdata->mem_size = resource_size(res);
> -	pdata->mem_address = res->start;
>  	/*
>  	 * Setting "unbuffered" is deprecated and will be ignored if
>  	 * "mem_type" is also specified.
> -- 
> 2.7.4
> 

-- 
Kees Cook