diff mbox

[1/3] nfit: in acpi_nfit_init, break on a 0-length table

Message ID 1444254577-23744-2-git-send-email-vishal.l.verma@intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Verma, Vishal L Oct. 7, 2015, 9:49 p.m. UTC
If acpi_nfit_init is called (such as from nfit_test), with an nfit table
that has more memory allocated than it needs (and a similarly large
'size' field, add_tables would happily keep adding null SPA Range tables
filling up all available memory.

Make it friendlier by breaking out if a 0-length header is found in any
of the tables.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <linux-acpi@vger.kernel.org>
Cc: <linux-nvdimm@lists.01.org>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jeff Moyer Oct. 9, 2015, 5:23 p.m. UTC | #1
Vishal Verma <vishal.l.verma@intel.com> writes:

> If acpi_nfit_init is called (such as from nfit_test), with an nfit table
> that has more memory allocated than it needs (and a similarly large
> 'size' field, add_tables would happily keep adding null SPA Range tables
> filling up all available memory.
>
> Make it friendlier by breaking out if a 0-length header is found in any
> of the tables.

Shouldn't that at least spew a warning?  Or does the spec allow for
zero-length tables?

-Jeff

>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <linux-acpi@vger.kernel.org>
> Cc: <linux-nvdimm@lists.01.org>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/acpi/nfit.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index c1b8d03..ed599d1 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -335,6 +335,9 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
>  		return NULL;
>  
>  	hdr = table;
> +	if (!hdr->length)
> +		return NULL;
> +
>  	switch (hdr->type) {
>  	case ACPI_NFIT_TYPE_SYSTEM_ADDRESS:
>  		if (!add_spa(acpi_desc, table))
--
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
Dan Williams Oct. 9, 2015, 5:27 p.m. UTC | #2
On Fri, Oct 9, 2015 at 10:23 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Vishal Verma <vishal.l.verma@intel.com> writes:
>
>> If acpi_nfit_init is called (such as from nfit_test), with an nfit table
>> that has more memory allocated than it needs (and a similarly large
>> 'size' field, add_tables would happily keep adding null SPA Range tables
>> filling up all available memory.
>>
>> Make it friendlier by breaking out if a 0-length header is found in any
>> of the tables.
>
> Shouldn't that at least spew a warning?  Or does the spec allow for
> zero-length tables?
>

The spec allows for zero length tables but the firmware implementation
should be self consistent and not report a total NFIT size that is
greater than the sum of the size of the sub-structures.  A warning /
nudge to firmware developers to fix their stuff seems appropriate.
--
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
Verma, Vishal L Oct. 9, 2015, 5:51 p.m. UTC | #3
On Fri, 2015-10-09 at 10:27 -0700, Dan Williams wrote:
> On Fri, Oct 9, 2015 at 10:23 AM, Jeff Moyer <jmoyer@redhat.com>

> wrote:

> > Vishal Verma <vishal.l.verma@intel.com> writes:

> > 

> > > If acpi_nfit_init is called (such as from nfit_test), with an

> > > nfit table

> > > that has more memory allocated than it needs (and a similarly

> > > large

> > > 'size' field, add_tables would happily keep adding null SPA Range

> > > tables

> > > filling up all available memory.

> > > 

> > > Make it friendlier by breaking out if a 0-length header is found

> > > in any

> > > of the tables.

> > 

> > Shouldn't that at least spew a warning?  Or does the spec allow for

> > zero-length tables?

> > 

> 

> The spec allows for zero length tables but the firmware

> implementation

> should be self consistent and not report a total NFIT size that is

> greater than the sum of the size of the sub-structures.  A warning /

> nudge to firmware developers to fix their stuff seems appropriate.


Agreed, I'll add a warning. The caveat is, the following (3/3) patch to
nfit test will trigger the warning for the first, non-hotplug pass, as
I just calculate the size and allocate the buffer once, and the first
pass will have the regular nfit, and the second pass will have new
hotplug entries.. Is that OK or should I look at reworking that?
diff mbox

Patch

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..ed599d1 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -335,6 +335,9 @@  static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
 		return NULL;
 
 	hdr = table;
+	if (!hdr->length)
+		return NULL;
+
 	switch (hdr->type) {
 	case ACPI_NFIT_TYPE_SYSTEM_ADDRESS:
 		if (!add_spa(acpi_desc, table))