* Re: [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl
2024-11-18 16:26 [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl Suraj Sonawane
@ 2024-11-25 14:28 ` Suraj Sonawane
2024-12-02 16:26 ` Ira Weiny
2024-11-25 21:12 ` Alison Schofield
2024-12-02 15:26 ` Dave Jiang
2 siblings, 1 reply; 6+ messages in thread
From: Suraj Sonawane @ 2024-11-25 14:28 UTC (permalink / raw)
To: dan.j.williams
Cc: vishal.l.verma, dave.jiang, ira.weiny, rafael, lenb, nvdimm,
linux-acpi, linux-kernel, syzbot+7534f060ebda6b8b51b3
On 11/18/24 21:56, 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 less than sizeof(*call_pkg)
> before accessing it. This ensures safe access to the members of
> call_pkg, including the nd_reserved2 array.
>
> Reported-by: syzbot+7534f060ebda6b8b51b3@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7534f060ebda6b8b51b3
> Tested-by: syzbot+7534f060ebda6b8b51b3@syzkaller.appspotmail.com
> Fixes: ebe9f6f19d80 ("acpi/nfit: Fix bus command validation")
> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
> ---
> V1: https://lore.kernel.org/lkml/20241111080429.9861-1-surajsonawane0215@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.
> V4: Removed the explicit cast to maintain the original code style.
> V5: Re-Initialized `out_obj` to NULL. To prevent
> potential uninitialized variable usage if condition is true.
> V6: Remove the goto out condition from the error handling and directly
> returned -EINVAL in the check for buf and buf_len
>
> drivers/acpi/nfit/core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 5429ec9ef..a5d47819b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -454,8 +454,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> if (cmd_rc)
> *cmd_rc = -EINVAL;
>
> - if (cmd == ND_CMD_CALL)
> + if (cmd == ND_CMD_CALL) {
> + if (!buf || buf_len < sizeof(*call_pkg))
> + return -EINVAL;
> +
> call_pkg = buf;
> + }
> +
> func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
> if (func < 0)
> return func;
Hello!
I wanted to follow up on the patch I submitted. I have incorporated all
the suggested changes up to v6. I was wondering if you had a chance to
review it and if there are any comments or feedback.
Here are the links to the discussion for all versions:
https://lore.kernel.org/lkml/?q=acpi%3A+nfit%3A+vmalloc-out-of-bounds+Read+in+acpi_nfit_ctl
Thank you for your time and consideration. I look forward to your response.
Best regards,
Suraj Sonawane
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl
2024-11-25 14:28 ` Suraj Sonawane
@ 2024-12-02 16:26 ` Ira Weiny
2024-12-03 9:18 ` Suraj Sonawane
0 siblings, 1 reply; 6+ messages in thread
From: Ira Weiny @ 2024-12-02 16:26 UTC (permalink / raw)
To: Suraj Sonawane, dan.j.williams
Cc: vishal.l.verma, dave.jiang, ira.weiny, rafael, lenb, nvdimm,
linux-acpi, linux-kernel, syzbot+7534f060ebda6b8b51b3
Suraj Sonawane wrote:
> On 11/18/24 21:56, Suraj Sonawane wrote:
[snip]
> >
> > drivers/acpi/nfit/core.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 5429ec9ef..a5d47819b 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -454,8 +454,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> > if (cmd_rc)
> > *cmd_rc = -EINVAL;
> >
> > - if (cmd == ND_CMD_CALL)
> > + if (cmd == ND_CMD_CALL) {
> > + if (!buf || buf_len < sizeof(*call_pkg))
> > + return -EINVAL;
> > +
> > call_pkg = buf;
> > + }
> > +
> > func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
> > if (func < 0)
> > return func;
>
> Hello!
>
> I wanted to follow up on the patch I submitted. I have incorporated all
> the suggested changes up to v6. I was wondering if you had a chance to
> review it and if there are any comments or feedback.
It just missed the soak period for the merge. But I'll be looking at it
for an rc pull request.
Thanks for sticking with it,
Ira
[snip]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl
2024-12-02 16:26 ` Ira Weiny
@ 2024-12-03 9:18 ` Suraj Sonawane
0 siblings, 0 replies; 6+ messages in thread
From: Suraj Sonawane @ 2024-12-03 9:18 UTC (permalink / raw)
To: Ira Weiny, dan.j.williams
Cc: vishal.l.verma, dave.jiang, rafael, lenb, nvdimm, linux-acpi,
linux-kernel, syzbot+7534f060ebda6b8b51b3
On 12/2/24 21:56, Ira Weiny wrote:
> Suraj Sonawane wrote:
>> On 11/18/24 21:56, Suraj Sonawane wrote:
>
> [snip]
>
>>>
>>> drivers/acpi/nfit/core.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>> index 5429ec9ef..a5d47819b 100644
>>> --- a/drivers/acpi/nfit/core.c
>>> +++ b/drivers/acpi/nfit/core.c
>>> @@ -454,8 +454,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>> if (cmd_rc)
>>> *cmd_rc = -EINVAL;
>>>
>>> - if (cmd == ND_CMD_CALL)
>>> + if (cmd == ND_CMD_CALL) {
>>> + if (!buf || buf_len < sizeof(*call_pkg))
>>> + return -EINVAL;
>>> +
>>> call_pkg = buf;
>>> + }
>>> +
>>> func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
>>> if (func < 0)
>>> return func;
>>
>> Hello!
>>
>> I wanted to follow up on the patch I submitted. I have incorporated all
>> the suggested changes up to v6. I was wondering if you had a chance to
>> review it and if there are any comments or feedback.
>
> It just missed the soak period for the merge. But I'll be looking at it
> for an rc pull request.
>
> Thanks for sticking with it,
> Ira
>
> [snip]
Thank you for the update.
I also appreciate everyone's efforts in reviewing the patch.
Thank you for reviewing the patch.
Best regards,
Suraj
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl
2024-11-18 16:26 [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl Suraj Sonawane
2024-11-25 14:28 ` Suraj Sonawane
@ 2024-11-25 21:12 ` Alison Schofield
2024-12-02 15:26 ` Dave Jiang
2 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2024-11-25 21:12 UTC (permalink / raw)
To: Suraj Sonawane
Cc: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny, rafael,
lenb, nvdimm, linux-acpi, linux-kernel,
syzbot+7534f060ebda6b8b51b3
On Mon, Nov 18, 2024 at 09:56:09PM +0530, 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 less than sizeof(*call_pkg)
> before accessing it. This ensures safe access to the members of
> call_pkg, including the nd_reserved2 array.
>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Reported-by: syzbot+7534f060ebda6b8b51b3@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7534f060ebda6b8b51b3
> Tested-by: syzbot+7534f060ebda6b8b51b3@syzkaller.appspotmail.com
> Fixes: ebe9f6f19d80 ("acpi/nfit: Fix bus command validation")
> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
> ---
> V1: https://lore.kernel.org/lkml/20241111080429.9861-1-surajsonawane0215@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.
> V4: Removed the explicit cast to maintain the original code style.
> V5: Re-Initialized `out_obj` to NULL. To prevent
> potential uninitialized variable usage if condition is true.
> V6: Remove the goto out condition from the error handling and directly
> returned -EINVAL in the check for buf and buf_len
>
> drivers/acpi/nfit/core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 5429ec9ef..a5d47819b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -454,8 +454,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> if (cmd_rc)
> *cmd_rc = -EINVAL;
>
> - if (cmd == ND_CMD_CALL)
> + if (cmd == ND_CMD_CALL) {
> + if (!buf || buf_len < sizeof(*call_pkg))
> + return -EINVAL;
> +
> call_pkg = buf;
> + }
> +
> func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
> if (func < 0)
> return func;
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl
2024-11-18 16:26 [PATCH v6] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl Suraj Sonawane
2024-11-25 14:28 ` Suraj Sonawane
2024-11-25 21:12 ` Alison Schofield
@ 2024-12-02 15:26 ` Dave Jiang
2 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2024-12-02 15:26 UTC (permalink / raw)
To: Suraj Sonawane, dan.j.williams
Cc: vishal.l.verma, ira.weiny, rafael, lenb, nvdimm, linux-acpi,
linux-kernel, syzbot+7534f060ebda6b8b51b3
On 11/18/24 9:26 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 less than sizeof(*call_pkg)
> before accessing it. This ensures safe access to the members of
> call_pkg, including the nd_reserved2 array.
>
> Reported-by: syzbot+7534f060ebda6b8b51b3@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7534f060ebda6b8b51b3
> Tested-by: syzbot+7534f060ebda6b8b51b3@syzkaller.appspotmail.com
> Fixes: ebe9f6f19d80 ("acpi/nfit: Fix bus command validation")
> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> V1: https://lore.kernel.org/lkml/20241111080429.9861-1-surajsonawane0215@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.
> V4: Removed the explicit cast to maintain the original code style.
> V5: Re-Initialized `out_obj` to NULL. To prevent
> potential uninitialized variable usage if condition is true.
> V6: Remove the goto out condition from the error handling and directly
> returned -EINVAL in the check for buf and buf_len
>
> drivers/acpi/nfit/core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 5429ec9ef..a5d47819b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -454,8 +454,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> if (cmd_rc)
> *cmd_rc = -EINVAL;
>
> - if (cmd == ND_CMD_CALL)
> + if (cmd == ND_CMD_CALL) {
> + if (!buf || buf_len < sizeof(*call_pkg))
> + return -EINVAL;
> +
> call_pkg = buf;
> + }
> +
> func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
> if (func < 0)
> return func;
^ permalink raw reply [flat|nested] 6+ messages in thread