2018-05-05 19:55:55

by Jerry Snitselaar

[permalink] [raw]
Subject: [PATCH] tpm_tis: verify locality released before returning from release_locality

For certain tpm chips releasing locality can take long enough that a
subsequent call to request_locality will see the locality as being
active when the access register is read in check_locality. So check
that the locality has been released before returning from
release_locality.

Cc: Jarkko Sakkinen <[email protected]>
Cc: Peter Huewe <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Reported-by: Laurent Bigonville <[email protected]>
Signed-off-by: Jerry Snitselaar <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 47 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 5a1f47b43947..d547cd309dbd 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -143,13 +143,58 @@ static bool check_locality(struct tpm_chip *chip, int l)
return false;
}

+static bool locality_inactive(struct tpm_chip *chip, int l)
+{
+ struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+ int rc;
+ u8 access;
+
+ rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
+ if (rc < 0)
+ return false;
+
+ if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
+ == TPM_ACCESS_VALID)
+ return true;
+
+ return false;
+}
+
static int release_locality(struct tpm_chip *chip, int l)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+ unsigned long stop, timeout;
+ long rc;

tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);

- return 0;
+ stop = jiffies + chip->timeout_a;
+
+ if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+again:
+ timeout = stop - jiffies;
+ if ((long)timeout <= 0)
+ return -1;
+
+ rc = wait_event_interruptible_timeout(priv->int_queue,
+ (locality_inactive(chip, l)),
+ timeout);
+
+ if (rc > 0)
+ return 0;
+
+ if (rc == -ERESTARTSYS && freezing(current)) {
+ clear_thread_flag(TIF_SIGPENDING);
+ goto again;
+ }
+ } else {
+ do {
+ if (locality_inactive(chip, l))
+ return 0;
+ tpm_msleep(TPM_TIMEOUT);
+ } while (time_before(jiffies, stop));
+ }
+ return -1;
}

static int request_locality(struct tpm_chip *chip, int l)
--
2.15.0



2018-05-05 20:03:40

by Jerry Snitselaar

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

On Sat May 05 18, Jerry Snitselaar wrote:
>For certain tpm chips releasing locality can take long enough that a
>subsequent call to request_locality will see the locality as being
>active when the access register is read in check_locality. So check
>that the locality has been released before returning from
>release_locality.
>
>Cc: Jarkko Sakkinen <[email protected]>
>Cc: Peter Huewe <[email protected]>
>Cc: Jason Gunthorpe <[email protected]>
>Reported-by: Laurent Bigonville <[email protected]>
>Signed-off-by: Jerry Snitselaar <[email protected]>
>---
> drivers/char/tpm/tpm_tis_core.c | 47 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 46 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>index 5a1f47b43947..d547cd309dbd 100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -143,13 +143,58 @@ static bool check_locality(struct tpm_chip *chip, int l)
> return false;
> }
>
>+static bool locality_inactive(struct tpm_chip *chip, int l)
>+{
>+ struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>+ int rc;
>+ u8 access;
>+
>+ rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
>+ if (rc < 0)
>+ return false;
>+
>+ if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
>+ == TPM_ACCESS_VALID)
>+ return true;
>+
>+ return false;
>+}
>+
> static int release_locality(struct tpm_chip *chip, int l)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>+ unsigned long stop, timeout;
>+ long rc;
>
> tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>
>- return 0;
>+ stop = jiffies + chip->timeout_a;
>+
>+ if (chip->flags & TPM_CHIP_FLAG_IRQ) {
>+again:
>+ timeout = stop - jiffies;
>+ if ((long)timeout <= 0)
>+ return -1;
>+
>+ rc = wait_event_interruptible_timeout(priv->int_queue,
>+ (locality_inactive(chip, l)),
>+ timeout);
>+
>+ if (rc > 0)
>+ return 0;
>+
>+ if (rc == -ERESTARTSYS && freezing(current)) {
>+ clear_thread_flag(TIF_SIGPENDING);
>+ goto again;
>+ }
>+ } else {
>+ do {
>+ if (locality_inactive(chip, l))
>+ return 0;
>+ tpm_msleep(TPM_TIMEOUT);
>+ } while (time_before(jiffies, stop));
>+ }
>+ return -1;
> }
>
> static int request_locality(struct tpm_chip *chip, int l)
>--
>2.15.0
>

Laurent,

Can you try this patch with your system since it is the one
that has exhibited the problem so far. I've tested on a
tpm2.0 and tpm1.2 system here.

Regards,
Jerry

2018-05-10 11:29:27

by Laurent Bigonville

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

Le 05/05/18 à 22:03, Jerry Snitselaar a écrit :
> On Sat May 05 18, Jerry Snitselaar wrote:
>> [...]
>>
>
> Laurent,

Hello Jerry,

> Can you try this patch with your system since it is the one
> that has exhibited the problem so far. I've tested on a
> tpm2.0 and tpm1.2 system here.

I just tested the patch and the driver is loading fine again and the
device in /dev is present again, so it seems to work.

But for some reason the tpm is again locked (for no visible reason) due
to "dictionary attack" so I cannot test further ATM :/

Regards,

Laurent

>
> Regards,
> Jerry


2018-05-11 10:21:34

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

On Thu, May 10, 2018 at 01:21:39PM +0200, Laurent Bigonville wrote:
> Le 05/05/18 ? 22:03, Jerry Snitselaar a ?crit?:
> > On Sat May 05 18, Jerry Snitselaar wrote:
> > > [...]
> > >
> >
> > Laurent,
>
> Hello Jerry,
>
> > Can you try this patch with your system since it is the one
> > that has exhibited the problem so far. I've tested on a
> > tpm2.0 and tpm1.2 system here.
>
> I just tested the patch and the driver is loading fine again and the device
> in /dev is present again, so it seems to work.
>
> But for some reason the tpm is again locked (for no visible reason) due to
> "dictionary attack" so I cannot test further ATM :/
>
> Regards,
>
> Laurent
>
> >
> > Regards,
> > Jerry
>

Can you response with the tested-by tag as soon as you can? Before we
have tested-by, I cannot land the fix.

/Jarkko

2018-05-11 19:04:30

by Laurent Bigonville

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

Le 05/05/18 à 22:03, Jerry Snitselaar a écrit :
> On Sat May 05 18, Jerry Snitselaar wrote:
>> For certain tpm chips releasing locality can take long enough that a
>> subsequent call to request_locality will see the locality as being
>> active when the access register is read in check_locality. So check
>> that the locality has been released before returning from
>> release_locality.
>>
>> Cc: Jarkko Sakkinen <[email protected]>
>> Cc: Peter Huewe <[email protected]>
>> Cc: Jason Gunthorpe <[email protected]>
>> Reported-by: Laurent Bigonville <[email protected]>
>> Signed-off-by: Jerry Snitselaar <[email protected]>
Tested-by: Laurent Bigonville <[email protected]>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 47
>> ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 46 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>> b/drivers/char/tpm/tpm_tis_core.c
>> index 5a1f47b43947..d547cd309dbd 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -143,13 +143,58 @@ static bool check_locality(struct tpm_chip
>> *chip, int l)
>>     return false;
>> }
>>
>> +static bool locality_inactive(struct tpm_chip *chip, int l)
>> +{
>> +    struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> +    int rc;
>> +    u8 access;
>> +
>> +    rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
>> +    if (rc < 0)
>> +        return false;
>> +
>> +    if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
>> +        == TPM_ACCESS_VALID)
>> +        return true;
>> +
>> +    return false;
>> +}
>> +
>> static int release_locality(struct tpm_chip *chip, int l)
>> {
>>     struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> +    unsigned long stop, timeout;
>> +    long rc;
>>
>>     tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>>
>> -    return 0;
>> +    stop = jiffies + chip->timeout_a;
>> +
>> +    if (chip->flags & TPM_CHIP_FLAG_IRQ) {
>> +again:
>> +        timeout = stop - jiffies;
>> +        if ((long)timeout <= 0)
>> +            return -1;
>> +
>> +        rc = wait_event_interruptible_timeout(priv->int_queue,
>> +                              (locality_inactive(chip, l)),
>> +                              timeout);
>> +
>> +        if (rc > 0)
>> +            return 0;
>> +
>> +        if (rc == -ERESTARTSYS && freezing(current)) {
>> +            clear_thread_flag(TIF_SIGPENDING);
>> +            goto again;
>> +        }
>> +    } else {
>> +        do {
>> +            if (locality_inactive(chip, l))
>> +                return 0;
>> +            tpm_msleep(TPM_TIMEOUT);
>> +        } while (time_before(jiffies, stop));
>> +    }
>> +    return -1;
>> }
>>
>> static int request_locality(struct tpm_chip *chip, int l)
>> --
>> 2.15.0
>>
>
> Laurent,
>
> Can you try this patch with your system since it is the one
> that has exhibited the problem so far. I've tested on a
> tpm2.0 and tpm1.2 system here.
>
> Regards,
> Jerry


2018-05-14 10:27:52

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

On Sat, May 05, 2018 at 12:54:53PM -0700, Jerry Snitselaar wrote:
> For certain tpm chips releasing locality can take long enough that a
> subsequent call to request_locality will see the locality as being
> active when the access register is read in check_locality. So check
> that the locality has been released before returning from
> release_locality.
>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Peter Huewe <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Reported-by: Laurent Bigonville <[email protected]>
> Signed-off-by: Jerry Snitselaar <[email protected]>

Reviewed-by: Jarkko Sakkinen <ja[email protected]>

/Jarkko

2018-05-28 08:47:03

by Laurent Bigonville

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

Hello,

Top posting, sorry.

I don't know if I did it well to include the "Tested-by" tag because I
don't see that the patch has landed in linus branch already.

And as far as I understand, this will not be in the upcoming 4.17
release as we are already late in the cycle?

Kind regards,

Laurent Bigonville


Le 11/05/18 à 21:02, Laurent Bigonville a écrit :
> Le 05/05/18 à 22:03, Jerry Snitselaar a écrit :
>> On Sat May 05 18, Jerry Snitselaar wrote:
>>> For certain tpm chips releasing locality can take long enough that a
>>> subsequent call to request_locality will see the locality as being
>>> active when the access register is read in check_locality. So check
>>> that the locality has been released before returning from
>>> release_locality.
>>>
>>> Cc: Jarkko Sakkinen <[email protected]>
>>> Cc: Peter Huewe <[email protected]>
>>> Cc: Jason Gunthorpe <[email protected]>
>>> Reported-by: Laurent Bigonville <[email protected]>
>>> Signed-off-by: Jerry Snitselaar <[email protected]>
> Tested-by: Laurent Bigonville <[email protected]>
>>> ---
>>> drivers/char/tpm/tpm_tis_core.c | 47
>>> ++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>>> b/drivers/char/tpm/tpm_tis_core.c
>>> index 5a1f47b43947..d547cd309dbd 100644
>>> --- a/drivers/char/tpm/tpm_tis_core.c
>>> +++ b/drivers/char/tpm/tpm_tis_core.c
>>> @@ -143,13 +143,58 @@ static bool check_locality(struct tpm_chip
>>> *chip, int l)
>>>     return false;
>>> }
>>>
>>> +static bool locality_inactive(struct tpm_chip *chip, int l)
>>> +{
>>> +    struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>> +    int rc;
>>> +    u8 access;
>>> +
>>> +    rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
>>> +    if (rc < 0)
>>> +        return false;
>>> +
>>> +    if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
>>> +        == TPM_ACCESS_VALID)
>>> +        return true;
>>> +
>>> +    return false;
>>> +}
>>> +
>>> static int release_locality(struct tpm_chip *chip, int l)
>>> {
>>>     struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>> +    unsigned long stop, timeout;
>>> +    long rc;
>>>
>>>     tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>>>
>>> -    return 0;
>>> +    stop = jiffies + chip->timeout_a;
>>> +
>>> +    if (chip->flags & TPM_CHIP_FLAG_IRQ) {
>>> +again:
>>> +        timeout = stop - jiffies;
>>> +        if ((long)timeout <= 0)
>>> +            return -1;
>>> +
>>> +        rc = wait_event_interruptible_timeout(priv->int_queue,
>>> +                              (locality_inactive(chip, l)),
>>> +                              timeout);
>>> +
>>> +        if (rc > 0)
>>> +            return 0;
>>> +
>>> +        if (rc == -ERESTARTSYS && freezing(current)) {
>>> +            clear_thread_flag(TIF_SIGPENDING);
>>> +            goto again;
>>> +        }
>>> +    } else {
>>> +        do {
>>> +            if (locality_inactive(chip, l))
>>> +                return 0;
>>> +            tpm_msleep(TPM_TIMEOUT);
>>> +        } while (time_before(jiffies, stop));
>>> +    }
>>> +    return -1;
>>> }
>>>
>>> static int request_locality(struct tpm_chip *chip, int l)
>>> --
>>> 2.15.0
>>>
>>
>> Laurent,
>>
>> Can you try this patch with your system since it is the one
>> that has exhibited the problem so far. I've tested on a
>> tpm2.0 and tpm1.2 system here.
>>
>> Regards,
>> Jerry
>


2018-05-28 09:03:06

by Jerry Snitselaar

[permalink] [raw]
Subject: Re: [PATCH] tpm_tis: verify locality released before returning from release_locality

On Mon May 28 18, Laurent Bigonville wrote:
>Hello,
>
>Top posting, sorry.
>
>I don't know if I did it well to include the "Tested-by" tag because I
>don't see that the patch has landed in linus branch already.
>
>And as far as I understand, this will not be in the upcoming 4.17
>release as we are already late in the cycle?
>
>Kind regards,
>
>Laurent Bigonville
>

It should go into his branch during the merge window for 4.18.

>
>Le 11/05/18 ? 21:02, Laurent Bigonville a ?crit?:
>>Le 05/05/18 ? 22:03, Jerry Snitselaar a ?crit?:
>>>On Sat May 05 18, Jerry Snitselaar wrote:
>>>>For certain tpm chips releasing locality can take long enough that a
>>>>subsequent call to request_locality will see the locality as being
>>>>active when the access register is read in check_locality. So check
>>>>that the locality has been released before returning from
>>>>release_locality.
>>>>
>>>>Cc: Jarkko Sakkinen <[email protected]>
>>>>Cc: Peter Huewe <[email protected]>
>>>>Cc: Jason Gunthorpe <[email protected]>
>>>>Reported-by: Laurent Bigonville <[email protected]>
>>>>Signed-off-by: Jerry Snitselaar <[email protected]>
>>Tested-by: Laurent Bigonville <[email protected]>
>>>>---
>>>>drivers/char/tpm/tpm_tis_core.c | 47
>>>>++++++++++++++++++++++++++++++++++++++++-
>>>>1 file changed, 46 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/drivers/char/tpm/tpm_tis_core.c
>>>>b/drivers/char/tpm/tpm_tis_core.c
>>>>index 5a1f47b43947..d547cd309dbd 100644
>>>>--- a/drivers/char/tpm/tpm_tis_core.c
>>>>+++ b/drivers/char/tpm/tpm_tis_core.c
>>>>@@ -143,13 +143,58 @@ static bool check_locality(struct tpm_chip
>>>>*chip, int l)
>>>>????return false;
>>>>}
>>>>
>>>>+static bool locality_inactive(struct tpm_chip *chip, int l)
>>>>+{
>>>>+??? struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>>+??? int rc;
>>>>+??? u8 access;
>>>>+
>>>>+??? rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
>>>>+??? if (rc < 0)
>>>>+??????? return false;
>>>>+
>>>>+??? if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
>>>>+??????? == TPM_ACCESS_VALID)
>>>>+??????? return true;
>>>>+
>>>>+??? return false;
>>>>+}
>>>>+
>>>>static int release_locality(struct tpm_chip *chip, int l)
>>>>{
>>>>????struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>>+??? unsigned long stop, timeout;
>>>>+??? long rc;
>>>>
>>>>????tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>>>>
>>>>-??? return 0;
>>>>+??? stop = jiffies + chip->timeout_a;
>>>>+
>>>>+??? if (chip->flags & TPM_CHIP_FLAG_IRQ) {
>>>>+again:
>>>>+??????? timeout = stop - jiffies;
>>>>+??????? if ((long)timeout <= 0)
>>>>+??????????? return -1;
>>>>+
>>>>+??????? rc = wait_event_interruptible_timeout(priv->int_queue,
>>>>+????????????????????????????? (locality_inactive(chip, l)),
>>>>+????????????????????????????? timeout);
>>>>+
>>>>+??????? if (rc > 0)
>>>>+??????????? return 0;
>>>>+
>>>>+??????? if (rc == -ERESTARTSYS && freezing(current)) {
>>>>+??????????? clear_thread_flag(TIF_SIGPENDING);
>>>>+??????????? goto again;
>>>>+??????? }
>>>>+??? } else {
>>>>+??????? do {
>>>>+??????????? if (locality_inactive(chip, l))
>>>>+??????????????? return 0;
>>>>+??????????? tpm_msleep(TPM_TIMEOUT);
>>>>+??????? } while (time_before(jiffies, stop));
>>>>+??? }
>>>>+??? return -1;
>>>>}
>>>>
>>>>static int request_locality(struct tpm_chip *chip, int l)
>>>>--
>>>>2.15.0
>>>>
>>>
>>>Laurent,
>>>
>>>Can you try this patch with your system since it is the one
>>>that has exhibited the problem so far. I've tested on a
>>>tpm2.0 and tpm1.2 system here.
>>>
>>>Regards,
>>>Jerry
>>
>