t2linux / T2-Debian-and-Ubuntu-Kernel

Ubuntu Kernel for T2 Macs.
130 stars 14 forks source link

UBSAN: array-index-out-of-bounds in drivers/acpi/acpica/dswexec.c:401:12 #14

Closed andersfugmann closed 2 years ago

andersfugmann commented 2 years ago

UBSAN complains about using array index of -1 when resolving ACPI operands when there are none.

UBSAN: array-index-out-of-bounds in drivers/acpi/acpica/dswexec.c:401:12
index -1 is out of range for type 'acpi_operand_object *[9]'
CPU: 3 PID: 811 Comm: tlp Tainted: G         C        5.17.0-t2 #1
Hardware name: Apple Inc. MacBookPro16,1/Mac-E1008331FDC96864, BIOS 1715.81.2.0.0 (iBridge: 19.16.10744.0.0,0) 01/06/2022

A more detailed description of the bug + possible solution can be seen here: https://github.com/acpica/acpica/pull/745 The bug seems to be triggered only on apple MBP's, but the solution seems correct for all architectures.

Could we add a patch to add a patch for this to the T2 kernels until the fix finds its way into the linux kernel:

patches/1003-fix-acpica-for-zero-arguments-acpi-calls.patch:

diff --git a/drivers/acpi/acpica/dswexec.c b/drivers/acpi/acpica/dswexec.c
index f2d2267054af..f3766ec3847a 100644
--- a/drivers/acpi/acpica/dswexec.c
+++ b/drivers/acpi/acpica/dswexec.c
@@ -391,7 +391,8 @@ acpi_status acpi_ds_exec_end_op(struct acpi_walk_state *walk_state)
                 * All opcodes require operand resolution, with the only exceptions
                 * being the object_type and size_of operators.
                 */
-               if (!(walk_state->op_info->flags & AML_NO_OPERAND_RESOLVE)) {
+               if (!(walk_state->op_info->flags & AML_NO_OPERAND_RESOLVE) &&
+                       walk_state->num_operands > 0) {

                        /* Resolve all operands */              
AdityaGarg8 commented 2 years ago

I’d rather want to have a review by @Redecorating and @aunali1 on this.

andersfugmann commented 2 years ago

Btw. This kernel error log messages was observed on v5.17-1

$ uname -a 
Linux trillian 5.17.0-t2 #1 SMP PREEMPT Wed Mar 23 12:03:25 UTC 2022 x86_64 GNU/Linux
AdityaGarg8 commented 2 years ago

Oh I see. So not in 5.16.x or 5.15.x?

AdityaGarg8 commented 2 years ago

Also, have you tested this patch?

andersfugmann commented 2 years ago

Actually not. I was hoping that GH Actions could test if it compiles. Let me see if I can create the packages locally and create a proper PR.

AdityaGarg8 commented 2 years ago

Well then you could just add the patch to the patches folder and create a PR for the same.

andersfugmann commented 2 years ago

That's exactly what I was planning to (and did) :-)

See #15

AdityaGarg8 commented 2 years ago

Alright. Lemme test it on my side and then send it to the collection of other patches.

AdityaGarg8 commented 2 years ago

Well, this bug seems to affect the older 5.16.x and 5.15.x kernels as well. Well, I am accepting the patch and backporting it as well. You probably could make the developers aware of the fact that it needs to be backported to stable as well.

AdityaGarg8 commented 2 years ago

New kernels with your patch shall be released soon.

andersfugmann commented 2 years ago

Awesome - Thanks. The ACPICA developers are well aware that the bug existed in at least Linux 5.15 (As they reference kernel 5.15rc3 in the commit message for the fix). I don't know why I did not see this error before, but errors are indeed also present in my kernel logs from 5.16.