On Huang Ying''s machine:
erst_tab->header_length == sizeof(struct acpi_table_einj)
but Yinghai reported that on his machine,
erst_tab->header_length == sizeof(struct acpi_table_einj) -
sizeof(struct acpi_table_header)
To make erst table size checking code works on all systems, both
testing are treated as PASS.
Same situation applies to einj_tab->header_length, so corresponding
table size checking is changed in similar way too.
Originally-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Huang Ying <ying.huang@intel.com>
- use switch() for better readability
- add comment explaining why a formally invalid size it also being
accepted
- check erst_tab->header.length before even looking at
erst_tab->header_length
- prefer sizeof(*erst_tab) over sizeof(struct acpi_table_erst)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -715,12 +715,23 @@ int erst_clear(u64 record_id)
static int __init erst_check_table(struct acpi_table_erst *erst_tab)
{
- if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+ if (erst_tab->header.length < sizeof(*erst_tab))
return -EINVAL;
- if (erst_tab->header.length < sizeof(struct acpi_table_erst))
+
+ switch (erst_tab->header_length) {
+ case sizeof(*erst_tab) - sizeof(erst_tab->header):
+ /*
+ * While invalid per specification, there are (early?) systems
+ * indicating the full header size here, so accept that value too.
+ */
+ case sizeof(*erst_tab):
+ break;
+ default:
return -EINVAL;
+ }
+
if (erst_tab->entries !- (erst_tab->header.length - sizeof(struct
acpi_table_erst)) /
+ (erst_tab->header.length - sizeof(*erst_tab)) /
sizeof(struct acpi_erst_entry))
return -EINVAL;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
On 16/10/2012 15:46, "Jan Beulich" <JBeulich@suse.com> wrote:> On Huang Ying''s machine: > > erst_tab->header_length == sizeof(struct acpi_table_einj) > > but Yinghai reported that on his machine, > > erst_tab->header_length == sizeof(struct acpi_table_einj) - > sizeof(struct acpi_table_header) > > To make erst table size checking code works on all systems, both > testing are treated as PASS. > > Same situation applies to einj_tab->header_length, so corresponding > table size checking is changed in similar way too. > > Originally-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Huang Ying <ying.huang@intel.com> > > - use switch() for better readability > - add comment explaining why a formally invalid size it also being > accepted > - check erst_tab->header.length before even looking at > erst_tab->header_length > - prefer sizeof(*erst_tab) over sizeof(struct acpi_table_erst) > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Acked-by: Keir Fraser <keir@xen.org>> --- a/xen/drivers/acpi/apei/erst.c > +++ b/xen/drivers/acpi/apei/erst.c > @@ -715,12 +715,23 @@ int erst_clear(u64 record_id) > > static int __init erst_check_table(struct acpi_table_erst *erst_tab) > { > - if (erst_tab->header_length != sizeof(struct acpi_table_erst)) > + if (erst_tab->header.length < sizeof(*erst_tab)) > return -EINVAL; > - if (erst_tab->header.length < sizeof(struct acpi_table_erst)) > + > + switch (erst_tab->header_length) { > + case sizeof(*erst_tab) - sizeof(erst_tab->header): > + /* > + * While invalid per specification, there are (early?) systems > + * indicating the full header size here, so accept that value too. > + */ > + case sizeof(*erst_tab): > + break; > + default: > return -EINVAL; > + } > + > if (erst_tab->entries !> - (erst_tab->header.length - sizeof(struct acpi_table_erst)) / > + (erst_tab->header.length - sizeof(*erst_tab)) / > sizeof(struct acpi_erst_entry)) > return -EINVAL; > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Tue, 2012-10-16 at 15:46 +0100, Jan Beulich wrote:> On Huang Ying''s machine: > > erst_tab->header_length == sizeof(struct acpi_table_einj)~~~~ ~~~~ Typo? Best Regards, Huang Ying> but Yinghai reported that on his machine, > > erst_tab->header_length == sizeof(struct acpi_table_einj) - > sizeof(struct acpi_table_header) > > To make erst table size checking code works on all systems, both > testing are treated as PASS. > > Same situation applies to einj_tab->header_length, so corresponding > table size checking is changed in similar way too. > > Originally-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Huang Ying <ying.huang@intel.com> > > - use switch() for better readability > - add comment explaining why a formally invalid size it also being > accepted > - check erst_tab->header.length before even looking at > erst_tab->header_length > - prefer sizeof(*erst_tab) over sizeof(struct acpi_table_erst) > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/drivers/acpi/apei/erst.c > +++ b/xen/drivers/acpi/apei/erst.c > @@ -715,12 +715,23 @@ int erst_clear(u64 record_id) > > static int __init erst_check_table(struct acpi_table_erst *erst_tab) > { > - if (erst_tab->header_length != sizeof(struct acpi_table_erst)) > + if (erst_tab->header.length < sizeof(*erst_tab)) > return -EINVAL; > - if (erst_tab->header.length < sizeof(struct acpi_table_erst)) > + > + switch (erst_tab->header_length) { > + case sizeof(*erst_tab) - sizeof(erst_tab->header): > + /* > + * While invalid per specification, there are (early?) systems > + * indicating the full header size here, so accept that value too. > + */ > + case sizeof(*erst_tab): > + break; > + default: > return -EINVAL; > + } > + > if (erst_tab->entries !> - (erst_tab->header.length - sizeof(struct acpi_table_erst)) / > + (erst_tab->header.length - sizeof(*erst_tab)) / > sizeof(struct acpi_erst_entry)) > return -EINVAL; > > > >
>>> On 17.10.12 at 08:49, Huang Ying <ying.huang@intel.com> wrote: > On Tue, 2012-10-16 at 15:46 +0100, Jan Beulich wrote: >> On Huang Ying''s machine: >> >> erst_tab->header_length == sizeof(struct acpi_table_einj) > ~~~~ ~~~~ > > Typo?Your typo: I copied the Linux commit message verbatim, to make it possible to match the two commits. The adjustments done in the actual patch eliminate the copy-n-paste mistake corrected by Linux commit 7ed28f2ed43ece424ff2fa4dedac7928bb37a23a. Jan>> but Yinghai reported that on his machine, >> >> erst_tab->header_length == sizeof(struct acpi_table_einj) - >> sizeof(struct acpi_table_header) >> >> To make erst table size checking code works on all systems, both >> testing are treated as PASS. >> >> Same situation applies to einj_tab->header_length, so corresponding >> table size checking is changed in similar way too. >> >> Originally-by: Yinghai Lu <yinghai@kernel.org> >> Signed-off-by: Huang Ying <ying.huang@intel.com> >> >> - use switch() for better readability >> - add comment explaining why a formally invalid size it also being >> accepted >> - check erst_tab->header.length before even looking at >> erst_tab->header_length >> - prefer sizeof(*erst_tab) over sizeof(struct acpi_table_erst) >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/drivers/acpi/apei/erst.c >> +++ b/xen/drivers/acpi/apei/erst.c >> @@ -715,12 +715,23 @@ int erst_clear(u64 record_id) >> >> static int __init erst_check_table(struct acpi_table_erst *erst_tab) >> { >> - if (erst_tab->header_length != sizeof(struct acpi_table_erst)) >> + if (erst_tab->header.length < sizeof(*erst_tab)) >> return -EINVAL; >> - if (erst_tab->header.length < sizeof(struct acpi_table_erst)) >> + >> + switch (erst_tab->header_length) { >> + case sizeof(*erst_tab) - sizeof(erst_tab->header): >> + /* >> + * While invalid per specification, there are (early?) systems >> + * indicating the full header size here, so accept that value too. >> + */ >> + case sizeof(*erst_tab): >> + break; >> + default: >> return -EINVAL; >> + } >> + >> if (erst_tab->entries !>> - (erst_tab->header.length - sizeof(struct acpi_table_erst)) / >> + (erst_tab->header.length - sizeof(*erst_tab)) / >> sizeof(struct acpi_erst_entry)) >> return -EINVAL; >> >> >> >>
On Wed, 2012-10-17 at 07:56 +0100, Jan Beulich wrote:> >>> On 17.10.12 at 08:49, Huang Ying <ying.huang@intel.com> wrote: > > On Tue, 2012-10-16 at 15:46 +0100, Jan Beulich wrote: > >> On Huang Ying''s machine: > >> > >> erst_tab->header_length == sizeof(struct acpi_table_einj) > > ~~~~ ~~~~ > > > > Typo? > > Your typo: I copied the Linux commit message verbatim, to make > it possible to match the two commits. The adjustments done in > the actual patch eliminate the copy-n-paste mistake corrected by > Linux commit 7ed28f2ed43ece424ff2fa4dedac7928bb37a23a.Sorry, my bad. Best Regards, Huang Ying> > >> but Yinghai reported that on his machine, > >> > >> erst_tab->header_length == sizeof(struct acpi_table_einj) - > >> sizeof(struct acpi_table_header) > >> > >> To make erst table size checking code works on all systems, both > >> testing are treated as PASS. > >> > >> Same situation applies to einj_tab->header_length, so corresponding > >> table size checking is changed in similar way too. > >> > >> Originally-by: Yinghai Lu <yinghai@kernel.org> > >> Signed-off-by: Huang Ying <ying.huang@intel.com> > >> > >> - use switch() for better readability > >> - add comment explaining why a formally invalid size it also being > >> accepted > >> - check erst_tab->header.length before even looking at > >> erst_tab->header_length > >> - prefer sizeof(*erst_tab) over sizeof(struct acpi_table_erst) > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> > >> --- a/xen/drivers/acpi/apei/erst.c > >> +++ b/xen/drivers/acpi/apei/erst.c > >> @@ -715,12 +715,23 @@ int erst_clear(u64 record_id) > >> > >> static int __init erst_check_table(struct acpi_table_erst *erst_tab) > >> { > >> - if (erst_tab->header_length != sizeof(struct acpi_table_erst)) > >> + if (erst_tab->header.length < sizeof(*erst_tab)) > >> return -EINVAL; > >> - if (erst_tab->header.length < sizeof(struct acpi_table_erst)) > >> + > >> + switch (erst_tab->header_length) { > >> + case sizeof(*erst_tab) - sizeof(erst_tab->header): > >> + /* > >> + * While invalid per specification, there are (early?) systems > >> + * indicating the full header size here, so accept that value too. > >> + */ > >> + case sizeof(*erst_tab): > >> + break; > >> + default: > >> return -EINVAL; > >> + } > >> + > >> if (erst_tab->entries !> >> - (erst_tab->header.length - sizeof(struct acpi_table_erst)) / > >> + (erst_tab->header.length - sizeof(*erst_tab)) / > >> sizeof(struct acpi_erst_entry)) > >> return -EINVAL; > >> > >> > >> > >> > > >