[PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Dexuan Cui]

Linux VM on Hyper-V crashes with the latest mainline:

[    4.069624] detected buffer overflow in strcpy
[    4.077733] kernel BUG at lib/string.c:1149!
..
[    4.085819] RIP: 0010:fortify_panic+0xf/0x11
...
[    4.085819] Call Trace:
[    4.085819]  acpi_device_add.cold.15+0xf2/0xfb
[    4.085819]  acpi_add_single_object+0x2a6/0x690
[    4.085819]  acpi_bus_check_add+0xc6/0x280
[    4.085819]  acpi_ns_walk_namespace+0xda/0x1aa
[    4.085819]  acpi_walk_namespace+0x9a/0xc2
[    4.085819]  acpi_bus_scan+0x78/0x90
[    4.085819]  acpi_scan_init+0xfa/0x248
[    4.085819]  acpi_init+0x2c1/0x321
[    4.085819]  do_one_initcall+0x44/0x1d0
[    4.085819]  kernel_init_freeable+0x1ab/0x1f4

This is because of the recent buffer overflow detection in the
commit 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified string functions")

Here acpi_device_bus_id->bus_id can only hold 14 characters, while the
the acpi_device_hid(device) returns a 22-char string
"HYPER_V_GEN_COUNTER_V1".

Per ACPI Spec v6.2, Section 6.1.5 _HID (Hardware ID), if the ID is a
string, it must be of the form AAA#### or NNNN####, i.e. 7 chars or 8
chars.

The field bus_id in struct acpi_device_bus_id was originally defined as
char bus_id[9], and later was enlarged to char bus_id[15] in 2007 in the
commit bb0958544f3c ("ACPI: use more understandable bus_id for ACPI devices")

It looks like so far an ID string of >=15 chars is only seen in the guest
BIOS/firmware by Hyper-V, and AFAIK the ID string "HYPER_V_GEN_COUNTER_V1"
is never used by Linux VM on Hyper-V, so let's just truncate the string to
fix the panic.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/acpi/scan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a1b226eb2ce2..b801442b6b1b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -674,7 +674,8 @@ int acpi_device_add(struct acpi_device *device,
 	}
 	if (!found) {
 		acpi_device_bus_id = new_bus_id;
-		strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
+		strlcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device),
+			sizeof(acpi_device_bus_id->bus_id));
 		acpi_device_bus_id->instance_no = 0;
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}
-- 
2.19.1

RE: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Dexuan Cui]

> From: Dexuan Cui <decui@microsoft.com>
> Sent: Thursday, December 17, 2020 8:08 PM
> 
> Linux VM on Hyper-V crashes with the latest mainline:
>  ...
> This is because of the recent buffer overflow detection in the
> commit 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified
> string functions")
> 
> Here acpi_device_bus_id->bus_id can only hold 14 characters, while the
> the acpi_device_hid(device) returns a 22-char string
> "HYPER_V_GEN_COUNTER_V1".
> 
> Per ACPI Spec v6.2, Section 6.1.5 _HID (Hardware ID), if the ID is a
> string, it must be of the form AAA#### or NNNN####, i.e. 7 chars or 8
> chars.
> 
> The field bus_id in struct acpi_device_bus_id was originally defined as
> char bus_id[9], and later was enlarged to char bus_id[15] in 2007 in the
> commit bb0958544f3c ("ACPI: use more understandable bus_id for ACPI
> devices")
> 
> It looks like so far an ID string of >=15 chars is only seen in the guest
> BIOS/firmware by Hyper-V, and AFAIK the ID string
> "HYPER_V_GEN_COUNTER_V1"
> is never used by Linux VM on Hyper-V, so let's just truncate the string to
> fix the panic.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>

IMO this patch should also go to the stable trees, so please add
Cc: <stable@vger.kernel.org>

Thanks,
-- Dexuan

RE: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Thursday, December 17, 2020 8:08 PM
> 
> Linux VM on Hyper-V crashes with the latest mainline:
> 
> [    4.069624] detected buffer overflow in strcpy
> [    4.077733] kernel BUG at lib/string.c:1149!
> ..
> [    4.085819] RIP: 0010:fortify_panic+0xf/0x11
> ...
> [    4.085819] Call Trace:
> [    4.085819]  acpi_device_add.cold.15+0xf2/0xfb
> [    4.085819]  acpi_add_single_object+0x2a6/0x690
> [    4.085819]  acpi_bus_check_add+0xc6/0x280
> [    4.085819]  acpi_ns_walk_namespace+0xda/0x1aa
> [    4.085819]  acpi_walk_namespace+0x9a/0xc2
> [    4.085819]  acpi_bus_scan+0x78/0x90
> [    4.085819]  acpi_scan_init+0xfa/0x248
> [    4.085819]  acpi_init+0x2c1/0x321
> [    4.085819]  do_one_initcall+0x44/0x1d0
> [    4.085819]  kernel_init_freeable+0x1ab/0x1f4
> 
> This is because of the recent buffer overflow detection in the
> commit 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified string
> functions")
> 
> Here acpi_device_bus_id->bus_id can only hold 14 characters, while the
> the acpi_device_hid(device) returns a 22-char string
> "HYPER_V_GEN_COUNTER_V1".
> 
> Per ACPI Spec v6.2, Section 6.1.5 _HID (Hardware ID), if the ID is a
> string, it must be of the form AAA#### or NNNN####, i.e. 7 chars or 8
> chars.
> 
> The field bus_id in struct acpi_device_bus_id was originally defined as
> char bus_id[9], and later was enlarged to char bus_id[15] in 2007 in the
> commit bb0958544f3c ("ACPI: use more understandable bus_id for ACPI devices")
> 
> It looks like so far an ID string of >=15 chars is only seen in the guest
> BIOS/firmware by Hyper-V, and AFAIK the ID string "HYPER_V_GEN_COUNTER_V1"
> is never used by Linux VM on Hyper-V, so let's just truncate the string to
> fix the panic.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/acpi/scan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a1b226eb2ce2..b801442b6b1b 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -674,7 +674,8 @@ int acpi_device_add(struct acpi_device *device,
>  	}
>  	if (!found) {
>  		acpi_device_bus_id = new_bus_id;
> -		strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
> +		strlcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device),
> +			sizeof(acpi_device_bus_id->bus_id));
>  		acpi_device_bus_id->instance_no = 0;
>  		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
>  	}
> --
> 2.19.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Tuesday, December 22, 2020 5:56 AM
> From: Dexuan Cui 
> Sent: Thursday, December 17, 2020
> 8:08 PM
> >
> > Linux VM on Hyper-V crashes with the latest mainline:
> > ...
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -674,7 +674,8 @@ int acpi_device_add(struct acpi_device *device,
> >  	}
> >  	if (!found) {
> >  		acpi_device_bus_id = new_bus_id;
> > -		strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
> > +		strlcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device),
> > +			sizeof(acpi_device_bus_id->bus_id));
> >  		acpi_device_bus_id->instance_no = 0;
> >  		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
> >  	}
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Hi, ACPI maintainers,
Would you please take a look at the small fix? Currently the mainline Linux
kernel, running in a VM on Hyper-V, has been broken for almost 3 weeks,
i.e. the VM always panics when it boots.

The patch has already had Michael's Reviewed-by.

BTW, the patch should have a stable tag:
Cc: <stable@vger.kernel.org>

Or, do you want the patch to go through the Hyper-V tree?
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-fixes

The small patch is unlikely to cause a merge conflict, and it only affects
Linux VMs on Hyper-V so far.

Thanks,
-- Dexuan

Re: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Dwaipayan Ray]

On 18/12/20 9:38 am, Dexuan Cui wrote:
> Linux VM on Hyper-V crashes with the latest mainline:
>
> [    4.069624] detected buffer overflow in strcpy
> [    4.077733] kernel BUG at lib/string.c:1149!
> ..
> [    4.085819] RIP: 0010:fortify_panic+0xf/0x11
> ...
> [    4.085819] Call Trace:
> [    4.085819]  acpi_device_add.cold.15+0xf2/0xfb
> [    4.085819]  acpi_add_single_object+0x2a6/0x690
> [    4.085819]  acpi_bus_check_add+0xc6/0x280
> [    4.085819]  acpi_ns_walk_namespace+0xda/0x1aa
> [    4.085819]  acpi_walk_namespace+0x9a/0xc2
> [    4.085819]  acpi_bus_scan+0x78/0x90
> [    4.085819]  acpi_scan_init+0xfa/0x248
> [    4.085819]  acpi_init+0x2c1/0x321
> [    4.085819]  do_one_initcall+0x44/0x1d0
> [    4.085819]  kernel_init_freeable+0x1ab/0x1f4
>
> This is because of the recent buffer overflow detection in the
> commit 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified string functions")
>
> Here acpi_device_bus_id->bus_id can only hold 14 characters, while the
> the acpi_device_hid(device) returns a 22-char string
> "HYPER_V_GEN_COUNTER_V1".
>
> Per ACPI Spec v6.2, Section 6.1.5 _HID (Hardware ID), if the ID is a
> string, it must be of the form AAA#### or NNNN####, i.e. 7 chars or 8
> chars.
>
> The field bus_id in struct acpi_device_bus_id was originally defined as
> char bus_id[9], and later was enlarged to char bus_id[15] in 2007 in the
> commit bb0958544f3c ("ACPI: use more understandable bus_id for ACPI devices")
>
> It looks like so far an ID string of >=15 chars is only seen in the guest
> BIOS/firmware by Hyper-V, and AFAIK the ID string "HYPER_V_GEN_COUNTER_V1"
> is never used by Linux VM on Hyper-V, so let's just truncate the string to
> fix the panic.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>   drivers/acpi/scan.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a1b226eb2ce2..b801442b6b1b 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -674,7 +674,8 @@ int acpi_device_add(struct acpi_device *device,
>   	}
>   	if (!found) {
>   		acpi_device_bus_id = new_bus_id;
> -		strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
> +		strlcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device),
> +			sizeof(acpi_device_bus_id->bus_id));

Please prefer strscpy() over strlcpy():

+		strscpy(acpi_device_bus_id->bus_id, acpi_device_hid(device),
+			sizeof(acpi_device_bus_id->bus_id));

See: 
https://lore.kernel.org/lkml/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/

Thanks,
Dwaipayan.

>   		acpi_device_bus_id->instance_no = 0;
>   		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
>   	}

Re: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Rafael J. Wysocki]

On Tue, Jan 5, 2021 at 11:02 PM Dexuan Cui <decui@microsoft.com> wrote:
>
> > From: Michael Kelley <mikelley@microsoft.com>
> > Sent: Tuesday, December 22, 2020 5:56 AM
> > From: Dexuan Cui
> > Sent: Thursday, December 17, 2020
> > 8:08 PM
> > >
> > > Linux VM on Hyper-V crashes with the latest mainline:
> > > ...
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -674,7 +674,8 @@ int acpi_device_add(struct acpi_device *device,
> > >     }
> > >     if (!found) {
> > >             acpi_device_bus_id = new_bus_id;
> > > -           strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
> > > +           strlcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device),
> > > +                   sizeof(acpi_device_bus_id->bus_id));
> > >             acpi_device_bus_id->instance_no = 0;
> > >             list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
> > >     }
> >
> > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
> Hi, ACPI maintainers,
> Would you please take a look at the small fix? Currently the mainline Linux
> kernel, running in a VM on Hyper-V, has been broken for almost 3 weeks,
> i.e. the VM always panics when it boots.

The root cause is a VM issue AFAICS, though.

> The patch has already had Michael's Reviewed-by.
>
> BTW, the patch should have a stable tag:
> Cc: <stable@vger.kernel.org>
>
> Or, do you want the patch to go through the Hyper-V tree?
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-fixes
>
> The small patch is unlikely to cause a merge conflict, and it only affects
> Linux VMs on Hyper-V so far.

It doesn't look like the right fix to me, though.

The problem appears to be that the string coming from _HID is too long
(which is a spec violation).  The patch truncates it to match the
length of the target buffer, but that is not particularly useful.

It would be better to use something like kstrdup_const() to initialize
acpi_device_bus_id->bus_id IMV.

RE: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Dexuan Cui]

> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Thursday, January 7, 2021 5:48 AM
> > > > Linux VM on Hyper-V crashes with the latest mainline:
> > > > ...
> The root cause is a VM issue AFAICS, though.

Yes.
 
> It doesn't look like the right fix to me, though.
> 
> The problem appears to be that the string coming from _HID is too long
> (which is a spec violation). 

Yes. We have requested Hyper-V team to fix the buggy BIOS/firmware,
but we have to cope with the existing buggy Hyper-V hosts, at least
before the Hyper-V fix is deployed to the hosts, and some old versions
of the hosts may not get updated. :-(

> The patch truncates it to match the
> length of the target buffer, but that is not particularly useful.
> 
> It would be better to use something like kstrdup_const() to initialize
> acpi_device_bus_id->bus_id IMV.

Makes sense. I'll submit v2 shortly.

Thanks,
-- Dexuan

Re: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Rafael J. Wysocki]

On Sat, Jan 9, 2021 at 6:08 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Saturday, January 9, 2021 10:37:41 AM CET Dexuan Cui wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Saturday, January 9, 2021 12:52 AM
> > >>
> > >> Hi Rafael, Len, and all,
> > >> Can you please take a look at the v2 patch?
> > >>
> > >> The Linux mainline has been broken for several weeks when it
> > >> runs as a guest on Hyper-V, so we'd like this to be fixed ASAP,
> > >> as more people are being affected
> > >
> > > I would like to see a warning printed when the dupped
> > > string violates the spec.
> >
> > Hi Andy,
> > Do you want a simple strlen() check like the below, or a full
> > check of the AAA#### or NNNN#### format?
>
> It would be good to check the format too while at it.
>
> > Can we have the v2 (https://lkml.org/lkml/2021/1/8/53) merged
> > first, and then we can add another patch for the format checking?
>
> Yes, we can.
>
> I'm going to apply the v2 early next week.

Applied now with a new subject ("ACPI: scan: Harden acpi_device_add()
against device ID overflows") and slightly adjusted white space,
thanks!

Re: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Andy Shevchenko]

On Sat, Jan 9, 2021 at 7:08 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Saturday, January 9, 2021 10:37:41 AM CET Dexuan Cui wrote:

...

> > Do you want a simple strlen() check like the below, or a full
> > check of the AAA#### or NNNN#### format?
>
> It would be good to check the format too while at it.

Let me summarize. It seems from my perspective that the best is to
have two checks here (as far as I got word "too" in Rafael's reply):
 - per length with a message that "length is exceeded"
 - per format with probably different messages depending on the checks
(like "vendor prefix has incorrect format" and "device id has
incorrect format").



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Rafael J. Wysocki]

On Saturday, January 9, 2021 10:37:41 AM CET Dexuan Cui wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com> 
> > Sent: Saturday, January 9, 2021 12:52 AM
> >> 
> >> Hi Rafael, Len, and all,
> >> Can you please take a look at the v2 patch?
> >> 
> >> The Linux mainline has been broken for several weeks when it
> >> runs as a guest on Hyper-V, so we'd like this to be fixed ASAP,
> >> as more people are being affected
> > 
> > I would like to see a warning printed when the dupped
> > string violates the spec.
> 
> Hi Andy,
> Do you want a simple strlen() check like the below, or a full
> check of the AAA#### or NNNN#### format?

It would be good to check the format too while at it.

> Can we have the v2 (https://lkml.org/lkml/2021/1/8/53) merged 
> first, and then we can add another patch for the format checking?

Yes, we can.

I'm going to apply the v2 early next week.

Thanks!

RE: [PATCH] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow

[Dexuan Cui]

> From: Andy Shevchenko <andy.shevchenko@gmail.com> 
> Sent: Saturday, January 9, 2021 12:52 AM
>> 
>> Hi Rafael, Len, and all,
>> Can you please take a look at the v2 patch?
>> 
>> The Linux mainline has been broken for several weeks when it
>> runs as a guest on Hyper-V, so we'd like this to be fixed ASAP,
>> as more people are being affected
> 
> I would like to see a warning printed when the dupped
> string violates the spec.

Hi Andy,
Do you want a simple strlen() check like the below, or a full
check of the AAA#### or NNNN#### format?

Can we have the v2 (https://lkml.org/lkml/2021/1/8/53) merged 
first, and then we can add another patch for the format checking?

I'm trying to do one thing in one patch so the patch is small enough
for easy reviewing.

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index cb229e24c563..e6a5d997241c 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -97,7 +97,7 @@ void acpi_scan_table_handler(u32 event, void *table, void *context);
 extern struct list_head acpi_bus_id_list;
 
 struct acpi_device_bus_id {
-	char bus_id[15];
+	const char *bus_id;
 	unsigned int instance_no;
 	struct list_head node;
 };
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a1b226eb2ce2..3b9902e5d965 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -486,6 +486,7 @@ static void acpi_device_del(struct acpi_device *device)
 				acpi_device_bus_id->instance_no--;
 			else {
 				list_del(&acpi_device_bus_id->node);
+				kfree_const(acpi_device_bus_id->bus_id);
 				kfree(acpi_device_bus_id);
 			}
 			break;
@@ -674,7 +675,23 @@ int acpi_device_add(struct acpi_device *device,
 	}
 	if (!found) {
 		acpi_device_bus_id = new_bus_id;
-		strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
+		acpi_device_bus_id->bus_id =
+			kstrdup_const(acpi_device_hid(device), GFP_KERNEL);
+		if (!acpi_device_bus_id->bus_id) {
+			pr_err(PREFIX "Memory allocation error for bus id\n");
+			result = -ENOMEM;
+			goto err_free_new_bus_id;
+		}
+
+		/*
+		 *  ACPI Spec v6.2, Section 6.1.5 _HID (Hardware ID): if the
+		 * ID is a string, it must be of the form AAA#### or NNNN####,
+		 * i.e. 7 chars or 8 characters.
+		 */
+		if (strlen(acpi_device_bus_id->bus_id) > 8)
+			pr_warn(PREFIX "too long HID name: %s\n",
+				acpi_device_bus_id->bus_id);
+
 		acpi_device_bus_id->instance_no = 0;
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}
@@ -709,6 +726,10 @@ int acpi_device_add(struct acpi_device *device,
 	if (device->parent)
 		list_del(&device->node);
 	list_del(&device->wakeup_list);
+
+ err_free_new_bus_id:
+	if (!found)
+		kfree(new_bus_id);
 	mutex_unlock(&acpi_device_lock);
 
  err_detach: