| Message ID | e65332cc9dbb7eff02f739aa5ba357d4097b4e6a.1448045168.git.linda.knippers@hpe.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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 >
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.
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 --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; }
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(-)