linux-kernel - Re: [PATCH v4] cxl: Fix possible null pointer dereference in read

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: <20240716132737.1642226-1-make24@iscas.ac.cn>
Date: Tue, 16 Jul 2024 21:27:37 +0800
From: Ma Ke <make24@...as.ac.cn>
To: mpe@...erman.id.au
Cc: ajd@...ux.ibm.com,
	arnd@...db.de,
	clombard@...ux.vnet.ibm.com,
	fbarrat@...ux.ibm.com,
	gregkh@...uxfoundation.org,
	imunsie@....ibm.com,
	linux-kernel@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org,
	make24@...as.ac.cn,
	dan.carpenter@...aro.org,
	manoj@...ux.vnet.ibm.com,
	stable@...r.kernel.org
Subject: Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

> Michael Ellerman<mpe@...erman.id.au> wrote:
> > In read_handle(), of_get_address() may return NULL if getting address and
> > size of the node failed. When of_read_number() uses prop to handle
> > conversions between different byte orders, it could lead to a null pointer
> > dereference. Add NULL check to fix potential issue.
> >
> > Found by static analysis.
> >
> > Cc: stable@...r.kernel.org
> > Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> > Signed-off-by: Ma Ke <make24@...as.ac.cn>
> > ---
> > Changes in v4:
> > - modified vulnerability description according to suggestions, making the 
> > process of static analysis of vulnerabilities clearer. No active research 
> > on developer behavior.
> > Changes in v3:
> > - fixed up the changelog text as suggestions.
> > Changes in v2:
> > - added an explanation of how the potential vulnerability was discovered,
> > but not meet the description specification requirements.
> > ---
> >  drivers/misc/cxl/of.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> > index bcc005dff1c0..d8dbb3723951 100644
> > --- a/drivers/misc/cxl/of.c
> > +++ b/drivers/misc/cxl/of.c
> > @@ -58,7 +58,7 @@ static int read_handle(struct device_node *np, u64 *handle)
> >  
> >  	/* Get address and size of the node */
> >  	prop = of_get_address(np, 0, &size, NULL);
> > -	if (size)
> > +	if (!prop || size)
> >  		return -EINVAL;
> >  
> >  	/* Helper to read a big number; size is in cells (not bytes) */
> 
> If you expand the context this could just use of_property_read_reg(),
> something like below.
> 
> cheers
> 
> 
> diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> index bcc005dff1c0..a184855b2a7b 100644
> --- a/drivers/misc/cxl/of.c
> +++ b/drivers/misc/cxl/of.c
> @@ -53,16 +53,15 @@ static const __be64 *read_prop64_dword(const struct device_node *np,
>  
>  static int read_handle(struct device_node *np, u64 *handle)
>  {
> -	const __be32 *prop;
>  	u64 size;
> +	
> +	if (of_property_read_reg(np, 0, handle, &size))
> +		return -EINVAL;
>  
> -	/* Get address and size of the node */
> -	prop = of_get_address(np, 0, &size, NULL);
> +	// Size must be zero per PAPR+ v2.13 § C.6.19
>  	if (size)
>  		return -EINVAL;
>  
> -	/* Helper to read a big number; size is in cells (not bytes) */
> -	*handle = of_read_number(prop, of_n_addr_cells(np));
>  	return 0;
>  }
Thank you for discussing and guiding me on the vulnerability I submitted. 
I've carefully read through your conversation with Dan Carpenter. I'm 
uncertain whether to use my patch or the one you provided. Could you please
advise on which patch would be more appropriate? 
Looking forward to your reply.
--
Regards,

Ma Ke