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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f9da193f-e1b4-c126-d272-b98bafda3b99@redhat.com>
Date:   Wed, 20 Dec 2017 19:21:31 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     linux-kernel@...r.kernel.org, James Ettle <james@...le.org.uk>,
        Hans de Goede <hdegoede@...hat.com>,
        Azhar Shaikh <azhar.shaikh@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Peter Huewe <peterhuewe@....de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-integrity@...r.kernel.org
Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
 memory region

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@...hat.com>
>>
>>  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