| Message ID | 1442243686-15845-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
On Monday, September 14, 2015 04:14:46 PM 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. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Well, I'm not a big fan of (void *) arithmetics and the patch seems to be doing too much to me. > --- > drivers/acpi/tables.c | 34 +++++++++------------------------- > 1 file changed, 9 insertions(+), 25 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 17a6fa01a338..145d4f6a1c54 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -217,16 +217,13 @@ acpi_parse_entries(char *id, unsigned long table_size, > int entry_id, unsigned int max_entries) > { > struct acpi_subtable_header *entry; > + void *entry_end, *table_end; > int count = 0; > - unsigned long table_end; I'd keep that as unsigned long and I'd add unsigned long entry_end here. > > if (acpi_disabled) > return -ENODEV; > > - if (!id || !handler) > - return -EINVAL; > - > - if (!table_size) > + if (!id || !handler || !table_size) > return -EINVAL; Please mention this cleanup bit in the changelog, as it is not related to the other changes. > > if (!table_header) { > @@ -234,34 +231,21 @@ acpi_parse_entries(char *id, unsigned long table_size, > return -ENODEV; > } > > - table_end = (unsigned long)table_header + table_header->length; > + table_end = (void *)table_header + table_header->length; > > /* Parse all entries looking for a match. */ > + entry = (void *)table_header + table_size; > + entry_end = (void *)entry + entry->length; > > - entry = (struct acpi_subtable_header *) > - ((unsigned long)table_header + table_size); entry_end = (unsigned long)table_header + table_size; entry = (struct acpi_subtable_header *)entry_end; entry_end += entry->length; > - > - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < > - table_end) { > + while (entry->length && entry_end <= table_end) { We used to return -EINVAL for entry->length == 0 and now we don't. Isn't that a problem? > if (entry->type == entry_id > && (!max_entries || count < max_entries)) { > - if (handler(entry, table_end)) > + if (handler(entry, (unsigned long)entry_end)) > return -EINVAL; > - > 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, entry_id); > - return -EINVAL; > - } Perhaps just reorder this with the previous if () and write the loop as while (entry_end <= table_end) { > - > - entry = (struct acpi_subtable_header *) > - ((unsigned long)entry + entry->length); > + entry = entry_end; > + entry_end = (void *)entry + entry->length; And this can be entry = (struct acpi_subtable_header *)entry_end; entry_end += entry->length; > } > > if (max_entries && count > max_entries) { > 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 15/09/15 00:41, Rafael J. Wysocki wrote: > On Monday, September 14, 2015 04:14:46 PM 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. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > Well, I'm not a big fan of (void *) arithmetics and the patch seems to be > doing too much to me. > Agreed, I will try to remove(if not most likely reduce) it. >> --- >> drivers/acpi/tables.c | 34 +++++++++------------------------- >> 1 file changed, 9 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 17a6fa01a338..145d4f6a1c54 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -217,16 +217,13 @@ acpi_parse_entries(char *id, unsigned long table_size, >> int entry_id, unsigned int max_entries) >> { >> struct acpi_subtable_header *entry; >> + void *entry_end, *table_end; >> int count = 0; >> - unsigned long table_end; > > I'd keep that as unsigned long and I'd add unsigned long entry_end here. > OK will change accordingly >> >> if (acpi_disabled) >> return -ENODEV; >> >> - if (!id || !handler) >> - return -EINVAL; >> - >> - if (!table_size) >> + if (!id || !handler || !table_size) >> return -EINVAL; > > Please mention this cleanup bit in the changelog, as it is not related to > the other changes. > Ah right, sorry for unnecessary change. Better I will remove it. >> >> if (!table_header) { >> @@ -234,34 +231,21 @@ acpi_parse_entries(char *id, unsigned long table_size, >> return -ENODEV; >> } >> >> - table_end = (unsigned long)table_header + table_header->length; >> + table_end = (void *)table_header + table_header->length; >> >> /* Parse all entries looking for a match. */ >> + entry = (void *)table_header + table_size; >> + entry_end = (void *)entry + entry->length; >> >> - entry = (struct acpi_subtable_header *) >> - ((unsigned long)table_header + table_size); > > entry_end = (unsigned long)table_header + table_size; > entry = (struct acpi_subtable_header *)entry_end; > entry_end += entry->length; > >> - >> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < >> - table_end) { >> + while (entry->length && entry_end <= table_end) { > > We used to return -EINVAL for entry->length == 0 and now we don't. Isn't that > a problem? > I thought the main reason was to break the infinite loop, will retain that as you suggested. >> if (entry->type == entry_id >> && (!max_entries || count < max_entries)) { >> - if (handler(entry, table_end)) >> + if (handler(entry, (unsigned long)entry_end)) >> return -EINVAL; >> - >> 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, entry_id); >> - return -EINVAL; >> - } > > Perhaps just reorder this with the previous if () and write the loop as > Make sense. > while (entry_end <= table_end) { > >> - >> - entry = (struct acpi_subtable_header *) >> - ((unsigned long)entry + entry->length); >> + entry = entry_end; >> + entry_end = (void *)entry + entry->length; > > And this can be > > entry = (struct acpi_subtable_header *)entry_end; > entry_end += entry->length; > OK Anyways I realized that I tested this on top of Al's MADT cleanup series. I think this might break BAD_MADT_ENTRY macro which expects to get table_end rather than entry_end(though latter is correct) I am not sure if that's the case with APIC, but for MADT and BAD_MADT_GICC_ENTRY, entry + sizeof(*entry) > entry_end can be true(as the specification is broken and Al is trying to solve) I will respin the patch as per your suggestion but needs to go only after Al's MADT patches. Regards, Sudeep -- 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 15/09/15 10:31, Sudeep Holla wrote: > > > On 15/09/15 00:41, Rafael J. Wysocki wrote: >> On Monday, September 14, 2015 04:14:46 PM 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. >>> >>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> [...] > I will respin the patch as per your suggestion but needs to go only > after Al's MADT patches. > Alternately we can retain passing table_end to the handlers for now. Once Al's MADT series removes those BAD_GIC_ENTRY macros, there will be no users of that second argument in subtable handlers and it can be completely removed. Let me know your thoughts. Regards, Sudeep -- 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 Tuesday, September 15, 2015 02:35:32 PM Sudeep Holla wrote: > > On 15/09/15 10:31, Sudeep Holla wrote: > > > > > > On 15/09/15 00:41, Rafael J. Wysocki wrote: > >> On Monday, September 14, 2015 04:14:46 PM 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. > >>> > >>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >> > > [...] > > > I will respin the patch as per your suggestion but needs to go only > > after Al's MADT patches. > > > > Alternately we can retain passing table_end to the handlers for now. > > Once Al's MADT series removes those BAD_GIC_ENTRY macros, there will be > no users of that second argument in subtable handlers and it can be > completely removed. Let me know your thoughts. I'm going to apply the Al's series, so that sounds like the way to go. 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 17a6fa01a338..145d4f6a1c54 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -217,16 +217,13 @@ acpi_parse_entries(char *id, unsigned long table_size, int entry_id, unsigned int max_entries) { struct acpi_subtable_header *entry; + void *entry_end, *table_end; int count = 0; - unsigned long table_end; if (acpi_disabled) return -ENODEV; - if (!id || !handler) - return -EINVAL; - - if (!table_size) + if (!id || !handler || !table_size) return -EINVAL; if (!table_header) { @@ -234,34 +231,21 @@ acpi_parse_entries(char *id, unsigned long table_size, return -ENODEV; } - table_end = (unsigned long)table_header + table_header->length; + table_end = (void *)table_header + table_header->length; /* Parse all entries looking for a match. */ + entry = (void *)table_header + table_size; + entry_end = (void *)entry + entry->length; - entry = (struct acpi_subtable_header *) - ((unsigned long)table_header + table_size); - - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < - table_end) { + while (entry->length && entry_end <= table_end) { if (entry->type == entry_id && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + if (handler(entry, (unsigned long)entry_end)) return -EINVAL; - 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, entry_id); - return -EINVAL; - } - - entry = (struct acpi_subtable_header *) - ((unsigned long)entry + entry->length); + entry = entry_end; + entry_end = (void *)entry + 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. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/acpi/tables.c | 34 +++++++++------------------------- 1 file changed, 9 insertions(+), 25 deletions(-)