[PATCH v3] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl

[PATCH v3] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl

[Suraj Sonawane]

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

 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;
 	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;
+	}
+
 	func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
 	if (func < 0)
 		return func;
-- 
2.34.1

Re: [PATCH v3] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl

[Dave Jiang]

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

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.

> +	}
> +
>  	func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
>  	if (func < 0)
>  		return func;

Re: [PATCH v3] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl

[Suraj Sonawane]

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

> 
> 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?

>> +	}
>> +
>>   	func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
>>   	if (func < 0)
>>   		return func;
> 

Thank you for your feedback and your time.

Re: [PATCH v3] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl

[Dave Jiang]

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

Re: [PATCH v3] acpi: nfit: vmalloc-out-of-bounds Read in acpi_nfit_ctl

[Suraj Sonawane]

On 14/11/24 21:12, Dave Jiang wrote:
> 
> 
> 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+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.
>>>>
>>>>    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.

I have submitted the patch with the original code unchanged(without 
casting) by testing with syzbot. You can view it 
here:https://lore.kernel.org/lkml/20241115164223.20854-1-surajsonawane0215@gmail.com/

> 
>>
>>>> +    }
>>>> +
>>>>        func = cmd_to_func(nfit_mem, cmd, call_pkg, &family);
>>>>        if (func < 0)
>>>>            return func;
>>>
>>
>> Thank you for your feedback and your time.
>