On Mon, Aug 26, 2019 at 04:44:00PM +0900, Seunghun Han wrote:
> I'm Seunghun Han and work at the Affiliated Institute of ETRI. I found
You can drop the first sentence from the commit message. The SoB below
is sufficient.
> a bug related to improper buffer size calculation in crb_fixup_cmd_size
> function.
The purpose is to cap to the ACPI region when we partially overlap to
workaround BIOS's reporting corrupted ACPI tables so that we don't get
failure from devm_ioremap().
The only funky thing in that function is that it lets through a buffer
that is fully outside the ACPI region. There actually exists hardware
with this configuration.
> When the TPM CRB regions are two or more, the crb_map_io function calls
> crb_fixup_cmd_size twice to calculate command buffer size and response
> buffer size. The purpose of crb_fixup_cmd_size function is to trust
> the ACPI region information.
This is not true. The driver deals with only one ACPI region ATM.
> However, the function compares only io_res argument with start and size
> arguments. It means the io_res argument is one of command buffer and
> response buffer regions. It also means the other region is not calculated
> correctly by the function because io_res argument doesn't cover all TPM
> CRB regions.
The driver gets command and response buffer metrics from the TPM2 ACPI
table, not from the ACPI region.
> To fix this bug, I change crb_check_resource function for storing all TPB
> CRB regions to a list and use the list to calculate command buffer size
> and response buffer size correctly.
This cannot be categorized as a bug. It is simply as new type of hardware.
Can you explain in detail what type of hardware are you using?
> ---
> drivers/char/tpm/tpm_crb.c | 50 ++++++++++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d7f3..b0e94e02e5eb 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -442,6 +442,9 @@ static int crb_check_resource(struct acpi_resource *ares,
> void *data)
> acpi_dev_resource_address_space(ares, &win)) {
> *io_res = *res;
> io_res->name = NULL;
> +
> + /* Add this TPM CRB resource to the list */
> + return 0;
> }
>
> return 1;
> @@ -471,20 +474,30 @@ static void __iomem *crb_map_res(struct device *dev,
> struct crb_priv *priv,
> * 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,
> +static u64 crb_fixup_cmd_size(struct device *dev, struct list_head
> *resources,
> u64 start, u64 size)
With a quick spin w/o knowing the details of the hardware I'm dealing
with it you should probably reduce the fixup function as
static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
u64 start, u64 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;
}
Then call this inside the loop.
Looking at your change it does not make much sense to me.
There is a weird asymmetry in it:
1. The code loops through all found ACPI regions when looking for
intersections with the command and response buffers.
2. The devm_ioremap() is done only to the last seen ACPI region. Why the
multiple regions matter for fixup's but not in this case?
/Jarkko
