invalid resource lists and extra checking
PATCH 1/1 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
rsaddr.c +30 -10
@@ -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
rsirq.c +12 -1
@@ -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 */
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
rsaddr.c line 173
Quoted 9 lines
rsaddr.c +3
-0@@ -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 );
For which acpica version is this exactly? I tried to apply it to a Linux tree which has nearly the latest, but it rejected on every hunk. The file is also repetive enough that hand applying looks a bit risky.
Can you perhaps post a patch against the latest ACPICA version?
-Andi
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
Quoted 14 lines · Andi Kleen
rsaddr.c line 173
Quoted 9 lines
rsaddr.c +3
-0@@ -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 );For which acpica version is this exactly? I tried to apply it to a Linux tree which has nearly the latest, but it rejected on every hunk. The file is also repetive enough that hand applying looks a bit risky.
Quoted 1 line
Can you perhaps post a patch against the latest ACPICA version?
This was against 0619. Things can't have diverged THAT much since the only public release after this was 0714 and the linux pushes have been mostly in the linux-specific code (i.e. PIC). I have a feeling you didn't run the patch through the LiNuXuLaTor de-caps tool and that's why it rejected. Apply it against a stock acpica unix dist and then de-caps it.
-Nate
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf