linux-hardening - Re: [PATCH] pstore/ram: Rework logic for detecting ramoops

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] [day] [month] [year] [list]

Date:   Fri, 13 Jan 2023 17:32:46 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Kees Cook <keescook@...omium.org>
CC:     <tony.luck@...el.com>, <gpiccoli@...lia.com>,
        <linux-hardening@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pstore/ram: Rework logic for detecting ramoops

Hi Kees,

Thanks for your comments.

On 1/13/2023 3:09 AM, Kees Cook wrote:
> 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@...cinc.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;
> 

Fixed it in v2.

> 
>> +	} 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?
>

Have updated the docs in v2;

-Mukesh