On 12/8/2022 12:55 PM, Rafael J. Wysocki wrote:
On Thu, Dec 8, 2022 at 12:51 PM Amadeusz Sławiński
<amadeuszx.slawinski@xxxxxxxxxxxxxxx> wrote:
In our tests we get UBSAN warning coming from ACPI parser. This is
caused by trying to resolve operands when there is none.
[ 0.000000] Linux version 5.15.0-rc3chromeavsrel1.0.184+ (root@...) (gcc (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #1 SMP PREEMPT Sat Oct 16 00:08:27 UTC 2021
...
[ 14.719508] ================================================================================
[ 14.719551] UBSAN: array-index-out-of-bounds in /.../linux/drivers/acpi/acpica/dswexec.c:401:12
[ 14.719594] index -1 is out of range for type 'acpi_operand_object *[9]'
[ 14.719621] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc3chromeavsrel1.0.184+ #1
[ 14.719657] Hardware name: Intel Corp. Geminilake/GLK RVP2 LP4SD (07), BIOS GELKRVPA.X64.0214.B50.2009111159 09/11/2020
[ 14.719694] Call Trace:
[ 14.719712] dump_stack_lvl+0x38/0x49
[ 14.719749] dump_stack+0x10/0x12
[ 14.719775] ubsan_epilogue+0x9/0x45
[ 14.719801] __ubsan_handle_out_of_bounds.cold+0x44/0x49
[ 14.719835] acpi_ds_exec_end_op+0x1d7/0x6b5
[ 14.719870] acpi_ps_parse_loop+0x942/0xb34
...
Problem happens because WalkState->NumOperands is 0 and it is used when
trying to access into operands table. Actual code is:
WalkState->Operands [WalkState->NumOperands -1]
which causes out of bound access. Improve the check before above access
to check if ACPI opcode should have any arguments (operands) at all.
Link: https://github.com/acpica/acpica/pull/745
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
Reviewed-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
---
drivers/acpi/acpica/dswexec.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpica/dswexec.c b/drivers/acpi/acpica/dswexec.c
index e8ad41387f84..489c9b9d8d15 100644
--- a/drivers/acpi/acpica/dswexec.c
+++ b/drivers/acpi/acpica/dswexec.c
@@ -389,9 +389,11 @@ 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.
+ * being the object_type and size_of operators as well as operands that
Should this be "opcodes that take no arguments" rather?
Yes, it makes more sense that way. I've send v2 and updated pull request
to acpica.
+ * take no arguments.
*/
- if (!(walk_state->op_info->flags & AML_NO_OPERAND_RESOLVE)) {
+ if (!(walk_state->op_info->flags & AML_NO_OPERAND_RESOLVE) &&
+ (walk_state->op_info->flags & AML_HAS_ARGS)) {
/* Resolve all operands */
--
