________________________________________
От: Dave Jiang <[email protected]>
Отправлено: 24 января 2025 г. 2:43
Кому: Masimov Murad; Dan Williams
Копия: Vishal Verma; Ira Weiny; Rafael J. Wysocki; Len Brown;
[email protected]; [email protected];
[email protected]; [email protected];
[email protected]; [email protected]
Тема: Re: [PATCH] acpi: nfit: fix narrowing conversion in acpi_nfit_ctl
> On 1/23/25 9:39 AM, Murad Masimov wrote:
> > Syzkaller has reported a warning in to_nfit_bus_uuid(): "only secondary
> > bus families can be translated". This warning is emited if the argument
> > is equal to NVDIMM_BUS_FAMILY_NFIT == 0. Function acpi_nfit_ctl() first
> > verifies that a user-provided value call_pkg->nd_family of type u64 is
> > not equal to 0. Then the value is converted to int, and only after that
> > is compared to NVDIMM_BUS_FAMILY_MAX. This can lead to passing an invalid
> > argument to acpi_nfit_ctl(), if call_pkg->nd_family is non-zero, while
> > the lower 32 bits are zero.
> >
> > All checks of the input value should be applied to the original variable
> > call_pkg->nd_family.
> >
> > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> >
> > Fixes: 6450ddbd5d8e ("ACPI: NFIT: Define runtime firmware activation
> > commands")
> > Cc: [email protected]
> > Reported-by: [email protected]
> > Closes: https://syzkaller.appspot.com/bug?extid=c80d8dc0d9fa81a3cd8c
> > Signed-off-by: Murad Masimov <[email protected]>
>
> While the change logically makes sense, the likelihood of nd_family >
> int_size is not ever likely. Given that NVDIMM_BUS_FAMILY_MAX is defined as
> 1, I don't think we care about values greater than that regardless of what is
> set in the upper 32bit of the u64. I'm leaning towards the fix is unnecessary.
Thank you for the review! But I believe there is a misunderstanding. The point
is that the code fragment affected by this patch is intended to make sure, that
family is in range between 1 and NVDIMM_BUS_FAMILY_MAX. This is necessary
because call_pkg contains user-provided data. However the implementation of
these validity checks is erroneous and leads to passing an invalid value. The
syzkaller report proves, that this bug can be triggered by a user. Here is an
example to demonstrate, what exactly happens:
1. Let's say call_pkg->nd_family is equal to (1ull << 32).
2. Expression (cmd == ND_CMD_CALL && call_pkg->nd_family) evaluates to true.
3. Since family is of type int, and call_pkg->nd_family is u64, assigning
call_pkg->nd_family to family will lead to a narrowing conversion.
4. As a result, family equals to 0, which will be passed in to_nfit_bus_uuid()
triggering the warning.
Moreover, family may also be a negative integer (e.g. call_pkg->nd_family ==
~(0ull)). This can lead to an undefined behaviour in test_bit() and potentially
out-of-bounds in to_nfit_uuid(). Thus, even if triggering a WARN is not
concerning, the bug still should be fixed.
> > ---
> > 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 a5d47819b3a4..ae035b93da08 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -485,7 +485,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
> > *nd_desc, struct nvdimm *nvdimm,
> > cmd_mask = nd_desc->cmd_mask;
> > if (cmd == ND_CMD_CALL && call_pkg->nd_family) {
> > family = call_pkg->nd_family;
> > - if (family > NVDIMM_BUS_FAMILY_MAX ||
> > + if (call_pkg->nd_family > NVDIMM_BUS_FAMILY_MAX ||
> > !test_bit(family, &nd_desc->bus_family_mask))
> > return -EINVAL;
> > family = array_index_nospec(family,
> > --
> > 2.39.2
> >
