| Message ID | Y8ldQn1v4r5i5WLX@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ACPI: NFIT: prevent underflow in acpi_nfit_ctl() | expand |
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;
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
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
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
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;
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(-)