Comment 1 njl 2003-09-04 01:51:32 UTC

Responsible Changed
From-To: freebsd-bugs->njl

I'm working on this.

A resource string was overwriting the GPE Methodnode pointer that was at
0xc14e3a00.  Luckily it was at the same address each boot so we used a
hwatchpoint in ddb.  The #3 case below is the culprit.

---------- Forwarded message ----------
Date: Wed, 17 Sep 2003 19:18:02 -0500
From: Larry Rosenman <ler@lerctr.org>
To: Nate Lawson <nate@root.org>
Cc: marks@ripe.net

--On Wednesday, September 17, 2003 00:01:55 -0700 Nate Lawson
<nate@root.org> wrote:
> Thanks.  While the dump was too late (since the data had already been
> overwritten), it's useful to have your kernel debug file.  I have some
> exact steps for you to do and then I can really track it down.
>
> 1. boot -sd
> 2. hwatch 0xc14e3a00
> 3. c
> 4. At each breakpoint, type tr.  Use scroll lock and arrow keys to get to
> the top of the buffer.  I only need the top 4-6 function names before
> "breakpoint()" or "calltrap()" but for the top one (probably
> AcpiSomething), I need FuncName + 0xwhatever.
> 5. After you've written that down, goto step 3.  Keep doing this until you
> get the panic.
> 6. Don't bother doing a dump.  Give me the above function trace for each
> run.

Here we go:

1) generic_bzero
   AcpiEvCreateGpeInfoBlocks

 I figure this is normal, so I didn't go farther in the trace.

2) AcpiEvSaveMethodInfo+0xA1
   AcpiNsWalkNameSpace
   AcpiCreateGpeBlock
   AcpiEvGpeInitialize

3) AcpiRsEndTagResource+0x17
   AcpiRsByteStreamToList
   AcpiRsCreateResourceList
   AcpiRsGetCrsMethodData

then we get the system to a prompt, and the standard panic on transition

LER

Here is where we find the exact resource list that is overwriting the
pointer.  I built a custom acpica interpreter with a small program
attached to step through the resource list decoding routine.

This also shows it was the end tag that was finally hitting MethodNode:
(gdb) l *AcpiRsEndTagResource+0x17 0xc0169397 is in AcpiRsEndTagResource
(/usr/src/sys/contrib/dev/acpica/rsmisc.c:174).
169         OutputStruct->Id = ACPI_RSTYPE_END_TAG;
170
171         /*
172          * Set the Length parameter
173          */
174         OutputStruct->Length = 0;
175
176         /*
177          * Return the final size of the structure
178          */


---------- Forwarded message ----------
Date: Thu, 18 Sep 2003 14:03:06 -0700 (PDT)
From: Nate Lawson <nate@root.org>
To: Larry Rosenman <ler@lerctr.org>
Cc: marks@ripe.net
Subject: Re: acpi panics

On Thu, 18 Sep 2003, Larry Rosenman wrote:
> That's better.  One hit.  dmesg attached.

Ok, I have isolated the problem to one particular resource, \PCI0\RSRC.
It has various addr 16 and addr 32 types.  I am certain the bug is in how
ACPICA parses this buffer.  Below is the output from two runs.  The first
is when it calculates the space needed.  The second is when it stores the
data in the buffer.  As you can see, the first run allocates 488 bytes and
then the second overflows this by 36 bytes.

Calc pass
---
Resource 88 len 64
Resource 88 len 28
Resource 88 len 64
Resource 88 len 64
Resource 87 len 64
Resource 87 len 64
Resource 87 len 64
Resource 87 len 64
Resource 87 len 12
total len 488

Parse pass
---
size 76
size 28
size 68
size 68
size 68
size 68
size 68
size 68
size 12
total len 524

And it so happens that the buf at 0xc14e3800 + 524 bytes is
[drumroll please]
0xc14e3a0c

Which is exactly 12 bytes into the GPE eventinfo buffer.

Now I just have to untangle the rsaddr.c functions for WORD/DWORD values
and see just WHAT THEIR PROBLEM IS!

-Nate

Here is the offending Buffer from his ASL:

\PCI0\RSRC

Name(RSRC, Buffer(0xa9) {
0x88, 0xe, 0x0, 0x2, 0xc, 0x0, 0x0, 0x0, 0x0, 0x0, 0xff, 0x0, 0x0, 0x0,
    0x0, 0x1, 0x0,
0x47, 0x1, 0xf8, 0xc, 0xf8, 0xc, 0x1, 0x8,
0x88, 0xe, 0x0, 0x1, 0xc, 0x3, 0x0, 0x0, 0x0, 0x0, 0xf7, 0xc, 0x0, 0x0,
    0xf8, 0xc, 0x0,
0x88, 0xe, 0x0, 0x1, 0xc, 0x3, 0x0, 0x0, 0x0, 0xd, 0xff, 0xff, 0x0, 0x0,
    0x0, 0xf3, 0x0,
0x87, 0x18, 0x0, 0x0, 0xc, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xa, 0x0,
    0xff, 0x ff, 0xb, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x2, 0x0, 0x0,
0x87, 0x18, 0x0, 0x0, 0xc, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc, 0x0,
    0xff, 0xff, 0xc, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0,
0x87, 0x18, 0x0, 0x0, 0xc, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xd, 0x0,
    0xff, 0xff, 0xe, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x2, 0x0, 0x0,
0x87, 0x18, 0x0, 0x0, 0xc, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
    0xff, 0xff, 0xbf, 0xfe, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x79, 0x0 })

And my interpretation of it:

88 e 0 2 c 0 0 0 0 0 ff 0 0 0 0 1 0
WORD address

47 1 f8 c f8 c 1 8
IO port

88 e 0 1 c 3 0 0 0 0 f7 c 0 0 f8 c 0
WORD address

88 e 0 1 c 3 0 0 0 d ff ff 0 0 0 f3 0
WORD address

87 18 0 0 c 3 0 0 0 0 0 0 a 0 ff ff b 0 0 0 0 0 0 0 2 0 0
DWORD address

87 18 0 0 c 3 0 0 0 0 0 f0 c 0 0 f0 c 0 0 0 0 0 0 0 0 0 0
DWORD address

87 18 0 0 c 3 0 0 0 0 0 f0 c 0 ff ff d 0 0 0 0 0 0 10 1 0 0
DWORD address

87 18 0 0 c 3 0 0 0 0 0 0 0 20 ff ff bf fe 0 0 0 0 0 0 c0 de 0
DWORD address

79 0
End tag

Here is the summary and patch.

---------- Forwarded message ----------
Date: Fri, 19 Sep 2003 08:09:02 -0700 (PDT)
From: Nate Lawson <nate@root.org>
To: acpi-jp@jp.freebsd.org
Cc: acpi-devel@lists.sourceforge.net
Subject: [PATCH] invalid resource lists and extra checking

A FreeBSD user has been having a problem where his system panics on
transition to battery from AC.  The PR is below:

    http://www.freebsd.org/cgi/query-pr.cgi?pr=56254

After some tracking, I found that his GPE block was being overwritten by
the end of a resource list (type Address16).  It also happened for
Address32.  I did some more debugging and found that he does not have a
Resource Index or Resource Source String but his resources have an extra
trailing zero byte on Address type resources (but not fixed-length
resources like IO ports).  For instance, here is an Address16 resource:

   0x88, 0xe, 0x0, 0x2, 0xc, 0x0, 0x0, 0x0,
   0x0, 0x0, 0xff, 0x0, 0x0, 0x0, 0x0, 0x1,
   0x0

As you can see, it has a length of 14 (total length 17) which is one extra
byte but there is no Resource Source String.  The spec explicitly says on
page 194 (Table 6-27):

====
Byte 16 -- Resource Source Index (Optional)
Only present if Resource Source (below) is present. This field gives an
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
index to the specific resource descriptor that this device consumes from
in the current resource template for the device object pointed to in
Resource Source.

String -- Resource Source (Optional)
If present, the device ...
====

This can be read that for an Address16, valid lengths are 13 and > 14
since if Resource Index is present, Resource Source String also must be
present.  I'm not sure if a zero-length string (other than the terminating
NUL) is valid.  If not, then the valid lengths are 13 and > 15.  This is
already assumed by AcpiRsGetListLength() where it subtracts the length
from 14 to get the length of the Resource Source String.

This allowed me to develop the following patch for Address16.  It also
includes extra checks that the length meets the minimum specified by the
standard and for the extended interrupt resource, that there is at least
one interrupt present.

Let me know that I can commit this to our vendor branch and can expect it
in a future release.

-Nate

Index: rsaddr.c
===================================================================
RCS file: /home/ncvs/src/sys/contrib/dev/acpica/rsaddr.c,v
retrieving revision 1.1.1.11
diff -u -r1.1.1.11 rsaddr.c
--- rsaddr.c	13 Jul 2003 22:43:31 -0000	1.1.1.11
+++ rsaddr.c	19 Sep 2003 04:59:50 -0000
@@ -168,6 +168,10 @@
     Buffer += 1;
     ACPI_MOVE_16_TO_16 (&Temp16, Buffer);

+    /* Check for the minimum length. */
+    if (Temp16 < 13)
+        return_ACPI_STATUS (AE_AML_INVALID_RESOURCE_TYPE);
+
     *BytesConsumed = Temp16 + 3;
     OutputStruct->Id = ACPI_RSTYPE_ADDRESS16;

@@ -275,11 +279,13 @@
     /*
      * This will leave us pointing to the Resource Source Index
      * If it is present, then save it off and calculate the
-     * pointer to where the null terminated string goes:
-     * Each Interrupt takes 32-bits + the 5 bytes of the
-     * stream that are default.
+     * pointer to where the null terminated string goes.
+     *
+     * Note that some buggy resources have a length that indicates the
+     * Index byte is present even though it isn't (since there is no
+     * following Resource String.)  We add one to catch these.
      */
-    if (*BytesConsumed > 16)
+    if (*BytesConsumed > 16 + 1)
     {
         /* Dereference the Index */

@@ -555,6 +561,10 @@
      */
     Buffer += 1;
     ACPI_MOVE_16_TO_16 (&Temp16, Buffer);
+
+    /* Check for the minimum length. */
+    if (Temp16 < 23)
+        return_ACPI_STATUS (AE_AML_INVALID_RESOURCE_TYPE);
     *BytesConsumed = Temp16 + 3;

     OutputStruct->Id = ACPI_RSTYPE_ADDRESS32;
@@ -667,9 +677,13 @@
     /*
      * This will leave us pointing to the Resource Source Index
      * If it is present, then save it off and calculate the
-     * pointer to where the null terminated string goes:
+     * pointer to where the null terminated string goes.
+     *
+     * Note that some buggy resources have a length that indicates the
+     * Index byte is present even though it isn't (since there is no
+     * following Resource String.)  We add one to catch these.
      */
-    if (*BytesConsumed > 26)
+    if (*BytesConsumed > 26 + 1)
     {
         /* Dereference the Index */

@@ -944,7 +958,11 @@
     Buffer += 1;
     ACPI_MOVE_16_TO_16 (&Temp16, Buffer);

+    /* Check for the minimum length. */
+    if (Temp16 < 43)
+        return_ACPI_STATUS (AE_AML_INVALID_RESOURCE_TYPE);
     *BytesConsumed = Temp16 + 3;
+
     OutputStruct->Id = ACPI_RSTYPE_ADDRESS64;

     /*
@@ -1056,11 +1074,13 @@
     /*
      * This will leave us pointing to the Resource Source Index
      * If it is present, then save it off and calculate the
-     * pointer to where the null terminated string goes:
-     * Each Interrupt takes 32-bits + the 5 bytes of the
-     * stream that are default.
+     * pointer to where the null terminated string goes.
+     *
+     * Note that some buggy resources have a length that indicates the
+     * Index byte is present even though it isn't (since there is no
+     * following Resource String.)  We add one to catch these.
      */
-    if (*BytesConsumed > 46)
+    if (*BytesConsumed > 46 + 1)
     {
         /* Dereference the Index */

Index: rsirq.c
===================================================================
RCS file: /home/ncvs/src/sys/contrib/dev/acpica/rsirq.c,v
retrieving revision 1.1.1.12
diff -u -r1.1.1.12 rsirq.c
--- rsirq.c	13 Jul 2003 22:43:39 -0000	1.1.1.12
+++ rsirq.c	19 Sep 2003 05:03:51 -0000
@@ -408,7 +408,11 @@
     Buffer += 1;
     ACPI_MOVE_16_TO_16 (&Temp16, Buffer);

+    /* Check for the minimum length. */
+    if (Temp16 < 6)
+        return_ACPI_STATUS (AE_AML_INVALID_RESOURCE_TYPE);
     *BytesConsumed = Temp16 + 3;
+
     OutputStruct->Id = ACPI_RSTYPE_EXT_IRQ;

     /*
@@ -446,6 +450,12 @@
     Buffer += 1;
     Temp8 = *Buffer;

+    /* Minimum number of IRQs is one. */
+    if (Temp8 < 1) {
+        *BytesConsumed = 0;
+        return_ACPI_STATUS (AE_AML_INVALID_RESOURCE_TYPE);
+    }
+
     OutputStruct->Data.ExtendedIrq.NumberOfInterrupts = Temp8;

     /*
@@ -480,7 +490,8 @@
      * stream that are default.
      */
     if (*BytesConsumed >
-        ((ACPI_SIZE) OutputStruct->Data.ExtendedIrq.NumberOfInterrupts * 4) + 5)
+        ((ACPI_SIZE) OutputStruct->Data.ExtendedIrq.NumberOfInterrupts * 4)
+        + 5 + 1)
     {
         /* Dereference the Index */

Comment 6 njl 2003-09-19 21:47:40 UTC

State Changed
From-To: open->closed

Patch committed and intel will integrate the changes.  User reports that 
both the battery transition panic and the shutdown panic are gone.