* [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl()
@ 2023-01-19 15:09 Dan Carpenter
2023-01-19 16:21 ` Jeff Moyer
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-01-19 15:09 UTC (permalink / raw)
To: Dan Williams
Cc: Vishal Verma, Dave Jiang, Ira Weiny, Rafael J. Wysocki,
Len Brown, nvdimm, linux-acpi, kernel-janitors, cip-dev,
Harshit Mogalapalli
The concern here would be that "family" is negative and we pass a
negative value to test_bit() resulting in an out of bounds read
and potentially a crash.
This patch is based on static analysis and not on testing.
Fixes: 9a7e3d7f0568 ("ACPI: NFIT: Fix input validation of bus-family")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2: add missing close parens ) in subject
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index f1cc5ec6a3b6..da0739f04c98 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -446,10 +446,10 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
const char *cmd_name, *dimm_name;
unsigned long cmd_mask, dsm_mask;
u32 offset, fw_status = 0;
+ unsigned int family = 0;
acpi_handle handle;
const guid_t *guid;
int func, rc, i;
- int family = 0;
if (cmd_rc)
*cmd_rc = -EINVAL;
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl()
2023-01-19 15:09 [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl() Dan Carpenter
@ 2023-01-19 16:21 ` Jeff Moyer
2023-01-20 5:22 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Moyer @ 2023-01-19 16:21 UTC (permalink / raw)
To: Dan Carpenter
Cc: Dan Williams, Vishal Verma, Dave Jiang, Ira Weiny,
Rafael J. Wysocki, Len Brown, nvdimm, linux-acpi,
kernel-janitors, cip-dev, Harshit Mogalapalli
Dan Carpenter <error27@gmail.com> writes:
> The concern here would be that "family" is negative and we pass a
> negative value to test_bit() resulting in an out of bounds read
> and potentially a crash.
I don't see how this can happen. Do you have a particular scenario in
mind?
-Jeff
> This patch is based on static analysis and not on testing.
>
> Fixes: 9a7e3d7f0568 ("ACPI: NFIT: Fix input validation of bus-family")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> v2: add missing close parens ) in subject
>
> drivers/acpi/nfit/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index f1cc5ec6a3b6..da0739f04c98 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -446,10 +446,10 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> const char *cmd_name, *dimm_name;
> unsigned long cmd_mask, dsm_mask;
> u32 offset, fw_status = 0;
> + unsigned int family = 0;
> acpi_handle handle;
> const guid_t *guid;
> int func, rc, i;
> - int family = 0;
>
> if (cmd_rc)
> *cmd_rc = -EINVAL;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl()
2023-01-19 16:21 ` Jeff Moyer
@ 2023-01-20 5:22 ` Dan Carpenter
2023-03-28 14:55 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-01-20 5:22 UTC (permalink / raw)
To: Jeff Moyer
Cc: Dan Williams, Vishal Verma, Dave Jiang, Ira Weiny,
Rafael J. Wysocki, Len Brown, nvdimm, linux-acpi,
kernel-janitors, cip-dev, Harshit Mogalapalli
On Thu, Jan 19, 2023 at 11:21:22AM -0500, Jeff Moyer wrote:
> Dan Carpenter <error27@gmail.com> writes:
>
> > The concern here would be that "family" is negative and we pass a
> > negative value to test_bit() resulting in an out of bounds read
> > and potentially a crash.
>
> I don't see how this can happen. Do you have a particular scenario in
> mind?
>
This is from static analysis. My main thinking was:
1) The static checker says that this comes from the user.
2) Every upper bounds check should have a lower bounds check.
3) family is passed to array_index_nospec() so we must not trust it.
But looking closer today here is what the checker is concerned about:
func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
Assume "nfit_mem" is NULL but "call_pkg" is non NULL (user input from
__nd_ioctl() or ars_get_status(). In that case family is unchecked user
input.
But probably, it's not possible for nfit_mem to be NULL in those caller
functions?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl()
2023-01-20 5:22 ` Dan Carpenter
@ 2023-03-28 14:55 ` Dan Carpenter
2023-03-28 14:57 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-03-28 14:55 UTC (permalink / raw)
To: Jeff Moyer
Cc: Dan Williams, Vishal Verma, Dave Jiang, Ira Weiny,
Rafael J. Wysocki, Len Brown, nvdimm, linux-acpi,
kernel-janitors, cip-dev, Harshit Mogalapalli
On Fri, Jan 20, 2023 at 08:22:06AM +0300, Dan Carpenter wrote:
> On Thu, Jan 19, 2023 at 11:21:22AM -0500, Jeff Moyer wrote:
> > Dan Carpenter <error27@gmail.com> writes:
> >
> > > The concern here would be that "family" is negative and we pass a
> > > negative value to test_bit() resulting in an out of bounds read
> > > and potentially a crash.
> >
> > I don't see how this can happen. Do you have a particular scenario in
> > mind?
> >
>
> This is from static analysis. My main thinking was:
>
> 1) The static checker says that this comes from the user.
> 2) Every upper bounds check should have a lower bounds check.
> 3) family is passed to array_index_nospec() so we must not trust it.
>
> But looking closer today here is what the checker is concerned about:
>
> func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
>
> Assume "nfit_mem" is NULL but "call_pkg" is non NULL (user input from
> __nd_ioctl() or ars_get_status(). In that case family is unchecked user
> input.
>
> But probably, it's not possible for nfit_mem to be NULL in those caller
> functions?
Did we ever figure out if it's possible for nfit_mem to be NULL?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl()
2023-03-28 14:55 ` Dan Carpenter
@ 2023-03-28 14:57 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2023-03-28 14:57 UTC (permalink / raw)
To: Jeff Moyer
Cc: Dan Williams, Vishal Verma, Dave Jiang, Ira Weiny,
Rafael J. Wysocki, Len Brown, nvdimm, linux-acpi,
kernel-janitors, cip-dev, Harshit Mogalapalli
On Tue, Mar 28, 2023 at 05:55:40PM +0300, Dan Carpenter wrote:
> On Fri, Jan 20, 2023 at 08:22:06AM +0300, Dan Carpenter wrote:
> > On Thu, Jan 19, 2023 at 11:21:22AM -0500, Jeff Moyer wrote:
> > > Dan Carpenter <error27@gmail.com> writes:
> > >
> > > > The concern here would be that "family" is negative and we pass a
> > > > negative value to test_bit() resulting in an out of bounds read
> > > > and potentially a crash.
> > >
> > > I don't see how this can happen. Do you have a particular scenario in
> > > mind?
> > >
> >
> > This is from static analysis. My main thinking was:
> >
> > 1) The static checker says that this comes from the user.
> > 2) Every upper bounds check should have a lower bounds check.
> > 3) family is passed to array_index_nospec() so we must not trust it.
> >
> > But looking closer today here is what the checker is concerned about:
> >
> > func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
> >
> > Assume "nfit_mem" is NULL but "call_pkg" is non NULL (user input from
> > __nd_ioctl() or ars_get_status(). In that case family is unchecked user
> > input.
> >
> > But probably, it's not possible for nfit_mem to be NULL in those caller
> > functions?
>
> Did we ever figure out if it's possible for nfit_mem to be NULL?
Another idea is I could send this patch as a static checker fix instead
of a security vulnerability. That way we would be safe going forward
regardless.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-28 14:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-19 15:09 [PATCH v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl() Dan Carpenter
2023-01-19 16:21 ` Jeff Moyer
2023-01-20 5:22 ` Dan Carpenter
2023-03-28 14:55 ` Dan Carpenter
2023-03-28 14:57 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.