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

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

[Jerry Snitselaar]

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 <jarkko.sakkinen@linux.intel.com>
Cc: Peter Huewe <peterhuewe@gmx.de>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Reported-by: Laurent Bigonville <bigon@debian.org>
Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
 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

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

[Jerry Snitselaar]

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 <jarkko.sakkinen@linux.intel.com>
>Cc: Peter Huewe <peterhuewe@gmx.de>
>Cc: Jason Gunthorpe <jgg@ziepe.ca>
>Reported-by: Laurent Bigonville <bigon@debian.org>
>Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>---
> 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

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

[Laurent Bigonville]

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

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

[Jarkko Sakkinen]

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

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

[Laurent Bigonville]

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 <jarkko.sakkinen@linux.intel.com>
>> Cc: Peter Huewe <peterhuewe@gmx.de>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>> Reported-by: Laurent Bigonville <bigon@debian.org>
>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
Tested-by: Laurent Bigonville <bigon@debian.org>
>> ---
>> 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

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

[Laurent Bigonville]

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 <jarkko.sakkinen@linux.intel.com>
>>> Cc: Peter Huewe <peterhuewe@gmx.de>
>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>>> Reported-by: Laurent Bigonville <bigon@debian.org>
>>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Laurent Bigonville <bigon@debian.org>
>>> ---
>>> 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
>

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

[Jerry Snitselaar]

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 <jarkko.sakkinen@linux.intel.com>
>>>>Cc: Peter Huewe <peterhuewe@gmx.de>
>>>>Cc: Jason Gunthorpe <jgg@ziepe.ca>
>>>>Reported-by: Laurent Bigonville <bigon@debian.org>
>>>>Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>>Tested-by: Laurent Bigonville <bigon@debian.org>
>>>>---
>>>>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
>>
>

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

[Jarkko Sakkinen]

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 <jarkko.sakkinen@linux.intel.com>
> Cc: Peter Huewe <peterhuewe@gmx.de>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Reported-by: Laurent Bigonville <bigon@debian.org>
> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko