[PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Ma Ke]

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@vger.kernel.org
Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
Signed-off-by: Ma Ke <make24@iscas.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) */
-- 
2.25.1

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Greg KH]

On Mon, Jul 15, 2024 at 10:54:42AM +0800, Ma Ke 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.

You need to describe the tool that found this.  And how did you test
that your fix is correct?

thanks,

greg k-h

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Michael Ellerman]

Ma Ke <make24@iscas.ac.cn> writes:
> 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@vger.kernel.org
> Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> Signed-off-by: Ma Ke <make24@iscas.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;
 }
 

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Dan Carpenter]

On Mon, Jul 15, 2024 at 04:28:15PM +1000, Michael Ellerman wrote:
> Ma Ke <make24@iscas.ac.cn> writes:
> > 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@vger.kernel.org
> > Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>

The bug is real and the fix looks okay to me. I'm surprised that Smatch doesn't
print a warning about "size" being uninitialized.  I must not have it enabled
in the .configs that I test.  But I also wouldn't have reported that because
it's from 2016 so it's too old.

> > ---
> > 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.
> 

You're a domain expert so I trust you, but as a static checker person, there is
no way I'd feel comfortable sending a patch like that...  It's way too
complicated and I wouldn't be able to test it.  If this were my patch I would
ask you to handle send that patch and give me Reported-by credit.

regards,
dan carpenter

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Ma Ke]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2895 bytes --]

> Michael Ellerman<mpe@ellerman.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@vger.kernel.org
> > Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> > Signed-off-by: Ma Ke <make24@iscas.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

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Michael Ellerman]

Ma Ke <make24@iscas.ac.cn> writes:
>> Michael Ellerman<mpe@ellerman.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@vger.kernel.org
>> > Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
>> > Signed-off-by: Ma Ke <make24@iscas.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.

Your patch is OK, I'll send an ack.

If we want to refactor it to use of_property_read_reg() we can do that
in future - though this code will probably be removed in the not too
distant future anyway.

cheers

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Markus Elfring]

> 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@vger.kernel.org
> Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>

How will interests evolve for caring more according to known research
and development processes?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n398
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v6.10#n5


> ---
> Changes in v4:
> - modified vulnerability description according to suggestions, making the
> process of static analysis of vulnerabilities clearer. No active research
> on developer behavior.
…

Does such information indicate any communication difficulties?

Regards,
Markus

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Greg Kroah-Hartman]

On Mon, Jul 15, 2024 at 03:18:56PM +0200, Markus Elfring 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@vger.kernel.org
> > Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> 
> How will interests evolve for caring more according to known research
> and development processes?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n398
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v6.10#n5
> 
> 
> > ---
> > Changes in v4:
> > - modified vulnerability description according to suggestions, making the
> > process of static analysis of vulnerabilities clearer. No active research
> > on developer behavior.
> …
> 
> Does such information indicate any communication difficulties?


Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot

Re: [PATCH v4] cxl: Fix possible null pointer dereference in read_handle()

[Michael Ellerman]

Ma Ke <make24@iscas.ac.cn> writes:
> 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@vger.kernel.org
> Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
 
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

In practice I don't this bug is triggerable, because the device tree
that's being parsed comes from a single source (IBM hypervisor), and if
this property was malformed that would simply be considered a bug in the
hypervisor.

cheers

> 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) */
> -- 
> 2.25.1