| Message ID | 20180501003907.4322-3-ahs3@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On Tue, May 1, 2018 at 2:39 AM, Al Stone <ahs3@redhat.com> wrote: > For ACPI tables that have subtables, acpi_parse_entries_array() gets used > to step through each of the subtables in memory. The primary loop for this > was checking that the beginning location of the subtable being examined > plus the length of struct acpi_subtable_header was not beyond the end of > the complete ACPI table; if it wasn't, the subtable could be examined, but > if it was the loop would terminate as it should. > > In the middle of this subtable loop, a callback is used to examine the > subtable in detail. > > Should the callback function try to examine elements of the subtable that > are located past the subtable header, and the ACPI table containing this > subtable has an incorrect length, it is possible to access either invalid > or protected memory and cause a fault. And, the length of struct > acpi_subtable_header will always be smaller than the length of the actual > subtable. > > To fix this, we make the main loop check that the beginning of the > subtable being examined plus the actual length of the subtable does > not go past the end of the enclosing ACPI table. While this cannot > protect us from malicious callback functions, it can prevent us from > failing because of some poorly constructed ACPI tables. > > Found by inspection. There is no functional change to existing code > that is known to work when calling acpi_parse_entries_array(). > > Signed-off-by: Al Stone <ahs3@redhat.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > --- > drivers/acpi/tables.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 4a3410aa6540..82c3e2c52dd9 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size, > entry = (struct acpi_subtable_header *) > ((unsigned long)table_header + table_size); > > - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < > - table_end) { > + while ((unsigned long)entry + entry->length <= table_end) { > if (max_entries && count >= max_entries) > break; > > -- This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among other things), so I'm dropping it. I can send you acpidump output from that machine if need be. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote: > On Tue, May 1, 2018 at 2:39 AM, Al Stone <ahs3@redhat.com> wrote: >> For ACPI tables that have subtables, acpi_parse_entries_array() gets used >> to step through each of the subtables in memory. The primary loop for this >> was checking that the beginning location of the subtable being examined >> plus the length of struct acpi_subtable_header was not beyond the end of >> the complete ACPI table; if it wasn't, the subtable could be examined, but >> if it was the loop would terminate as it should. >> >> In the middle of this subtable loop, a callback is used to examine the >> subtable in detail. >> >> Should the callback function try to examine elements of the subtable that >> are located past the subtable header, and the ACPI table containing this >> subtable has an incorrect length, it is possible to access either invalid >> or protected memory and cause a fault. And, the length of struct >> acpi_subtable_header will always be smaller than the length of the actual >> subtable. >> >> To fix this, we make the main loop check that the beginning of the >> subtable being examined plus the actual length of the subtable does >> not go past the end of the enclosing ACPI table. While this cannot >> protect us from malicious callback functions, it can prevent us from >> failing because of some poorly constructed ACPI tables. >> >> Found by inspection. There is no functional change to existing code >> that is known to work when calling acpi_parse_entries_array(). >> >> Signed-off-by: Al Stone <ahs3@redhat.com> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Len Brown <lenb@kernel.org> >> --- >> drivers/acpi/tables.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 4a3410aa6540..82c3e2c52dd9 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size, >> entry = (struct acpi_subtable_header *) >> ((unsigned long)table_header + table_size); >> >> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < >> - table_end) { >> + while ((unsigned long)entry + entry->length <= table_end) { >> if (max_entries && count >= max_entries) >> break; >> >> -- > > This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among > other things), so I'm dropping it. I can send you acpidump output > from that machine if need be. > > Thanks, > Rafael > Yes, please. My fear is that there are a bunch of MADT tables in the real world that aren't quite right so that while this may be theoretically correct, it may be wrong as a practical matter.
On 05/15/2018 03:53 PM, Al Stone wrote: > On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote: >> On Tue, May 1, 2018 at 2:39 AM, Al Stone <ahs3@redhat.com> wrote: >>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used >>> to step through each of the subtables in memory. The primary loop for this >>> was checking that the beginning location of the subtable being examined >>> plus the length of struct acpi_subtable_header was not beyond the end of >>> the complete ACPI table; if it wasn't, the subtable could be examined, but >>> if it was the loop would terminate as it should. >>> >>> In the middle of this subtable loop, a callback is used to examine the >>> subtable in detail. >>> >>> Should the callback function try to examine elements of the subtable that >>> are located past the subtable header, and the ACPI table containing this >>> subtable has an incorrect length, it is possible to access either invalid >>> or protected memory and cause a fault. And, the length of struct >>> acpi_subtable_header will always be smaller than the length of the actual >>> subtable. >>> >>> To fix this, we make the main loop check that the beginning of the >>> subtable being examined plus the actual length of the subtable does >>> not go past the end of the enclosing ACPI table. While this cannot >>> protect us from malicious callback functions, it can prevent us from >>> failing because of some poorly constructed ACPI tables. >>> >>> Found by inspection. There is no functional change to existing code >>> that is known to work when calling acpi_parse_entries_array(). >>> >>> Signed-off-by: Al Stone <ahs3@redhat.com> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Len Brown <lenb@kernel.org> >>> --- >>> drivers/acpi/tables.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >>> index 4a3410aa6540..82c3e2c52dd9 100644 >>> --- a/drivers/acpi/tables.c >>> +++ b/drivers/acpi/tables.c >>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size, >>> entry = (struct acpi_subtable_header *) >>> ((unsigned long)table_header + table_size); >>> >>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < >>> - table_end) { >>> + while ((unsigned long)entry + entry->length <= table_end) { >>> if (max_entries && count >= max_entries) >>> break; >>> >>> -- >> >> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among >> other things), so I'm dropping it. I can send you acpidump output >> from that machine if need be. >> >> Thanks, >> Rafael Let's just drop this completely -- but please do send the acpidump. It's going to take me a while to figure why this innocuous little loop does not behave the way I expect it to. I'll send a separate patch, if needed.
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 4a3410aa6540..82c3e2c52dd9 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size, entry = (struct acpi_subtable_header *) ((unsigned long)table_header + table_size); - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < - table_end) { + while ((unsigned long)entry + entry->length <= table_end) { if (max_entries && count >= max_entries) break;
For ACPI tables that have subtables, acpi_parse_entries_array() gets used to step through each of the subtables in memory. The primary loop for this was checking that the beginning location of the subtable being examined plus the length of struct acpi_subtable_header was not beyond the end of the complete ACPI table; if it wasn't, the subtable could be examined, but if it was the loop would terminate as it should. In the middle of this subtable loop, a callback is used to examine the subtable in detail. Should the callback function try to examine elements of the subtable that are located past the subtable header, and the ACPI table containing this subtable has an incorrect length, it is possible to access either invalid or protected memory and cause a fault. And, the length of struct acpi_subtable_header will always be smaller than the length of the actual subtable. To fix this, we make the main loop check that the beginning of the subtable being examined plus the actual length of the subtable does not go past the end of the enclosing ACPI table. While this cannot protect us from malicious callback functions, it can prevent us from failing because of some poorly constructed ACPI tables. Found by inspection. There is no functional change to existing code that is known to work when calling acpi_parse_entries_array(). Signed-off-by: Al Stone <ahs3@redhat.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> --- drivers/acpi/tables.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)