| Message ID | 20210108072348.34091-1-decui@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow | expand |
> From: Dexuan Cui <decui@microsoft.com> > Sent: Thursday, January 7, 2021 11:24 PM > ... > Linux VM on Hyper-V crashes with the latest mainline: > ... > > Changes in v2: > strlcpy -> kstrdup_const. Thanks Rafael J. Wysocki! > Change commit log accordingly. 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, e.g. https://bugzilla.kernel.org/show_bug.cgi?id=210449 Thanks, -- Dexuan
On 2021-01-08 08:23, 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") > > Fix the issue by changing the field bus_id to const char *, and use > kstrdup_const() to initialize it. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=210449 Tested-By: Jethro Beekman <jethro@fortanix.com> -- Jethro Beekman | Fortanix
> 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:
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!
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
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!
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..8d49d192d1c1 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,14 @@ 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_device_bus_id->instance_no = 0; list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); } @@ -709,6 +717,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:
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") Fix the issue by changing the field bus_id to const char *, and use kstrdup_const() to initialize it. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- Changes in v2: strlcpy -> kstrdup_const. Thanks Rafael J. Wysocki! Change commit log accordingly. drivers/acpi/internal.h | 2 +- drivers/acpi/scan.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-)