| Message ID | 20240715025442.3229209-1-make24@iscas.ac.cn (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v4] cxl: Fix possible null pointer dereference in read_handle() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
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
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; }
> 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
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
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
> 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
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
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
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) */
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(-)