linux-kernel - Re: [PATCH] pstore/zone: Fix return value in psz_init_zone() and psz_init

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]

Message-ID: <202504151404.B106097461@keescook>
Date: Tue, 15 Apr 2025 14:05:45 -0700
From: Kees Cook <kees@...nel.org>
To: hhtracer@...il.com
Cc: tony.luck@...el.com, gpiccoli@...lia.com,
	liaoweixiong@...winnertech.com, linux-hardening@...r.kernel.org,
	linux-kernel@...r.kernel.org, huhai <huhai@...inos.cn>
Subject: Re: [PATCH] pstore/zone: Fix return value in psz_init_zone() and
 psz_init_zones()

On Sun, Apr 13, 2025 at 03:27:45PM +0800, hhtracer@...il.com wrote:
> From: huhai <huhai@...inos.cn>
> 
> The psz_init_zone() and psz_init_zones() functions return NULL on error,
> but psz_alloc_zones() only checks the return value using IS_ERR(). Since
> NULL is not an error pointer, this causes psz_alloc_zones() to mistakenly
> treat a failure as success and return 0, which may lead to a NULL pointer
> dereference.

Have you encountered this failure?

> 
> Update both psz_init_zone() and psz_init_zones() to return proper error
> pointers using ERR_PTR() instead of NULL.
> 
> Fixes: d26c3321fe18 ("pstore/zone: Introduce common layer to manage storage zones")
> Signed-off-by: huhai <huhai@...inos.cn>
> ---
>  fs/pstore/zone.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
> index ceb5639a0629..57ffcf76f254 100644
> --- a/fs/pstore/zone.c
> +++ b/fs/pstore/zone.c
> @@ -1157,7 +1157,7 @@ static struct pstore_zone *psz_init_zone(enum pstore_type_id type,
>  	const char *name = pstore_type_to_name(type);
>  
>  	if (!size)
> -		return NULL;

Because as far as I know, this is by design: a size 0 prz means there's
no zone. Nothing should ever try to use it because the size is 0.

> +		return ERR_PTR(-EINVAL);
>  
>  	if (*off + size > info->total_size) {
>  		pr_err("no room for %s (0x%zx@...llx over 0x%lx)\n",
> @@ -1203,7 +1203,7 @@ static struct pstore_zone **psz_init_zones(enum pstore_type_id type,
>  
>  	*cnt = 0;
>  	if (!total_size || !record_size)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (*off + total_size > info->total_size) {
>  		pr_err("no room for zones %s (0x%zx@...llx over 0x%lx)\n",
> @@ -1225,7 +1225,7 @@ static struct pstore_zone **psz_init_zones(enum pstore_type_id type,
>  
>  	for (i = 0; i < c; i++) {
>  		zone = psz_init_zone(type, off, record_size);
> -		if (!zone || IS_ERR(zone)) {
> +		if (IS_ERR(zone)) {
>  			pr_err("initialize zones %s failed\n", name);
>  			psz_free_zones(&zones, &i);
>  			return (void *)zone;

I think the code is correct as-is. Have you found a place where the prz
is used even when NULL?

-- 
Kees Cook