diff mbox

[1/3] nfit: Account for table size length variation

Message ID e65332cc9dbb7eff02f739aa5ba357d4097b4e6a.1448045168.git.linda.knippers@hpe.com (mailing list archive)
State Superseded
Headers show

Commit Message

Linda Knippers Nov. 21, 2015, 12:05 a.m. UTC
The size of NFIT tables don't necessarily match the size of the
data structures that we use for them.  For example, the NVDIMM
Control Region Structure table is shorter for a device with
no block control windows than for a device with block control windows.
Other tables, such as Flush Hint Address Structure and the Interleave
Structure are variable length by definition.

Account for the size difference when comparing table entries by
using the actual table size from the table header if it's less
than the structure size.  

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

Comments

Verma, Vishal L Nov. 23, 2015, 11:21 p.m. UTC | #1
On Fri, 2015-11-20 at 19:05 -0500, Linda Knippers wrote:
> The size of NFIT tables don't necessarily match the size of the

> data structures that we use for them.  For example, the NVDIMM

> Control Region Structure table is shorter for a device with

> no block control windows than for a device with block control windows.

> Other tables, such as Flush Hint Address Structure and the Interleave

> Structure are variable length by definition.

> 

> Account for the size difference when comparing table entries by

> using the actual table size from the table header if it's less

> than the structure size.  

> 


Agreed about the variable length tables - Flush Hint and Interleave. But
for the others, this makes it possible for a buggy bios implementation
to have a *really* small table - one that doesn't even have enough
fields to work - to pass the add_tables stage where it might have failed
previously.

I feel there may be need for an ACPI clarification for this specifying
whether if certain fields are irrelevant, they can be excluded entirely.

For example, the DCR wording is:
Number of Block Control Windows must match the
corresponding number of Block Data Windows. Fields that
follow this field are valid only if the number of Block Control
Windows is non-zero.

This leads me to believe that those fields should be 'present' but
ignored in the case of zero block windows.

If we make add_tables process only header.length and accept the
shortened table, there is nothing to tell future code that the structure
that piece of memory is casted to is a truncated one.

Thoughts?

	-Vishal
Linda Knippers Nov. 24, 2015, 4:24 p.m. UTC | #2
On 11/23/2015 6:21 PM, Verma, Vishal L wrote:
> On Fri, 2015-11-20 at 19:05 -0500, Linda Knippers wrote:
>> The size of NFIT tables don't necessarily match the size of the
>> data structures that we use for them.  For example, the NVDIMM
>> Control Region Structure table is shorter for a device with
>> no block control windows than for a device with block control windows.
>> Other tables, such as Flush Hint Address Structure and the Interleave
>> Structure are variable length by definition.
>>
>> Account for the size difference when comparing table entries by
>> using the actual table size from the table header if it's less
>> than the structure size.
>>
>
> Agreed about the variable length tables - Flush Hint and Interleave. But
> for the others, this makes it possible for a buggy bios implementation
> to have a *really* small table - one that doesn't even have enough
> fields to work - to pass the add_tables stage where it might have failed
> previously.
>
> I feel there may be need for an ACPI clarification for this specifying
> whether if certain fields are irrelevant, they can be excluded entirely.
>
> For example, the DCR wording is:
> Number of Block Control Windows must match the
> corresponding number of Block Data Windows. Fields that
> follow this field are valid only if the number of Block Control
> Windows is non-zero.
>
> This leads me to believe that those fields should be 'present' but
> ignored in the case of zero block windows.

Actually, the spec is pretty clear in this case.  If you look at the
length definition for that table (5-133) it says:

	Length in bytes for entire structure.
	The length of this structure is either 32 bytes or 80 bytes. The
	length of the structure can be 32 bytes only if the Number of
	Block Control Windows field has a value of 0.

The structure is 80 bytes but it is legal to have a 32-byte table.
We hit a similar problem with the original NFIT code.  We could
explicitly check for a size of 32 but we didn't before.

> If we make add_tables process only header.length and accept the
> shortened table, there is nothing to tell future code that the structure
> that piece of memory is casted to is a truncated one.
>
> Thoughts?

If we want to be more paranoid about buggy FW when we're comparing old
and new tables, we could compare based on the length of the old and new
tables since we have both pieces of information.  That would let you catch
the case where a table size changes during a hotplug event or whenever the
_FIT is processed.  Since you were comparing based on structure size instead
of header length, I didn't change that.

-- ljk
>
> 	-Vishal
>
Dan Williams Nov. 24, 2015, 4:31 p.m. UTC | #3
On Tue, Nov 24, 2015 at 8:24 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
[..]
> If we want to be more paranoid about buggy FW when we're comparing old
> and new tables, we could compare based on the length of the old and new
> tables since we have both pieces of information.  That would let you catch
> the case where a table size changes during a hotplug event or whenever the
> _FIT is processed.  Since you were comparing based on structure size instead
> of header length, I didn't change that.

Comparing that the size didn't change sounds like a good incremental
change to tag on later.
Verma, Vishal L Nov. 24, 2015, 5:47 p.m. UTC | #4
On Tue, 2015-11-24 at 11:24 -0500, Linda Knippers wrote:

> Actually, the spec is pretty clear in this case.  If you look at the

> length definition for that table (5-133) it says:

> 

> 	Length in bytes for entire structure.

> 	The length of this structure is either 32 bytes or 80 bytes. The

> 	length of the structure can be 32 bytes only if the Number of

> 	Block Control Windows field has a value of 0.

> 

> The structure is 80 bytes but it is legal to have a 32-byte table.

> We hit a similar problem with the original NFIT code.  We could

> explicitly check for a size of 32 but we didn't before.


Thanks, I missed that. No objections from me any more :)

> 

> > If we make add_tables process only header.length and accept the

> > shortened table, there is nothing to tell future code that the

> > structure

> > that piece of memory is casted to is a truncated one.

> > 

> > Thoughts?

> 

> If we want to be more paranoid about buggy FW when we're comparing old

> and new tables, we could compare based on the length of the old and

> new

> tables since we have both pieces of information.  That would let you

> catch

> the case where a table size changes during a hotplug event or whenever

> the

> _FIT is processed.  Since you were comparing based on structure size

> instead

> of header length, I didn't change that.


Agreed this can be incremental work.

> 

> -- ljk

> > 

> > 	-Vishal

> > 

>
diff mbox

Patch

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index f7dab53..710092a 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -235,9 +235,13 @@  static bool add_spa(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_spa *nfit_spa;
+	size_t length = sizeof(*spa);
+
+	if (length > spa->header.length)
+		length = spa->header.length;
 
 	list_for_each_entry(nfit_spa, &prev->spas, list) {
-		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0) {
+		if (memcmp(nfit_spa->spa, spa, length) == 0) {
 			list_move_tail(&nfit_spa->list, &acpi_desc->spas);
 			return true;
 		}
@@ -261,9 +265,13 @@  static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_memdev *nfit_memdev;
+	size_t length = sizeof(*memdev);
+
+	if (length > memdev->header.length)
+		length = memdev->header.length;
 
 	list_for_each_entry(nfit_memdev, &prev->memdevs, list)
-		if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0) {
+		if (memcmp(nfit_memdev->memdev, memdev, length) == 0) {
 			list_move_tail(&nfit_memdev->list, &acpi_desc->memdevs);
 			return true;
 		}
@@ -286,9 +294,13 @@  static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_dcr *nfit_dcr;
+	size_t length = sizeof(*dcr);
+
+	if (length > dcr->header.length)
+		length = dcr->header.length;
 
 	list_for_each_entry(nfit_dcr, &prev->dcrs, list)
-		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0) {
+		if (memcmp(nfit_dcr->dcr, dcr, length) == 0) {
 			list_move_tail(&nfit_dcr->list, &acpi_desc->dcrs);
 			return true;
 		}
@@ -310,9 +322,13 @@  static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_bdw *nfit_bdw;
+	size_t length = sizeof(*bdw);
+
+	if (length > bdw->header.length)
+		length = bdw->header.length;
 
 	list_for_each_entry(nfit_bdw, &prev->bdws, list)
-		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0) {
+		if (memcmp(nfit_bdw->bdw, bdw, length) == 0) {
 			list_move_tail(&nfit_bdw->list, &acpi_desc->bdws);
 			return true;
 		}
@@ -334,9 +350,13 @@  static bool add_idt(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_idt *nfit_idt;
+	size_t length = sizeof(*idt);
+
+	if (length > idt->header.length)
+		length = idt->header.length;
 
 	list_for_each_entry(nfit_idt, &prev->idts, list)
-		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0) {
+		if (memcmp(nfit_idt->idt, idt, length) == 0) {
 			list_move_tail(&nfit_idt->list, &acpi_desc->idts);
 			return true;
 		}
@@ -358,9 +378,13 @@  static bool add_flush(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_flush *nfit_flush;
+	size_t length = sizeof(*flush);
+
+	if (length > flush->header.length)
+		length = flush->header.length;
 
 	list_for_each_entry(nfit_flush, &prev->flushes, list)
-		if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0) {
+		if (memcmp(nfit_flush->flush, flush, length) == 0) {
 			list_move_tail(&nfit_flush->list, &acpi_desc->flushes);
 			return true;
 		}