Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

From: Javier Martinez Canillas
Date: Wed Dec 20 2017 - 13:21:42 EST


On 12/20/2017 07:08 PM, Jason Gunthorpe wrote:
> On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote:
>> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
>> but on the error path the memory is accessed by the .clk_enable handler
>> after this was already unmapped. So only unmap the I/O memory region if
>> it will not be used anymore.
>>
>> Also, the correct thing to do is to cleanup the resources in the inverse
>> order that were acquired to prevent issues like these.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
>>
>> drivers/char/tpm/tpm_tis_core.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index c2227983ed88..3455abbb2035 100644
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>
> Yoiks. This patch is helping but the more I look at this the wronger
> everything looks..
>
> 1) tpm_chip_unregister makes chip->ops == NULL, so this sequence:
>
> static int tpm_tis_plat_remove(struct platform_device *pdev)
> tpm_chip_unregister(chip);
> tpm_tis_remove(chip);
> void tpm_tis_remove(struct tpm_chip *chip)
> if (chip->ops->clk_enable != NULL)
>
> Will oops
>
> 2) tpm_chip_register can also NULL ops in error cases, so this
> sequence can oops:
>
> rc = tpm_chip_register(chip);
> if (rc && is_bsw())
> iounmap(priv->ilb_base_addr);
>
> if (chip->ops->clk_enable != NULL)
> chip->ops->clk_enable(chip, false);
>
> 3) iounmap should not be split between tpm_tis and tpm_tis_core
> Put it at the end of tpm_tis_remove.
>
> 4) This sequence:
>
> + return tpm_chip_register(chip);
> +out_err:
> + tpm_tis_remove(chip);
> + return rc;
>
> Doesn't look right. If tpm_chip_register fails then
> tpm_tis_remove will never be called. This was sort of OK when
> tpm_tis_remove didn't manage any resources, but now that it does
> the above needs fixing too.
>

Right, I only noticed the issue this patch fixes and (wrongly) assumed the
rest was correct.

> The below draft fixes everything except #1. That needs a more thoughtful
> idea..
>

I'll just drop this patch from the series and you can fix all the issues in
the error / driver removal paths. It's not a dependency anyways, I included
it just because noticed the issue while reading the code.

Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat