| Message ID | 1443712312-9176-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Rafael, On 01/10/15 16:11, Sudeep Holla wrote: > acpi_parse_entries passes the table end pointer to the sub-table entry > handler. acpi_parse_entries itself could validate the end of an entry > against the table end using the length in the sub-table entry. > > This patch adds the validation of the sub-table entry end using the > length field.This will help to eliminate the need to pass the table end > to the handlers. > > It also moves the check for zero length entry early so that execution of > the handler can be avoided. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Now that Al's MADT is queued, can you consider applying these ?
On 10/15/2015 09:44 AM, Sudeep Holla wrote: > Hi Rafael, > > On 01/10/15 16:11, Sudeep Holla wrote: >> acpi_parse_entries passes the table end pointer to the sub-table entry >> handler. acpi_parse_entries itself could validate the end of an entry >> against the table end using the length in the sub-table entry. >> >> This patch adds the validation of the sub-table entry end using the >> length field.This will help to eliminate the need to pass the table end >> to the handlers. >> >> It also moves the check for zero length entry early so that execution of >> the handler can be avoided. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Now that Al's MADT is queued, can you consider applying these ? > I think we have to remove my patches from the queue until I can make them arch-specific; doing these MADT checks breaks far too many existing x86 systems where the firmware does things it should not; re-reading some of the ia64 kernel code, there's a pathological case there where it could break (but shouldn't if iasl is being used to compile tables). I'll be working on the new version today.
On Thursday, October 15, 2015 09:57:38 AM Al Stone wrote: > On 10/15/2015 09:44 AM, Sudeep Holla wrote: > > Hi Rafael, > > > > On 01/10/15 16:11, Sudeep Holla wrote: > >> acpi_parse_entries passes the table end pointer to the sub-table entry > >> handler. acpi_parse_entries itself could validate the end of an entry > >> against the table end using the length in the sub-table entry. > >> > >> This patch adds the validation of the sub-table entry end using the > >> length field.This will help to eliminate the need to pass the table end > >> to the handlers. > >> > >> It also moves the check for zero length entry early so that execution of > >> the handler can be avoided. > >> > >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > Now that Al's MADT is queued, can you consider applying these ? > > > > I think we have to remove my patches from the queue until I can make > them arch-specific; doing these MADT checks breaks far too many existing > x86 systems where the firmware does things it should not; Right. I've dropped the series already. > re-reading some of the ia64 kernel code, there's a pathological case there > where it could break (but shouldn't if iasl is being used to compile tables). I wouldn't count on iasl being used. > I'll be working on the new version today. 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
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index a2ed38a20e7e..24b867e26191 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -476,7 +476,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size, unsigned int max_entries) { struct acpi_subtable_header *entry; - unsigned long table_end; + unsigned long table_end, entry_end; int count = 0; int i; @@ -497,12 +497,20 @@ acpi_parse_entries_array(char *id, unsigned long table_size, table_end = (unsigned long)table_header + table_header->length; /* Parse all entries looking for a match. */ + entry_end = (unsigned long)table_header + table_size; + entry = (struct acpi_subtable_header *)entry_end; + entry_end += entry->length; - entry = (struct acpi_subtable_header *) - ((unsigned long)table_header + table_size); + while (entry_end <= table_end) { + /* + * If entry->length is 0, break from this loop to avoid + * infinite loop. + */ + if (entry->length == 0) { + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); + return -EINVAL; + } - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < - table_end) { if (max_entries && count >= max_entries) break; @@ -523,17 +531,8 @@ acpi_parse_entries_array(char *id, unsigned long table_size, if (i != proc_num) count++; - /* - * If entry->length is 0, break from this loop to avoid - * infinite loop. - */ - if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); - return -EINVAL; - } - - entry = (struct acpi_subtable_header *) - ((unsigned long)entry + entry->length); + entry = (struct acpi_subtable_header *)entry_end; + entry_end += entry->length; } if (max_entries && count > max_entries) {
acpi_parse_entries passes the table end pointer to the sub-table entry handler. acpi_parse_entries itself could validate the end of an entry against the table end using the length in the sub-table entry. This patch adds the validation of the sub-table entry end using the length field.This will help to eliminate the need to pass the table end to the handlers. It also moves the check for zero length entry early so that execution of the handler can be avoided. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/acpi/tables.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) v2->v3: - Rebased on Rafael's linux-pm/bleeding-edge branch to avoid conflicts v1->v2: - Incorporated Rafael's review comments - Moved zero length entry check early - Added a patch to remove the unused table_end parameter