Bug 194845

Summary: AML Debugger - UBSAN: Undefined behaviour in drivers/acpi/acpica/dsutils.c:640:16
Product: ACPI Reporter: Ronald (rwarsow)
Component: ACPICA-CoreAssignee: Lv Zheng (lv.zheng)
Status: CLOSED DUPLICATE    
Severity: normal CC: Robert.Moore, rui.zhang, rwarsow
Priority: P1    
Hardware: Intel   
OS: Linux   
Kernel Version: 4.10.1 Subsystem:
Regression: No Bisected commit-id:
Attachments: dmesg
acpidump generated with acpidump -z -o acpidump.txt attached

Description Ronald 2017-03-11 00:02:46 UTC
Created attachment 255177 [details]
dmesg

with this setting:

CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
CONFIG_UBSAN=y
CONFIG_UBSAN_SANITIZE_ALL=y
CONFIG_UBSAN_NULL=y

I get a Call Trace (see attachement: dmesg)
Comment 1 Lv Zheng 2017-03-27 08:22:38 UTC
Linking to http://bugs.acpica.org/show_bug.cgi?id=1372
Please track it there.
Comment 2 Robert Moore 2017-03-28 15:49:32 UTC
Please post the acpidump for this machine, thanks.
Comment 3 Ronald 2017-03-28 16:21:50 UTC
Created attachment 255613 [details]
acpidump generated with acpidump -z -o acpidump.txt attached
Comment 4 Robert Moore 2017-03-28 20:04:08 UTC
[    0.361973] UBSAN: Undefined behaviour in drivers/acpi/acpica/dsutils.c:640:16
[    0.362026] index -1 is out of range for type 'acpi_operand_object *[9]'

This is in fact a bug in ACPICA, but fortunately does not cause any problems other than being caught by UBSAN.

The problem code is used only for single stepping AML code with the AML debugger, and should not be present if the debugger is not present anyway.
Comment 5 Ronald 2017-03-28 21:20:17 UTC
yep, box runs fine without above config options !

it's up to you to close this bug report.

THX for quick support
Comment 6 Robert Moore 2017-03-28 21:53:45 UTC
Ok, I'm closing this. We will make a change for the next release of ACPICA, but as you have validated, there is no harm for now, except for the UBSAN message.
Comment 7 Lv Zheng 2017-03-29 02:58:57 UTC
I don't know what you guys are talking about...
I can even reproduce this issue with latest ACPICA upstream.
It's not related to the debugger, to the single stepping mode.

In the DSDT, there is such a function:

                Device (LPTE)
                {
                    Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
                    {
                        If (^^SIO1.LPTM (0x02))
                        {
                            Return (0x0104D041)
                        }
                        Else
                        {
                            Return (0x0004D041)
                        }
                    }

This happens each time AcpiUtExecute_HID() executes LPTE._HID after executing several other _HIDs.

However if I create a table with only this device:
DefinitionBlock ("dsdt-hid.aml", "DSDT", 2, "INTEL ", "DH77KC  ", 0x0000006E)
{
    Scope (\_SB)
    {
        Device (LPTE)
        {
            Method (_HID, 0, NotSerialized)
            {
                Return (0x0004D041)
            }
        }
    }
}

It then cannot be reproduced.

As such, I don't think this issue has been solved.
Comment 8 Lv Zheng 2017-03-29 04:44:09 UTC
And I cannot reproduce with this:

DefinitionBlock ("dsdt-hid.aml", "DSDT", 2, "INTEL ", "DH77KC  ", 0x0000006E)
{
    Scope (\_SB)
    {
        Device (LPTE)
        {
            Method (_HID, 0, NotSerialized)
            {
                Return (0x0104D041)
            }
        }
    }
}

Capturing an execution sequence of the opcodes, I obtained:
  extrace-0304 [07] ExTracePoint          : Method Begin [0x00733200:\_SB.PCI0.L
PCB.LPTE._HID] execution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x00733200:If] executi
on.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x00733202:-NamePath-]
 execution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x0073320D:ByteConst]
execution.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x0073320D:ByteConst] ex
ecution.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x00733202:-MethodCall-]
 execution.
  extrace-0304 [09] ExTracePoint          : Method Begin [0x007327BA:\_SB.PCI0.L
PCB.SIO1.LPTM] execution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x007327BA:-NamePath-]
 execution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x007327BE:-NamePath-]
 execution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x007327C2:Arg0] execu
tion.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x007327C2:Arg0] executi
on.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x007327BE:-MethodCall-]
 execution.
  extrace-0304 [09] ExTracePoint          : Method Begin [0x00732913:\_SB.PCI0.L
PCB.SIO1.CGLD] execution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x00732913:Return] exe
cution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x00732914:DerefOf] ex
ecution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x00732915:Index] exec
ution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x00732916:-NamePath-]
 execution.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x00732916:-NamePath-] e
xecution.
  extrace-0304 [08] ExTracePoint          : Opcode Begin [0x0073291A:Arg0] execu
tion.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x0073291A:Arg0] executi
on.
  extrace-0304 [10] ExTracePoint          : Opcode End [0x00732915:Index] execut
ion.

The bug should happen during this sequence.
Comment 9 Lv Zheng 2017-03-29 04:49:15 UTC
It should either be a bug of DerefOf or a bug of MethodCall.
Comment 10 Lv Zheng 2017-03-29 04:50:40 UTC
This looks suspicious:
                    Method (CGLD, 1, NotSerialized)
                    {
                        Return (DerefOf (Index (DCAT, Arg0)))
                    }
Comment 11 Lv Zheng 2017-03-29 04:56:49 UTC
> It's not related to the debugger, to the single stepping mode.

If you mean the following line won't be executed if CONFIG_ACPI_DEBUGGER=n:
            AcpiDbDisplayArgumentObject (
                WalkState->Operands [WalkState->NumOperands - 1], WalkState);
Yes, as AcpiDbDisplayArgumentObject() will be no-op if CONFIG_ACPI_DEBUGGER=n.
Comment 12 Lv Zheng 2017-03-29 07:22:56 UTC
This may fix the issue:
https://github.com/acpica/acpica/pull/245

The ACPI_PARSEOP_IN_STACK implementation looks hackish.
Probably need to be validated and removed.
However fixing this issue needn't go that far.
Comment 13 Lv Zheng 2017-03-29 07:37:28 UTC

*** This bug has been marked as a duplicate of bug 120351 ***
Comment 14 Robert Moore 2017-03-29 14:58:04 UTC
The problem only shows up under UBSAN. Normal behavior is able to handle the -1 index even though it is wrong (probably just by chance).
Comment 15 Lv Zheng 2017-03-30 02:05:01 UTC
If CONFIG_ACPI_DEBUGGER=n or single step mode is not enabled (need to initiate acpidbg and type "debug xxxx" in its shell), the content of the wrong memory address won't be touched. And even in case of CONFIG_ACPI_DEBUGGER=y and single step mode is enabled, the wrong memory address will only be read, and it is likely that the address is pointing to a readable memory.
So there is no critical problems occurred to this bug.
However there are really out-of-bound access bugs occurred in ACPICA in this case.

IMO, this bug is trivial.
Comment 16 Robert Moore 2017-03-30 17:17:20 UTC
Is the AML debugger enabled via:

CONFIG_ACPI_DEBUGGER=y

This makes a difference in the code path.
Comment 17 Lv Zheng 2017-03-31 05:52:57 UTC
Yes, this function is a stub when CONFIG_ACPI_DEBUGGER is enabled.
Comment 18 Lv Zheng 2017-03-31 05:53:08 UTC
Yes, this function is a stub when CONFIG_ACPI_DEBUGGER is disabled.
Comment 19 Lv Zheng 2017-03-31 05:56:38 UTC
See, there is no chance this line can actually be triggered to lead to a final "read" of the wrong memory address.
As the single step mode is always false when this line is reached.
Even we are using "debug some_method", we'll have to type "go" to continue and it changes the single step mode back to false.
So for now, it is dead untested code.
If we changed single step determination logics, then we can see this debugger message only dumps duplicate junks ranther than any useful information.
Comment 20 Lv Zheng 2017-03-31 06:05:58 UTC
IMO, this line is debugging purpose, used along with the "Argument previously created, already stacked" debugging message to debug PARSEOP_IN_STACK logics.

Such debugging message isn't clean and useful, so we can remove them both.