On 11/14/24 2:19 AM, Suraj Sonawane wrote:
> On 13/11/24 22:32, Dave Jiang wrote:
>>
>>
>> On 11/13/24 5:51 AM, Suraj Sonawane wrote:
>>> Fix an issue detected by syzbot with KASAN:
>>>
>>> BUG: KASAN: vmalloc-out-of-bounds in cmd_to_func drivers/acpi/nfit/
>>> core.c:416 [inline]
>>> BUG: KASAN: vmalloc-out-of-bounds in acpi_nfit_ctl+0x20e8/0x24a0
>>> drivers/acpi/nfit/core.c:459
>>>
>>> The issue occurs in cmd_to_func when the call_pkg->nd_reserved2
>>> array is accessed without verifying that call_pkg points to a buffer
>>> that is appropriately sized as a struct nd_cmd_pkg. This can lead
>>> to out-of-bounds access and undefined behavior if the buffer does not
>>> have sufficient space.
>>>
>>> To address this, a check was added in acpi_nfit_ctl() to ensure that
>>> buf is not NULL and that buf_len is greater than sizeof(*call_pkg)
>>> before casting buf to struct nd_cmd_pkg *. This ensures safe access
>>> to the members of call_pkg, including the nd_reserved2 array.
>>>
>>> Reported-by: syzbot+7534f060ebda6b8b5...@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=7534f060ebda6b8b51b3
>>> Tested-by: syzbot+7534f060ebda6b8b5...@syzkaller.appspotmail.com
>>> Fixes: ebe9f6f19d80 ("acpi/nfit: Fix bus command validation")
>>> Signed-off-by: Suraj Sonawane <surajsonawane0...@gmail.com>
>>> ---
>>> V1:
>>> https://lore.kernel.org/lkml/20241111080429.9861-1-surajsonawane0...@gmail.com/
>>> V2: Initialized `out_obj` to `NULL` in `acpi_nfit_ctl()` to prevent
>>> potential uninitialized variable usage if condition is true.
>>> V3: Changed the condition to if (!buf || buf_len < sizeof(*call_pkg))
>>> and updated the Fixes tag to reference the correct commit.
>>>
>>> drivers/acpi/nfit/core.c | 12 +++++++++---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>> index 5429ec9ef..eb5349606 100644
>>> --- a/drivers/acpi/nfit/core.c
>>> +++ b/drivers/acpi/nfit/core.c
>>> @@ -439,7 +439,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
>>> *nd_desc, struct nvdimm *nvdimm,
>>> {
>>> struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
>>> struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
>>> - union acpi_object in_obj, in_buf, *out_obj;
>>> + union acpi_object in_obj, in_buf, *out_obj = NULL;
>>
>> Looking at the code later, out_obj is always assigned before access. I'm not
>> seeing a path where out_obj would be accessed unitialized...
>
> I initialized out_obj to NULL to prevent potential issues where goto out
> might access an uninitialized pointer, ensuring ACPI_FREE(out_obj) handles
> NULL safely in the cleanup section. This covers cases where the condition
> !buf || buf_len < sizeof(*call_pkg) triggers an early exit, preventing
> unintended behavior.
ok
>
>>
>> https://elixir.bootlin.com/linux/v6.12-rc7/source/drivers/acpi/nfit/core.c#L538
>>
>>> const struct nd_cmd_desc *desc = NULL;
>>> struct device *dev = acpi_desc->dev;
>>> struct nd_cmd_pkg *call_pkg = NULL;
>>> @@ -454,8 +454,14 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
>>> *nd_desc, struct nvdimm *nvdimm,
>>> if (cmd_rc)
>>> *cmd_rc = -EINVAL;
>>> - if (cmd == ND_CMD_CALL)
>>> - call_pkg = buf;
>>> + if (cmd == ND_CMD_CALL) {
>>> + if (!buf || buf_len < sizeof(*call_pkg)) {
>>> + rc = -EINVAL;
>>> + goto out;
>>> + }
>>> + call_pkg = (struct nd_cmd_pkg *)buf;
>>
>> Is the casting needed? It wasn't in the old code
>>
>
> I tested the code both with and without the cast using syzbot, and it didn't
> result in any errors in either case. Since the buffer (buf) is being used as
> a pointer to struct nd_cmd_pkg, and the casting works in both scenarios, it
> appears that the cast may not be strictly necessary for this particular case.
>
> I can remove the cast and retain the original code structure, as it does not
> seem to affect functionality. However, the cast was added for clarity and
> type safety to ensure that buf is explicitly treated as a struct nd_cmd_pkg *.
>
> Would you prefer to remove the cast, or should I keep it as is for type
> safety and clarity?
I would just leave it as it was.
>
>>> + }
>>> +
>>> func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
>>> if (func < 0)
>>> return func;
>>
>
> Thank you for your feedback and your time.
