| Message ID | 20170221211424.GA16091@obsidianresearch.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 21, 2017 at 02:14:24PM -0700, Jason Gunthorpe wrote: > The expectation is that the if the CRB cmd/rsp buffer falls within the > ACPI region that the entire buffer will be within the reason. Otherwise > resource reservation will fail when it crosses regions. > > Work around this BIOS bug by limiting the cmd/rsp buffer to the length > of the declared ACPI region. BIOS vendors should fix this by making > the ACPI and register length declarations consistent. > > Reported-by: davide.guerri@gmail.com > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Thanks for fixing this issue! /Jarkko > --- > drivers/char/tpm/tpm_crb.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index a7c870af916c3d..fac1934eedf0bb 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -243,6 +243,27 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, > return priv->iobase + (new_res.start - io_res->start); > } > > +/* > + * Work around broken BIOSs that return inconsistent values from the ACPI > + * region vs the registers. Trust the ACPI region. Such broken systems > + * probably cannot send large TPM commands since the buffer will be truncated. > + */ > +static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res, > + u64 start, u64 size) > +{ > + if (io_res->start > start || io_res->end < start) > + return size; > + > + if (start + size - 1 <= io_res->end) > + return size; > + > + dev_err(dev, > + FW_BUG "ACPI region does not cover the entire command/response buffer. %pr vs %llx %llx\n", > + io_res, start, size); > + > + return io_res->end - start + 1; > +} > + > static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > struct acpi_table_tpm2 *buf) > { > @@ -278,14 +299,16 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > (u64) ioread32(&priv->cca->cmd_pa_low); > - cmd_size = ioread32(&priv->cca->cmd_size); > + cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa, > + ioread32(&priv->cca->cmd_size)); > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > if (IS_ERR(priv->cmd)) > return PTR_ERR(priv->cmd); > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > rsp_pa = le64_to_cpu(rsp_pa); > - rsp_size = ioread32(&priv->cca->rsp_size); > + rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa, > + ioread32(&priv->cca->rsp_size)); > > if (cmd_pa != rsp_pa) { > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > -- > 2.7.4 > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Feb 21, 2017 at 02:14:24PM -0700, Jason Gunthorpe wrote: > The expectation is that the if the CRB cmd/rsp buffer falls within the > ACPI region that the entire buffer will be within the reason. Otherwise > resource reservation will fail when it crosses regions. > > Work around this BIOS bug by limiting the cmd/rsp buffer to the length > of the declared ACPI region. BIOS vendors should fix this by making > the ACPI and register length declarations consistent. > > Reported-by: davide.guerri@gmail.com > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> I'll have to wait until Davide gives a tested-by. /Jarkko > --- > drivers/char/tpm/tpm_crb.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index a7c870af916c3d..fac1934eedf0bb 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -243,6 +243,27 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, > return priv->iobase + (new_res.start - io_res->start); > } > > +/* > + * Work around broken BIOSs that return inconsistent values from the ACPI > + * region vs the registers. Trust the ACPI region. Such broken systems > + * probably cannot send large TPM commands since the buffer will be truncated. > + */ > +static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res, > + u64 start, u64 size) > +{ > + if (io_res->start > start || io_res->end < start) > + return size; > + > + if (start + size - 1 <= io_res->end) > + return size; > + > + dev_err(dev, > + FW_BUG "ACPI region does not cover the entire command/response buffer. %pr vs %llx %llx\n", > + io_res, start, size); > + > + return io_res->end - start + 1; > +} > + > static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > struct acpi_table_tpm2 *buf) > { > @@ -278,14 +299,16 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > (u64) ioread32(&priv->cca->cmd_pa_low); > - cmd_size = ioread32(&priv->cca->cmd_size); > + cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa, > + ioread32(&priv->cca->cmd_size)); > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > if (IS_ERR(priv->cmd)) > return PTR_ERR(priv->cmd); > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > rsp_pa = le64_to_cpu(rsp_pa); > - rsp_size = ioread32(&priv->cca->rsp_size); > + rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa, > + ioread32(&priv->cca->rsp_size)); > > if (cmd_pa != rsp_pa) { > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > -- > 2.7.4 > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> On 24 Feb 2017, at 1:02 pm, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > On Tue, Feb 21, 2017 at 02:14:24PM -0700, Jason Gunthorpe wrote: >> The expectation is that the if the CRB cmd/rsp buffer falls within the >> ACPI region that the entire buffer will be within the reason. Otherwise >> resource reservation will fail when it crosses regions. >> >> Work around this BIOS bug by limiting the cmd/rsp buffer to the length >> of the declared ACPI region. BIOS vendors should fix this by making >> the ACPI and register length declarations consistent. >> >> Reported-by: davide.guerri@gmail.com >> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Tested-by: Davide Guerri <davide.guerri@gmail.com> Thanks again, D. > > Thanks for fixing this issue! > > /Jarkko > >> --- >> drivers/char/tpm/tpm_crb.c | 27 +++++++++++++++++++++++++-- >> 1 file changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c >> index a7c870af916c3d..fac1934eedf0bb 100644 >> --- a/drivers/char/tpm/tpm_crb.c >> +++ b/drivers/char/tpm/tpm_crb.c >> @@ -243,6 +243,27 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, >> return priv->iobase + (new_res.start - io_res->start); >> } >> >> +/* >> + * Work around broken BIOSs that return inconsistent values from the ACPI >> + * region vs the registers. Trust the ACPI region. Such broken systems >> + * probably cannot send large TPM commands since the buffer will be truncated. >> + */ >> +static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res, >> + u64 start, u64 size) >> +{ >> + if (io_res->start > start || io_res->end < start) >> + return size; >> + >> + if (start + size - 1 <= io_res->end) >> + return size; >> + >> + dev_err(dev, >> + FW_BUG "ACPI region does not cover the entire command/response buffer. %pr vs %llx %llx\n", >> + io_res, start, size); >> + >> + return io_res->end - start + 1; >> +} >> + >> static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, >> struct acpi_table_tpm2 *buf) >> { >> @@ -278,14 +299,16 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, >> >> cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | >> (u64) ioread32(&priv->cca->cmd_pa_low); >> - cmd_size = ioread32(&priv->cca->cmd_size); >> + cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa, >> + ioread32(&priv->cca->cmd_size)); >> priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); >> if (IS_ERR(priv->cmd)) >> return PTR_ERR(priv->cmd); >> >> memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); >> rsp_pa = le64_to_cpu(rsp_pa); >> - rsp_size = ioread32(&priv->cca->rsp_size); >> + rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa, >> + ioread32(&priv->cca->rsp_size)); >> >> if (cmd_pa != rsp_pa) { >> priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); >> -- >> 2.7.4 >> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index a7c870af916c3d..fac1934eedf0bb 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -243,6 +243,27 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, return priv->iobase + (new_res.start - io_res->start); } +/* + * Work around broken BIOSs that return inconsistent values from the ACPI + * region vs the registers. Trust the ACPI region. Such broken systems + * probably cannot send large TPM commands since the buffer will be truncated. + */ +static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res, + u64 start, u64 size) +{ + if (io_res->start > start || io_res->end < start) + return size; + + if (start + size - 1 <= io_res->end) + return size; + + dev_err(dev, + FW_BUG "ACPI region does not cover the entire command/response buffer. %pr vs %llx %llx\n", + io_res, start, size); + + return io_res->end - start + 1; +} + static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, struct acpi_table_tpm2 *buf) { @@ -278,14 +299,16 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | (u64) ioread32(&priv->cca->cmd_pa_low); - cmd_size = ioread32(&priv->cca->cmd_size); + cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa, + ioread32(&priv->cca->cmd_size)); priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); if (IS_ERR(priv->cmd)) return PTR_ERR(priv->cmd); memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); rsp_pa = le64_to_cpu(rsp_pa); - rsp_size = ioread32(&priv->cca->rsp_size); + rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa, + ioread32(&priv->cca->rsp_size)); if (cmd_pa != rsp_pa) { priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
The expectation is that the if the CRB cmd/rsp buffer falls within the ACPI region that the entire buffer will be within the reason. Otherwise resource reservation will fail when it crosses regions. Work around this BIOS bug by limiting the cmd/rsp buffer to the length of the declared ACPI region. BIOS vendors should fix this by making the ACPI and register length declarations consistent. Reported-by: davide.guerri@gmail.com Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/char/tpm/tpm_crb.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)