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

Dan Carpenter <error27@...> 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@...>
---
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@...> 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@...> 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@...> 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