Re: [tpmdd-devel] [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Am 17. Februar 2017 06:09:42 MEZ schrieb Christophe Ricard 
:
>Are you sure it is not better to introduce this delay directly in the 
>rpi spi driver ?
>
>Other than that i don't see any issue with it.

Yes - it would be perfect to fix the issue in the upstream rpi spi master 
driver.
However this only happens sporadically in some strange cases (i.e. 300-500khz, 
single byte transfer after having more bytes in the same #cs frame) (unrelated 
to the tpm)

We are still looking into it, why /where it happens and how to reproduce it 
reliably and then file a bug/fix it.

For now however the priority is to make the tpm_tis_spi driver work reliably 
also on the rpi, and this is what this workaround does.

We can simply remove it once the rpi spi master is fixed.

Peter
>
>
>On 16/02/2017 08:09, Peter Huewe wrote:
>> Testing the implementation with a Raspberry Pi 2 showed that under
>some
>> circumstances its SPI master erroneously releases the CS line before
>the
>> transfer is complete, i.e. before the end of the last clock. In this
>case
>> the TPM ignores the transfer and misses for example the GO command.
>The
>> driver is unable to detect this communication problem and will wait
>for a
>> command response that is never going to arrive, timing out
>eventually.
>>
>> As a workaround, the small delay ensures that the CS line is held
>long
>> enough, even with a faulty SPI master. Other SPI masters are not
>affected,
>> except for a negligible performance penalty.
>>
>> Cc: 
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen 
>> Signed-off-by: Peter Huewe 
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index b50c5b072df3..685c51bf5d7e 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   
>>  spi_xfer.cs_change = 0;
>>  spi_xfer.len = transfer_len;
>> +spi_xfer.delay_usecs = 5;
>>   
>>  if (direction) {
>>  spi_xfer.tx_buf = NULL;

-- 
Sent from my mobile

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Am 17. Februar 2017 06:11:53 MEZ schrieb Christophe Ricard 
:
>I am not sure i understand here, are you considering there could be 
>burstcount > 64 with "TCG" TPM ?
>
With the current upstream version, any command larger with a response of more 
than 64 byte is broken, since the splitting loop went missing during the merge.
At least with our tpms that's the case.
(I think we are signalling a higher burstcount value than 64, which is allowed)

>Or is this because of TIS vs PTP differences ?
>
>To be honest, this is a little behind me now :-)
>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
>> layers, as tpm_tis has no such limitation. Add a loop to hide that
>> limitation.
>>
>> Cc: 
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen 
>> Signed-off-by: Peter Huewe 
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 108
>++---
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index 16938e2253d2..b50c5b072df3 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   {
>>  struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>>  int ret;
>> -struct spi_message m;
>> -struct spi_transfer spi_xfer = {
>> -.tx_buf = phy->tx_buf,
>> -.rx_buf = phy->rx_buf,
>> -.len = 4,
>> -.cs_change = 1,
>> -};
>> -
>> -if (len > MAX_SPI_FRAMESIZE)
>> -return -ENOMEM;
>>   
>> -phy->tx_buf[0] = direction | (len - 1);
>> -phy->tx_buf[1] = 0xd4;
>> -phy->tx_buf[2] = addr >> 8;
>> -phy->tx_buf[3] = addr;
>> +spi_bus_lock(phy->spi_device->master);
>>   
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> +while (len) {
>> +struct spi_message m;
>> +struct spi_transfer spi_xfer = {
>> +.tx_buf = phy->tx_buf,
>> +.rx_buf = phy->rx_buf,
>> +.len = 4,
>> +.cs_change = 1,
>> +};
>> +u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>> +
>> +phy->tx_buf[0] = direction | (transfer_len - 1);
>> +phy->tx_buf[1] = 0xd4;
>> +phy->tx_buf[2] = addr >> 8;
>> +phy->tx_buf[3] = addr;
>> +
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>>   
>> -spi_bus_lock(phy->spi_device->master);
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> -goto exit;
>> -
>> -if ((phy->rx_buf[3] & 0x01) == 0) {
>> -// handle SPI wait states
>> -int i;
>> -
>> -phy->tx_buf[0] = 0;
>> -
>> -for (i = 0; i < TPM_RETRY; i++) {
>> -spi_xfer.len = 1;
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> +if ((phy->rx_buf[3] & 0x01) == 0) {
>> +// handle SPI wait states
>> +int i;
>> +
>> +phy->tx_buf[0] = 0;
>> +
>> +for (i = 0; i < TPM_RETRY; i++) {
>> +spi_xfer.len = 1;
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>> +if (phy->rx_buf[0] & 0x01)
>> +break;
>> +}
>> +
>> +if (i == TPM_RETRY) {
>> +ret = -ETIMEDOUT;
>>  goto exit;
>> -if (phy->rx_buf[0] & 0x01)
>> -break;
>> +}
>>  }
>>   
>> -if (i == TPM_RETRY) {
>> -ret = -ETIMEDOUT;
>> -goto exit;
>> +spi_xfer.cs_change = 0;
>> +spi_xfer.len = transfer_len;
>> +
>> +if (direction) {
>> +spi_xfer.tx_buf = NULL;
>> +spi_xfer.rx_buf = buffer;
>> +} else {
>> +spi_xfer.tx_buf = buffer;
>> +spi_xfer.rx_buf = NULL;
>>  }
>> -}
>>   
>> -

Re: [tpmdd-devel] [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Am 17. Februar 2017 06:09:30 MEZ schrieb Christophe Ricard 
:
>That's is correct, this is a mistake on my side and never saw it :-(.
>
>I guess it was possibly leading to "waste" at least 1 wait state on
>some 
>TPMs.

Unfortunately the 1 for indicating end of waitstates does only appear once so 
it actually rendered the driver non-functional - atleast with our tpms.


>
>Wouldn't it be better to merge that with #1 and update the comment 
>consequently?

Yes, that's what I wanted to express in the cover letter, logically it makes 
sense to squash #1 and #3 - but reviewing it merged with #1 is quite hard since 
it "obfuscates" the problem - since too much stuff moves around.
That's why I decided to split it - for easier review.

Peter


>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Wait states are signaled in the last byte received from the TPM in
>> response to the header, not the first byte. Check rx_buf[3] instead
>of
>> rx_buf[0].
>>
>> Cc: 
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen 
>> Signed-off-by: Peter Huewe 
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 40
>+---
>>   1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index d782b9974c14..16938e2253d2 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data
>*data, u32 addr, u8 len,
>>  u8 *buffer, u8 direction)
>>   {
>>  struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>> -int ret, i;
>> +int ret;
>>  struct spi_message m;
>>  struct spi_transfer spi_xfer = {
>>  .tx_buf = phy->tx_buf,
>> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>  if (ret < 0)
>>  goto exit;
>>   
>> -phy->tx_buf[0] = 0;
>> -
>> -/* According to TCG PTP specification, if there is no TPM present
>at
>> - * all, then the design has a weak pull-up on MISO. If a TPM is not
>> - * present, a pull-up on MISO means that the SB controller sees a
>1,
>> - * and will latch in 0xFF on the read.
>> - */
>> -for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
>> -spi_xfer.len = 1;
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> +if ((phy->rx_buf[3] & 0x01) == 0) {
>> +// handle SPI wait states
>> +int i;
>> +
>> +phy->tx_buf[0] = 0;
>> +
>> +for (i = 0; i < TPM_RETRY; i++) {
>> +spi_xfer.len = 1;
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>> +if (phy->rx_buf[0] & 0x01)
>> +break;
>> +}
>> +
>> +if (i == TPM_RETRY) {
>> +ret = -ETIMEDOUT;
>>  goto exit;
>> -}
>> -
>> -if (i == TPM_RETRY) {
>> -ret = -ETIMEDOUT;
>> -goto exit;
>> +}
>>  }
>>   
>>  spi_xfer.cs_change = 0;

-- 
Sent from my mobile

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Christophe Ricard]

I am not sure i understand here, are you considering there could be 
burstcount > 64 with "TCG" TPM ?

Or is this because of TIS vs PTP differences ?

To be honest, this is a little behind me now :-)


On 16/02/2017 08:08, Peter Huewe wrote:
> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
> layers, as tpm_tis has no such limitation. Add a loop to hide that
> limitation.
>
> Cc: 
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen 
> Signed-off-by: Peter Huewe 
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 108 
> ++---
>   1 file changed, 57 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 16938e2253d2..b50c5b072df3 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data 
> *data, u32 addr, u8 len,
>   {
>   struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>   int ret;
> - struct spi_message m;
> - struct spi_transfer spi_xfer = {
> - .tx_buf = phy->tx_buf,
> - .rx_buf = phy->rx_buf,
> - .len = 4,
> - .cs_change = 1,
> - };
> -
> - if (len > MAX_SPI_FRAMESIZE)
> - return -ENOMEM;
>   
> - phy->tx_buf[0] = direction | (len - 1);
> - phy->tx_buf[1] = 0xd4;
> - phy->tx_buf[2] = addr >> 8;
> - phy->tx_buf[3] = addr;
> + spi_bus_lock(phy->spi_device->master);
>   
> - spi_message_init();
> - spi_message_add_tail(_xfer, );
> + while (len) {
> + struct spi_message m;
> + struct spi_transfer spi_xfer = {
> + .tx_buf = phy->tx_buf,
> + .rx_buf = phy->rx_buf,
> + .len = 4,
> + .cs_change = 1,
> + };
> + u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> +
> + phy->tx_buf[0] = direction | (transfer_len - 1);
> + phy->tx_buf[1] = 0xd4;
> + phy->tx_buf[2] = addr >> 8;
> + phy->tx_buf[3] = addr;
> +
> + spi_message_init();
> + spi_message_add_tail(_xfer, );
> + ret = spi_sync_locked(phy->spi_device, );
> + if (ret < 0)
> + goto exit;
>   
> - spi_bus_lock(phy->spi_device->master);
> - ret = spi_sync_locked(phy->spi_device, );
> - if (ret < 0)
> - goto exit;
> -
> - if ((phy->rx_buf[3] & 0x01) == 0) {
> - // handle SPI wait states
> - int i;
> -
> - phy->tx_buf[0] = 0;
> -
> - for (i = 0; i < TPM_RETRY; i++) {
> - spi_xfer.len = 1;
> - spi_message_init();
> - spi_message_add_tail(_xfer, );
> - ret = spi_sync_locked(phy->spi_device, );
> - if (ret < 0)
> + if ((phy->rx_buf[3] & 0x01) == 0) {
> + // handle SPI wait states
> + int i;
> +
> + phy->tx_buf[0] = 0;
> +
> + for (i = 0; i < TPM_RETRY; i++) {
> + spi_xfer.len = 1;
> + spi_message_init();
> + spi_message_add_tail(_xfer, );
> + ret = spi_sync_locked(phy->spi_device, );
> + if (ret < 0)
> + goto exit;
> + if (phy->rx_buf[0] & 0x01)
> + break;
> + }
> +
> + if (i == TPM_RETRY) {
> + ret = -ETIMEDOUT;
>   goto exit;
> - if (phy->rx_buf[0] & 0x01)
> - break;
> + }
>   }
>   
> - if (i == TPM_RETRY) {
> - ret = -ETIMEDOUT;
> - goto exit;
> + spi_xfer.cs_change = 0;
> + spi_xfer.len = transfer_len;
> +
> + if (direction) {
> + spi_xfer.tx_buf = NULL;
> + spi_xfer.rx_buf = buffer;
> + } else {
> + spi_xfer.tx_buf = buffer;
> + spi_xfer.rx_buf = NULL;
>   }
> - }
>   
> - spi_xfer.cs_change = 0;
> - spi_xfer.len = len;
> + spi_message_init();
> + spi_message_add_tail(_xfer, );
> + ret = spi_sync_locked(phy->spi_device, );
> + if (ret < 0)
> + goto exit;
>   
> - if (direction) {
> - spi_xfer.tx_buf = NULL;
> - spi_xfer.rx_buf = buffer;
> - } else {
> - spi_xfer.tx_buf = 

Re: [tpmdd-devel] [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Christophe Ricard]

That's is correct, this is a mistake on my side and never saw it :-(.

I guess it was possibly leading to "waste" at least 1 wait state on some 
TPMs.

Wouldn't it be better to merge that with #1 and update the comment 
consequently?


On 16/02/2017 08:08, Peter Huewe wrote:
> Wait states are signaled in the last byte received from the TPM in
> response to the header, not the first byte. Check rx_buf[3] instead of
> rx_buf[0].
>
> Cc: 
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen 
> Signed-off-by: Peter Huewe 
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 40 +---
>   1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index d782b9974c14..16938e2253d2 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
> u32 addr, u8 len,
>   u8 *buffer, u8 direction)
>   {
>   struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> - int ret, i;
> + int ret;
>   struct spi_message m;
>   struct spi_transfer spi_xfer = {
>   .tx_buf = phy->tx_buf,
> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data 
> *data, u32 addr, u8 len,
>   if (ret < 0)
>   goto exit;
>   
> - phy->tx_buf[0] = 0;
> -
> - /* According to TCG PTP specification, if there is no TPM present at
> -  * all, then the design has a weak pull-up on MISO. If a TPM is not
> -  * present, a pull-up on MISO means that the SB controller sees a 1,
> -  * and will latch in 0xFF on the read.
> -  */
> - for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
> - spi_xfer.len = 1;
> - spi_message_init();
> - spi_message_add_tail(_xfer, );
> - ret = spi_sync_locked(phy->spi_device, );
> - if (ret < 0)
> + if ((phy->rx_buf[3] & 0x01) == 0) {
> + // handle SPI wait states
> + int i;
> +
> + phy->tx_buf[0] = 0;
> +
> + for (i = 0; i < TPM_RETRY; i++) {
> + spi_xfer.len = 1;
> + spi_message_init();
> + spi_message_add_tail(_xfer, );
> + ret = spi_sync_locked(phy->spi_device, );
> + if (ret < 0)
> + goto exit;
> + if (phy->rx_buf[0] & 0x01)
> + break;
> + }
> +
> + if (i == TPM_RETRY) {
> + ret = -ETIMEDOUT;
>   goto exit;
> - }
> -
> - if (i == TPM_RETRY) {
> - ret = -ETIMEDOUT;
> - goto exit;
> + }
>   }
>   
>   spi_xfer.cs_change = 0;


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Christophe Ricard]

Are you sure it is not better to introduce this delay directly in the 
rpi spi driver ?

Other than that i don't see any issue with it.


On 16/02/2017 08:09, Peter Huewe wrote:
> Testing the implementation with a Raspberry Pi 2 showed that under some
> circumstances its SPI master erroneously releases the CS line before the
> transfer is complete, i.e. before the end of the last clock. In this case
> the TPM ignores the transfer and misses for example the GO command. The
> driver is unable to detect this communication problem and will wait for a
> command response that is never going to arrive, timing out eventually.
>
> As a workaround, the small delay ensures that the CS line is held long
> enough, even with a faulty SPI master. Other SPI masters are not affected,
> except for a negligible performance penalty.
>
> Cc: 
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen 
> Signed-off-by: Peter Huewe 
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index b50c5b072df3..685c51bf5d7e 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data 
> *data, u32 addr, u8 len,
>   
>   spi_xfer.cs_change = 0;
>   spi_xfer.len = transfer_len;
> + spi_xfer.delay_usecs = 5;
>   
>   if (direction) {
>   spi_xfer.tx_buf = NULL;


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Christophe Ricard]

Nitpick: Are you sure this patch is necessary having #3 on top of it ?

On 16/02/2017 08:08, Peter Huewe wrote:
> Abort the transfer with ETIMEDOUT when the TPM signals more than
> TPM_RETRY wait states. Continuing with the transfer in this state
> will only lead to arbitrary failures in other parts of the code.
>
> Cc: 
> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
> Signed-off-by: Alexander Steffen 
> Signed-off-by: Peter Huewe 
> ---
>   drivers/char/tpm/tpm_tis_spi.c | 5 +
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
> index 6e1a3c43f621..d782b9974c14 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data 
> *data, u32 addr, u8 len,
>   goto exit;
>   }
>   
> + if (i == TPM_RETRY) {
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
>   spi_xfer.cs_change = 0;
>   spi_xfer.len = len;
>   


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Winkler, Tomas]

This is a BIOS issue + kernel ACPI platform driver refusing to handle 
overlapping memory space. The issue here is that the ACPI platform driver is 
not created, however this should not prevent  the tpm_crb driver from working 
as it registers directly the ACPI device. For now you can suppress the message 
by adding the device in forbidden_id_list in drivers/acpi/acpi_platform.c

I’m already testing a fixed BIOS, not sure whether and when it will be 
available for your particular device.

Thanks
Tomas

From: Davide Guerri [mailto:davide.gue...@gmail.com]
Sent: Thursday, February 16, 2017 20:40
To: Jason Gunthorpe 
Cc: tpmdd-devel@lists.sourceforge.net
Subject: Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

Sorry I missed 1 line:


[20417.678952] ACPI resource is [mem 0xfed4-0xfed4087f flags 0x200]

[20417.678975] map request is is [mem 0xfed40040-0xfed4006f flags 0x200]

[20417.678990] map request is is [mem 0xfed40080-0xfed40fff flags 0x200]

[20417.678996] tpm_crb MSFT0101:00: can't request region for resource [mem 
0xfed40080-0xfed40fff]

[20417.688797] tpm_crb: probe of MSFT0101:00 failed with error -16


On 16 February 2017 at 18:39, Davide Guerri 
> wrote:

[20417.678975] map request is is [mem 0xfed40040-0xfed4006f flags 0x200]

[20417.678990] map request is is [mem 0xfed40080-0xfed40fff flags 0x200]

[20417.678996] tpm_crb MSFT0101:00: can't request region for resource [mem 
0xfed40080-0xfed40fff]

[20417.688797] tpm_crb: probe of MSFT0101:00 failed with error -16



On 16 February 2017 at 18:26, Davide Guerri 
> wrote:
No lines including the requested range:


fed1-fed17fff : pnp 00:06

fed18000-fed18fff : pnp 00:06

fed19000-fed19fff : pnp 00:06

fed2-fed3 : pnp 00:06

fed4-fed4087f : MSFT0101:00

fed45000-fed8 : pnp 00:06

fed9-fed93fff : pnp 00:06

fee0-fee00fff : Local APIC

This is the NUC I am using, if that can be useful.


root@vhsv1:~# cat /sys/class/dmi/id/board_name /sys/class/dmi/id/board_version

NUC6i7KYB

H90766-404

I am compiling the module right as we speak, I will get back to you soon.

On 16 February 2017 at 18:19, Jason Gunthorpe 
> wrote:
On Thu, Feb 16, 2017 at 06:10:43PM +, Davide Guerri wrote:
>Hey thanks for the prompt reply.
>I think you are interested in this:
>   fed4-fed4087f : MSFT0101:00

Are there more lines below that?

Can you apply this patch and report what the results are?

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index a7c870af916c3d..acc54a03d6025d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -233,6 +233,8 @@ static void __iomem *crb_map_res(struct device *dev, struct 
crb_priv *priv,
.flags  = IORESOURCE_MEM,
};

+   printk("map request is is %pr\n",_res);
+
/* Detect a 64 bit address on a 32 bit system */
if (start != new_res.start)
return (void __iomem *) ERR_PTR(-EINVAL);
@@ -267,6 +269,8 @@ static int crb_map_io(struct acpi_device *device, struct 
crb_priv *priv,
return -EINVAL;
}

+   printk("ACPI resource is %pr\n",_res);
+
priv->iobase = devm_ioremap_resource(dev, _res);
if (IS_ERR(priv->iobase))
return PTR_ERR(priv->iobase);



--


[Davide Guerri on about.me]



Davide Guerri
about.me/davide_guerri






--


[Davide Guerri on about.me]



Davide Guerri
about.me/davide_guerri






--


[Davide Guerri on about.me]



Davide Guerri
about.me/davide_guerri




--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Davide Guerri]

> On 16 Feb 2017, at 8:27 pm, Jarkko Sakkinen  
> wrote:
> 
> On Thu, Feb 16, 2017 at 06:26:29PM +, Davide Guerri wrote:
>>   No lines including the requested range:
>> 
>>   fed1-fed17fff : pnp 00:06
>> 
>>   fed18000-fed18fff : pnp 00:06
>> 
>>   fed19000-fed19fff : pnp 00:06
>> 
>>   fed2-fed3 : pnp 00:06
>> 
>>   fed4-fed4087f : MSFT0101:00
> 
> It really looks like that the BIOS is reporting wrong value.
> Have you tried to update it?
> 
> /Jarkko

Hi Jarkko,
I apologise for having used an HTML user agent, I hadn't one configured for 
this account and I just used the google UI.

So, I can try upgrading it. But I wonder whether acpi_enforce_resources=lax 
would work around the issue.
I am trying that later and I will let you know.

Thanks,
 Davide.
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 02:06:42PM -0600, Dr. Greg Wettstein wrote:
> Just as an aside, has anyone given any thought about TPM2 resource
> management in things like TXT/tboot environments?  The current tboot
> code makes a rather naive assumption that it can take a handle slot to
> protect its platform verification secret.  Doing resource management
> correctly will require addressing extra-OS environments such as this
> which may have TPM2 state requirement issues.

The current implementation handles stuff created from regular /dev/tpm0
so I do not think this would be an issue. You can only access objects
from a TPM space that are created within that space.

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 06:26:29PM +, Davide Guerri wrote:
>No lines including the requested range:
> 
>fed1-fed17fff : pnp 00:06
> 
>fed18000-fed18fff : pnp 00:06
> 
>fed19000-fed19fff : pnp 00:06
> 
>fed2-fed3 : pnp 00:06
> 
>fed4-fed4087f : MSFT0101:00

It really looks like that the BIOS is reporting wrong value.
Have you tried to update it?

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Jarkko Sakkinen]

On Thu, Feb 16, 2017 at 11:15:41AM +, Davide Guerri wrote:
>Hello guys,
>I am trying to get the fTPM on an Intel NUC working bit I get this error
> 
>[    3.191376] tpm_crb MSFT0101:00: can't request region for resource
>[mem 0xfed40080-0xfed40fff]
> 
>[    3.200378] tpm_crb: probe of MSFT0101:00 failed with error -16
> 
>and, of course, the TPM device is not registered in dev/sysfs
>In my understanding this is a regression as it has been already fixed in
>the past and merged in 4.7 or 4.8.
>I also verified my source and tpm_crb.c contains this
>patch: 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=422eac3f7deae34dbaffd08e03e27f37a5394a56
>Is that a known issue? If so, are there workaround/patches?
>Thanks in advance,
> Davide.

Could you please use plain text email, '>' quoting and reply inline?
Your emails are almost impossible to read with plain text email client
like the one that I use (links dump looks terrible).

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms

[Jarkko Sakkinen]

From: James Bottomley 

Currently the tpm spaces are not exposed to userspace.  Make this
exposure via a separate device, which can now be opened multiple times
because each read/write transaction goes separately via the space.

Concurrency is protected by the chip->tpm_mutex for each read/write
transaction separately.  The TPM is cleared of all transient objects
by the time the mutex is dropped, so there should be no interference
between the kernel and userspace.

Signed-off-by: James Bottomley 
---
 drivers/char/tpm/Makefile|  3 +-
 drivers/char/tpm/tpm-chip.c  | 73 ++--
 drivers/char/tpm/tpm-interface.c | 13 +--
 drivers/char/tpm/tpm.h   |  4 +++
 drivers/char/tpm/tpms-dev.c  | 65 +++
 5 files changed, 152 insertions(+), 6 deletions(-)
 create mode 100644 drivers/char/tpm/tpms-dev.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 10e5827..bbe6531 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -3,7 +3,8 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-tpm-dev-common.o tpm1_eventlog.o tpm2_eventlog.o tpm2-space.o
+tpm-dev-common.o tpms-dev.o tpm1_eventlog.o tpm2_eventlog.o \
+ tpm2-space.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 993b9ae..c71c353 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,6 +33,7 @@ DEFINE_IDR(dev_nums_idr);
 static DEFINE_MUTEX(idr_lock);
 
 struct class *tpm_class;
+struct class *tpms_class;
 dev_t tpm_devt;
 
 /**
@@ -132,6 +133,14 @@ static void tpm_dev_release(struct device *dev)
kfree(chip);
 }
 
+static void tpm_devs_release(struct device *dev)
+{
+   struct tpm_chip *chip = container_of(dev, struct tpm_chip, devs);
+
+   /* release the master device reference */
+   put_device(>dev);
+}
+
 /**
  * tpm_chip_alloc() - allocate a new struct tpm_chip instance
  * @pdev: device to which the chip is associated
@@ -168,27 +177,47 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
chip->dev_num = rc;
 
device_initialize(>dev);
+   device_initialize(>devs);
 
chip->dev.class = tpm_class;
chip->dev.release = tpm_dev_release;
chip->dev.parent = pdev;
chip->dev.groups = chip->groups;
 
+   chip->devs.parent = pdev;
+   chip->devs.class = tpms_class;
+   chip->devs.release = tpm_devs_release;
+   /* get extra reference on main device to hold on
+* behalf of devs.  This holds the chip structure
+* while cdevs is in use.  The corresponding put
+* is in the tpm_devs_release
+*/
+   get_device(>dev);
+
if (chip->dev_num == 0)
chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
else
chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
 
+   chip->devs.devt =
+   MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);
+
rc = dev_set_name(>dev, "tpm%d", chip->dev_num);
if (rc)
goto out;
+   rc = dev_set_name(>devs, "tpms%d", chip->dev_num);
+   if (rc)
+   goto out;
 
if (!pdev)
chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
 
cdev_init(>cdev, _fops);
+   cdev_init(>cdevs, _fops);
chip->cdev.owner = THIS_MODULE;
+   chip->cdevs.owner = THIS_MODULE;
chip->cdev.kobj.parent = >dev.kobj;
+   chip->cdevs.kobj.parent = >devs.kobj;
 
chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
if (!chip->work_space.context_buf) {
@@ -199,6 +228,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
return chip;
 
 out:
+   put_device(>devs);
put_device(>dev);
return ERR_PTR(rc);
 }
@@ -244,7 +274,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
dev_name(>dev), MAJOR(chip->dev.devt),
MINOR(chip->dev.devt), rc);
 
-   return rc;
+   goto err_1;
}
 
rc = device_add(>dev);
@@ -254,16 +284,44 @@ static int tpm_add_char_device(struct tpm_chip *chip)
dev_name(>dev), MAJOR(chip->dev.devt),
MINOR(chip->dev.devt), rc);
 
-   cdev_del(>cdev);
-   return rc;
+   goto err_2;
+   }
+
+   if (chip->flags & TPM_CHIP_FLAG_TPM2)
+   rc = cdev_add(>cdevs, chip->devs.devt, 1);
+   if (rc) {
+   dev_err(>dev,
+   "unable to cdev_add() %s, major %d, minor %d, err=%d\n",
+   dev_name(>devs), MAJOR(chip->devs.devt),
+   

[tpmdd-devel] [PATCH v2 3/7] tpm: export tpm2_flush_context_cmd

[Jarkko Sakkinen]

Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm.h  |  2 ++
 drivers/char/tpm/tpm2-cmd.c | 62 +
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 50aebe9..0ec1cf0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -555,6 +555,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 
*res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
struct tpm2_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max);
+void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
+   unsigned int flags);
 int tpm2_seal_trusted(struct tpm_chip *chip,
  struct trusted_key_payload *payload,
  struct trusted_key_options *options);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 7a56f44..897902a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -419,6 +419,35 @@ static const struct tpm_input_header 
tpm2_get_tpm_pt_header = {
 };
 
 /**
+ * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ *
+ * Return: same as with tpm_transmit_cmd
+ */
+void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
+   unsigned int flags)
+{
+   struct tpm_buf buf;
+   int rc;
+
+   rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
+   if (rc) {
+   dev_warn(>dev, "0x%08x was not flushed, out of memory\n",
+handle);
+   return;
+   }
+
+   tpm_buf_append_u32(, handle);
+
+   (void) tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, flags,
+   "flushing context");
+
+   tpm_buf_destroy();
+}
+
+/**
  * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
  *
  * @buf: an allocated tpm_buf instance
@@ -628,39 +657,6 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 }
 
 /**
- * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
- *
- * @chip: TPM chip to use
- * @handle: the key data in clear and encrypted form
- * @flags: tpm transmit flags
- *
- * Return: Same as with tpm_transmit_cmd.
- */
-static void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
-  unsigned int flags)
-{
-   struct tpm_buf buf;
-   int rc;
-
-   rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
-   if (rc) {
-   dev_warn(>dev, "0x%08x was not flushed, out of memory\n",
-handle);
-   return;
-   }
-
-   tpm_buf_append_u32(, handle);
-
-   rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, flags,
- "flushing context");
-   if (rc)
-   dev_warn(>dev, "0x%08x was not flushed, rc=%d\n", handle,
-rc);
-
-   tpm_buf_destroy();
-}
-
-/**
  * tpm2_unseal_cmd() - execute a TPM2_Unload command
  *
  * @chip: TPM chip to use
-- 
2.9.3


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH v2 7/7] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

From: James Bottomley 

Sessions are different from transient objects in that their handles
may not be virtualized (because they're used for some hmac
calculations).  Additionally when a session is context saved, a
vestigial memory remains in the TPM and if it is also flushed, that
will be lost and the session context will refuse to load next time, so
the code is updated to flush only transient objects after a context
save.  Add a separate array (chip->session_tbl) to save and restore
sessions by handle.  Use the failure of a context save or load to
signal that the session has been flushed from the TPM and we can
remove its memory from chip->session_tbl.

Sessions are also isolated during each instance of a tpm space.  This
means that spaces shouldn't be able to see each other's sessions and
is enforced by ensuring that a space user may only refer to sessions
handles that are present in their own chip->session_tbl.  Finally when
a space is closed, all the sessions belonging to it should be flushed
so the handles may be re-used by other spaces.

Note that if we get a session save or load error, all sessions are
effectively flushed.  Even though we restore the session buffer, all
the old sessions will refuse to load after the flush and they'll be
purged from our session memory.  This means that while transient
context handling is still soft in the face of errors, session handling
is hard (any failure of the model means all sessions are lost).

Signed-off-by: James Bottomley 
---
 drivers/char/tpm/tpm-chip.c   |   6 +++
 drivers/char/tpm/tpm.h|   4 +-
 drivers/char/tpm/tpm2-space.c | 105 +-
 drivers/char/tpm/tpms-dev.c   |   2 +-
 4 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index c71c353..e8536ab 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -130,6 +130,7 @@ static void tpm_dev_release(struct device *dev)
 
kfree(chip->log.bios_event_log);
kfree(chip->work_space.context_buf);
+   kfree(chip->work_space.session_buf);
kfree(chip);
 }
 
@@ -224,6 +225,11 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
rc = -ENOMEM;
goto out;
}
+   chip->work_space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+   if (!chip->work_space.session_buf) {
+   rc = -ENOMEM;
+   goto out;
+   }
 
return chip;
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 822ca67..ffee8b5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -161,6 +161,8 @@ enum tpm2_cc_attrs {
 struct tpm_space {
u32 context_tbl[3];
u8 *context_buf;
+   u32 session_tbl[3];
+   u8 *session_buf;
 };
 
 enum tpm_chip_flags {
@@ -589,7 +591,7 @@ int tpm2_probe(struct tpm_chip *chip);
 ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
 int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 int tpm2_init_space(struct tpm_space *space);
-void tpm2_del_space(struct tpm_space *space);
+void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
 int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
   u8 *cmd);
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index e955548..d36d81e 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -32,18 +32,39 @@ struct tpm2_context {
__be16 blob_size;
 } __packed;
 
+static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
+{
+   int i;
+
+   for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+   if (space->session_tbl[i])
+   tpm2_flush_context_cmd(chip, space->session_tbl[i],
+  TPM_TRANSMIT_UNLOCKED);
+   }
+}
+
 int tpm2_init_space(struct tpm_space *space)
 {
space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
if (!space->context_buf)
return -ENOMEM;
 
+   space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+   if (space->session_buf == NULL) {
+   kfree(space->context_buf);
+   return -ENOMEM;
+   }
+
return 0;
 }
 
-void tpm2_del_space(struct tpm_space *space)
+void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
 {
+   mutex_lock(>tpm_mutex);
+   tpm2_flush_sessions(chip, space);
+   mutex_unlock(>tpm_mutex);
kfree(space->context_buf);
+   kfree(space->session_buf);
 }
 
 static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
@@ -69,6 +90,20 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 __func__, rc);
tpm_buf_destroy();
return 

[tpmdd-devel] [PATCH v2 5/7] tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c

[Jarkko Sakkinen]

From: James Bottomley 

Signed-off-by: James Bottomley 
---
 drivers/char/tpm/Makefile |   2 +-
 drivers/char/tpm/tpm-dev-common.c | 148 ++
 drivers/char/tpm/tpm-dev.c| 143 
 drivers/char/tpm/tpm-dev.h|  27 +++
 4 files changed, 190 insertions(+), 130 deletions(-)
 create mode 100644 drivers/char/tpm/tpm-dev-common.c
 create mode 100644 drivers/char/tpm/tpm-dev.h

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 8f07fcf..10e5827 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -3,7 +3,7 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-tpm1_eventlog.o tpm2_eventlog.o tpm2-space.o
+tpm-dev-common.o tpm1_eventlog.o tpm2_eventlog.o tpm2-space.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-dev-common.c 
b/drivers/char/tpm/tpm-dev-common.c
new file mode 100644
index 000..610638a
--- /dev/null
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -0,0 +1,148 @@
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Authors:
+ * Leendert van Doorn 
+ * Dave Safford 
+ * Reiner Sailer 
+ * Kylene Hall 
+ *
+ * Copyright (C) 2013 Obsidian Research Corp
+ * Jason Gunthorpe 
+ *
+ * Device file system interface to the TPM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ */
+#include 
+#include 
+#include "tpm.h"
+#include "tpm-dev.h"
+
+static void user_reader_timeout(unsigned long ptr)
+{
+   struct file_priv *priv = (struct file_priv *)ptr;
+
+   pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
+   task_tgid_nr(current));
+
+   schedule_work(>work);
+}
+
+static void timeout_work(struct work_struct *work)
+{
+   struct file_priv *priv = container_of(work, struct file_priv, work);
+
+   mutex_lock(>buffer_mutex);
+   atomic_set(>data_pending, 0);
+   memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
+   mutex_unlock(>buffer_mutex);
+}
+
+void tpm_common_open(struct file *file, struct tpm_chip *chip,
+struct file_priv *priv)
+{
+   priv->chip = chip;
+   atomic_set(>data_pending, 0);
+   mutex_init(>buffer_mutex);
+   setup_timer(>user_read_timer, user_reader_timeout,
+   (unsigned long)priv);
+   INIT_WORK(>work, timeout_work);
+
+   file->private_data = priv;
+}
+
+ssize_t tpm_common_read(struct file *file, char __user *buf,
+   size_t size, loff_t *off)
+{
+   struct file_priv *priv = file->private_data;
+   ssize_t ret_size;
+   ssize_t orig_ret_size;
+   int rc;
+
+   del_singleshot_timer_sync(>user_read_timer);
+   flush_work(>work);
+   ret_size = atomic_read(>data_pending);
+   if (ret_size > 0) { /* relay data */
+   orig_ret_size = ret_size;
+   if (size < ret_size)
+   ret_size = size;
+
+   mutex_lock(>buffer_mutex);
+   rc = copy_to_user(buf, priv->data_buffer, ret_size);
+   memset(priv->data_buffer, 0, orig_ret_size);
+   if (rc)
+   ret_size = -EFAULT;
+
+   mutex_unlock(>buffer_mutex);
+   }
+
+   atomic_set(>data_pending, 0);
+
+   return ret_size;
+}
+
+ssize_t tpm_common_write(struct file *file, const char __user *buf,
+size_t size, loff_t *off, struct tpm_space *space)
+{
+   struct file_priv *priv = file->private_data;
+   size_t in_size = size;
+   ssize_t out_size;
+
+   /* Cannot perform a write until the read has cleared either via
+* tpm_read or a user_read_timer timeout. This also prevents split
+* buffered writes from blocking here.
+*/
+   if (atomic_read(>data_pending) != 0)
+   return -EBUSY;
+
+   if (in_size > TPM_BUFSIZE)
+   return -E2BIG;
+
+   mutex_lock(>buffer_mutex);
+
+   if (copy_from_user
+   (priv->data_buffer, (void __user *) buf, in_size)) {
+   mutex_unlock(>buffer_mutex);
+   return -EFAULT;
+   }
+
+   /* atomic tpm command send and result receive. We only hold the ops
+* lock during this period so that the tpm can be unregistered even if
+* the char dev is held open.
+*/
+   if (tpm_try_get_ops(priv->chip)) {
+   mutex_unlock(>buffer_mutex);
+   return -EPIPE;
+   }
+   

[tpmdd-devel] [PATCH v2 4/7] tpm: infrastructure for TPM spaces

[Jarkko Sakkinen]

Added an ability to virtualize TPM commands into an isolated context
that we call a TPM space because the word context is already heavily
used in the TPM specification. Both the handle areas and bodies (where
necessary) are virtualized.

The mechanism works by adding a new parameter struct tpm_space to the
tpm_transmit() function. This new structure contains the list of virtual
handles and a buffer of page size (currently) for backing storage.

When tpm_transmit() is called with a struct tpm_space instance it will
execute the following sequence:

1. Take locks.
2. Load transient objects from the backing storage by using ContextLoad
   and map virtual handles to physical handles.
3. Perform the transaction.
4. Save transient objects to backing storage by using ContextSave and
   map resulting physical handle to virtual handle if there is such.

This commit does not implement virtualization support for hmac and
policy sessions.

Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/Makefile|   2 +-
 drivers/char/tpm/tpm-chip.c  |   7 +
 drivers/char/tpm/tpm-dev.c   |   2 +-
 drivers/char/tpm/tpm-interface.c |  68 +++---
 drivers/char/tpm/tpm-sysfs.c |   2 +-
 drivers/char/tpm/tpm.h   |  26 ++-
 drivers/char/tpm/tpm2-cmd.c  |  33 +--
 drivers/char/tpm/tpm2-space.c| 431 +++
 8 files changed, 520 insertions(+), 51 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2-space.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 3d386a8..8f07fcf 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -3,7 +3,7 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-   tpm1_eventlog.o tpm2_eventlog.o
+tpm1_eventlog.o tpm2_eventlog.o tpm2-space.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index c406343..993b9ae 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -128,6 +128,7 @@ static void tpm_dev_release(struct device *dev)
mutex_unlock(_lock);
 
kfree(chip->log.bios_event_log);
+   kfree(chip->work_space.context_buf);
kfree(chip);
 }
 
@@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
chip->cdev.owner = THIS_MODULE;
chip->cdev.kobj.parent = >dev.kobj;
 
+   chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+   if (!chip->work_space.context_buf) {
+   rc = -ENOMEM;
+   goto out;
+   }
+
return chip;
 
 out:
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 02a8850..414553b 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -147,7 +147,7 @@ static ssize_t tpm_write(struct file *file, const char 
__user *buf,
mutex_unlock(>buffer_mutex);
return -EPIPE;
}
-   out_size = tpm_transmit(priv->chip, priv->data_buffer,
+   out_size = tpm_transmit(priv->chip, NULL, priv->data_buffer,
sizeof(priv->data_buffer), 0);
 
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 20b1fe3..db5ffe9 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -376,11 +376,12 @@ static bool tpm_validate_command(struct tpm_chip *chip, 
const u8 *cmd,
  * 0 when the operation is successful.
  * A negative number for system errors (errno).
  */
-ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
-unsigned int flags)
+ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
+u8 *buf, size_t bufsiz, unsigned int flags)
 {
-   const struct tpm_output_header *header = (void *)buf;
-   ssize_t rc;
+   struct tpm_output_header *header = (void *)buf;
+   int rc;
+   ssize_t len = 0;
u32 count, ordinal;
unsigned long stop;
 
@@ -406,10 +407,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 
*buf, size_t bufsiz,
if (chip->dev.parent)
pm_runtime_get_sync(chip->dev.parent);
 
+   rc = tpm2_prepare_space(chip, space, ordinal, buf);
+   if (rc)
+   goto out;
+
rc = chip->ops->send(chip, (u8 *) buf, count);
if (rc < 0) {
dev_err(>dev,
-   "tpm_transmit: tpm_send: error %zd\n", rc);
+   "tpm_transmit: tpm_send: error %d\n", rc);
goto out;
}
 
@@ -442,18 +447,23 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 
*buf, size_t bufsiz,
goto out;
 
 out_recv:
-   rc = chip->ops->recv(chip, (u8 *) buf, bufsiz);
-   if (rc < 0) {
+   len = 

[tpmdd-devel] [PATCH v2 2/7] tpm: validate TPM 2.0 commands

[Jarkko Sakkinen]

Check for every TPM 2.0 command that the command code is supported and
the command buffer has at least the length that can contain the header
and the handle area.

For ContextSave and FlushContext we mark the body to be part of the
handle area. This gives validation for these commands at zero
cost, including the body of the command.

The more important reason for this is that we can virtualize these
commands in the same way as you would virtualize the handle area of a
command.

Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm-interface.c | 38 ++-
 drivers/char/tpm/tpm.h   | 15 
 drivers/char/tpm/tpm2-cmd.c  | 79 
 3 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 708d356..20b1fe3 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -328,6 +328,42 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip 
*chip,
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
+static bool tpm_validate_command(struct tpm_chip *chip, const u8 *cmd,
+size_t len)
+{
+   const struct tpm_input_header *header = (const void *)cmd;
+   int i;
+   u32 cc;
+   u32 attrs;
+   unsigned int nr_handles;
+
+   if (len < TPM_HEADER_SIZE)
+   return false;
+
+   if (chip->flags & TPM_CHIP_FLAG_TPM2 && chip->nr_commands) {
+   cc = be32_to_cpu(header->ordinal);
+
+   i = tpm2_find_cc(chip, cc);
+   if (i < 0) {
+   dev_dbg(>dev, "0x%04X is an invalid command\n",
+   cc);
+   return false;
+   }
+
+   attrs = chip->cc_attrs_tbl[i];
+   nr_handles =
+   4 * ((attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0));
+   if (len < TPM_HEADER_SIZE + 4 * nr_handles)
+   goto err_len;
+   }
+
+   return true;
+err_len:
+   dev_dbg(>dev,
+   "%s: insufficient command length %zu", __func__, len);
+   return false;
+}
+
 /**
  * tmp_transmit - Internal kernel interface to transmit TPM commands.
  *
@@ -348,7 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, 
size_t bufsiz,
u32 count, ordinal;
unsigned long stop;
 
-   if (bufsiz < TPM_HEADER_SIZE)
+   if (!tpm_validate_command(chip, buf, bufsiz))
return -EINVAL;
 
if (bufsiz > TPM_BUFSIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6b4e7aa..50aebe9 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -114,6 +114,7 @@ enum tpm2_command_codes {
TPM2_CC_CREATE  = 0x0153,
TPM2_CC_LOAD= 0x0157,
TPM2_CC_UNSEAL  = 0x015E,
+   TPM2_CC_CONTEXT_SAVE= 0x0162,
TPM2_CC_FLUSH_CONTEXT   = 0x0165,
TPM2_CC_GET_CAPABILITY  = 0x017A,
TPM2_CC_GET_RANDOM  = 0x017B,
@@ -127,15 +128,25 @@ enum tpm2_permanent_handles {
 };
 
 enum tpm2_capabilities {
+   TPM2_CAP_COMMANDS   = 2,
TPM2_CAP_PCRS   = 5,
TPM2_CAP_TPM_PROPERTIES = 6,
 };
 
+enum tpm2_properties {
+   TPM_PT_TOTAL_COMMANDS   = 0x0129,
+};
+
 enum tpm2_startup_types {
TPM2_SU_CLEAR   = 0x,
TPM2_SU_STATE   = 0x0001,
 };
 
+enum tpm2_cc_attrs {
+   TPM2_CC_ATTR_CHANDLES   = 25,
+   TPM2_CC_ATTR_RHANDLE= 28,
+};
+
 #define TPM_VID_INTEL0x8086
 #define TPM_VID_WINBOND  0x1050
 #define TPM_VID_STM  0x104A
@@ -199,6 +210,9 @@ struct tpm_chip {
acpi_handle acpi_dev_handle;
char ppi_version[TPM_PPI_VERSION_LEN + 1];
 #endif /* CONFIG_ACPI */
+
+   u32 nr_commands;
+   u32 *cc_attrs_tbl;
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
@@ -555,4 +569,5 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 
shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm2_probe(struct tpm_chip *chip);
 ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
+int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 #endif
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 10f97e6..7a56f44 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -997,6 +997,70 @@ int tpm2_probe(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm2_probe);
 
+static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
+{
+   struct tpm_buf buf;
+   u32 nr_commands;
+   u32 *attrs;
+   u32 cc;
+   int i;
+   int rc;
+
+   rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, _commands, NULL);
+   if (rc)
+   goto out;
+
+   if (nr_commands > 0xF) {
+   rc = -EFAULT;
+   goto out;
+   }
+
+   chip->cc_attrs_tbl = 

[tpmdd-devel] [PATCH v2 0/7] in-kernel resource manager

[Jarkko Sakkinen]

This patch set adds support for TPM spaces that provide an isolated
execution context for transient objects and HMAC and policy sessions. A
space is swapped into TPM volatile memory only when it is used and
swapped out after the use.

There's a test script for trying out TPM spaces in

  git://git.infradead.org/users/jjs/tpm2-scripts.git

A simple smoke test suite can be run by

  sudo python -m unittest -v tpm2_smoke.SpaceTest   

v2:
* Substitute virtual handle in ContextSave.
* Substitute virtual handles in GetCapability.
* Validate that the real response length and the one reported in the
  header match in tpm_transmit().

James Bottomley (3):
  tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c
  tpm: expose spaces via a device link /dev/tpms
  tpm2: add session handle context saving and restoring to the space
code

Jarkko Sakkinen (4):
  tpm: move length validation to tpm_transmit()
  tpm: validate TPM 2.0 commands
  tpm: export tpm2_flush_context_cmd
  tpm: infrastructure for TPM spaces

 drivers/char/tpm/Makefile |   3 +-
 drivers/char/tpm/tpm-chip.c   |  86 +-
 drivers/char/tpm/tpm-dev-common.c | 148 +++
 drivers/char/tpm/tpm-dev.c| 143 +-
 drivers/char/tpm/tpm-dev.h|  27 ++
 drivers/char/tpm/tpm-interface.c  | 131 +++---
 drivers/char/tpm/tpm-sysfs.c  |   2 +-
 drivers/char/tpm/tpm.h|  49 +++-
 drivers/char/tpm/tpm2-cmd.c   | 168 
 drivers/char/tpm/tpm2-space.c | 532 ++
 drivers/char/tpm/tpms-dev.c   |  65 +
 11 files changed, 1135 insertions(+), 219 deletions(-)
 create mode 100644 drivers/char/tpm/tpm-dev-common.c
 create mode 100644 drivers/char/tpm/tpm-dev.h
 create mode 100644 drivers/char/tpm/tpm2-space.c
 create mode 100644 drivers/char/tpm/tpms-dev.c

-- 
2.9.3


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH v2 1/7] tpm: move length validation to tpm_transmit()

[Jarkko Sakkinen]

Check that the length matches the length reported by the response
header already in tpm_transmit() to improve validation.

Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm-interface.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bd2128e..708d356 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -343,6 +343,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 unsigned int flags)
 {
+   const struct tpm_output_header *header = (void *)buf;
ssize_t rc;
u32 count, ordinal;
unsigned long stop;
@@ -406,9 +407,18 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, 
size_t bufsiz,
 
 out_recv:
rc = chip->ops->recv(chip, (u8 *) buf, bufsiz);
-   if (rc < 0)
+   if (rc < 0) {
dev_err(>dev,
"tpm_transmit: tpm_recv: error %zd\n", rc);
+   goto out;
+   } else if (rc < TPM_HEADER_SIZE) {
+   rc = -EFAULT;
+   goto out;
+   }
+
+   if (rc != be32_to_cpu(header->length))
+   goto out;
+
 out:
if (chip->dev.parent)
pm_runtime_put_sync(chip->dev.parent);
@@ -438,19 +448,13 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const 
void *buf,
 size_t bufsiz, size_t min_rsp_body_length,
 unsigned int flags, const char *desc)
 {
-   const struct tpm_output_header *header;
+   const struct tpm_output_header *header = buf;
int err;
ssize_t len;
 
len = tpm_transmit(chip, (const u8 *)buf, bufsiz, flags);
if (len <  0)
return len;
-   else if (len < TPM_HEADER_SIZE)
-   return -EFAULT;
-
-   header = buf;
-   if (len != be32_to_cpu(header->length))
-   return -EFAULT;
 
err = be32_to_cpu(header->return_code);
if (err != 0 && desc)
-- 
2.9.3


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Jason Gunthorpe]

On Thu, Feb 16, 2017 at 06:40:02PM +, Davide Guerri wrote:
>Sorry I missed 1 line:
> 
>[20417.678952] ACPI resource is [mem 0xfed4-0xfed4087f flags 0x200]

The BIOS is broken.. That range declared in the ACPI is too small

Or the cmd_size is too big:

>[20417.678990] map request is is [mem 0xfed40080-0xfed40fff flags 0x200]

I have no idea which.

If we trust the ACPI table then the cmd_size is 2047 bytes

If we trust the register then it is 3967..

Try this?

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index a7c870af916c3d..2d16cc4aa0f43b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -233,6 +233,8 @@ static void __iomem *crb_map_res(struct device *dev, struct 
crb_priv *priv,
.flags  = IORESOURCE_MEM,
};
 
+   printk("map request is is %pr\n",_res);
+
/* Detect a 64 bit address on a 32 bit system */
if (start != new_res.start)
return (void __iomem *) ERR_PTR(-EINVAL);
@@ -243,6 +245,26 @@ static void __iomem *crb_map_res(struct device *dev, 
struct crb_priv *priv,
return priv->iobase + (new_res.start - io_res->start);
 }
 
+/*
+ * Work around broken BIOSs that return inconsistent values from the ACPI
+ * region vs the registers. Trust the ACPI region.
+ */
+static u64 crb_clamp_cmd_size(struct device *dev, struct resource *io_res,
+ u64 start, u64 size)
+{
+   if (io_res->start > start || io_res->end < start)
+   return 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;
+}
+
 static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
  struct acpi_table_tpm2 *buf)
 {
@@ -267,6 +289,8 @@ static int crb_map_io(struct acpi_device *device, struct 
crb_priv *priv,
return -EINVAL;
}
 
+   printk("ACPI resource is %pr\n",_res);
+
priv->iobase = devm_ioremap_resource(dev, _res);
if (IS_ERR(priv->iobase))
return PTR_ERR(priv->iobase);
@@ -278,14 +302,16 @@ static int crb_map_io(struct acpi_device *device, struct 
crb_priv *priv,
 
cmd_pa = ((u64) ioread32(>cca->cmd_pa_high) << 32) |
  (u64) ioread32(>cca->cmd_pa_low);
-   cmd_size = ioread32(>cca->cmd_size);
+   cmd_size = crb_clamp_cmd_size(dev, _res, cmd_pa,
+ ioread32(>cca->cmd_size));
priv->cmd = crb_map_res(dev, priv, _res, cmd_pa, cmd_size);
if (IS_ERR(priv->cmd))
return PTR_ERR(priv->cmd);
 
memcpy_fromio(_pa, >cca->rsp_pa, 8);
rsp_pa = le64_to_cpu(rsp_pa);
-   rsp_size = ioread32(>cca->rsp_size);
+   rsp_size = crb_clamp_cmd_size(dev, _res, rsp_pa,
+ ioread32(>cca->rsp_size));
 
if (cmd_pa != rsp_pa) {
priv->rsp = crb_map_res(dev, priv, _res, rsp_pa, rsp_size);

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Davide Guerri]

No lines including the requested range:

fed1-fed17fff : pnp 00:06

fed18000-fed18fff : pnp 00:06

fed19000-fed19fff : pnp 00:06

fed2-fed3 : pnp 00:06

fed4-fed4087f : MSFT0101:00

fed45000-fed8 : pnp 00:06

fed9-fed93fff : pnp 00:06

fee0-fee00fff : Local APIC

This is the NUC I am using, if that can be useful.

root@vhsv1:~# cat /sys/class/dmi/id/board_name
/sys/class/dmi/id/board_version

NUC6i7KYB

H90766-404

I am compiling the module right as we speak, I will get back to you soon.

On 16 February 2017 at 18:19, Jason Gunthorpe <
jguntho...@obsidianresearch.com> wrote:

> On Thu, Feb 16, 2017 at 06:10:43PM +, Davide Guerri wrote:
> >Hey thanks for the prompt reply.
> >I think you are interested in this:
> >   fed4-fed4087f : MSFT0101:00
>
> Are there more lines below that?
>
> Can you apply this patch and report what the results are?
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index a7c870af916c3d..acc54a03d6025d 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -233,6 +233,8 @@ static void __iomem *crb_map_res(struct device *dev,
> struct crb_priv *priv,
> .flags  = IORESOURCE_MEM,
> };
>
> +   printk("map request is is %pr\n",_res);
> +
> /* Detect a 64 bit address on a 32 bit system */
> if (start != new_res.start)
> return (void __iomem *) ERR_PTR(-EINVAL);
> @@ -267,6 +269,8 @@ static int crb_map_io(struct acpi_device *device,
> struct crb_priv *priv,
> return -EINVAL;
> }
>
> +   printk("ACPI resource is %pr\n",_res);
> +
> priv->iobase = devm_ioremap_resource(dev, _res);
> if (IS_ERR(priv->iobase))
> return PTR_ERR(priv->iobase);
>



-- 


[image: Davide Guerri on about.me]

Davide Guerri
about.me/davide_guerri
  
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Jason Gunthorpe]

On Thu, Feb 16, 2017 at 06:10:43PM +, Davide Guerri wrote:
>Hey thanks for the prompt reply.
>I think you are interested in this:
>   fed4-fed4087f : MSFT0101:00

Are there more lines below that?

Can you apply this patch and report what the results are?

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index a7c870af916c3d..acc54a03d6025d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -233,6 +233,8 @@ static void __iomem *crb_map_res(struct device *dev, struct 
crb_priv *priv,
.flags  = IORESOURCE_MEM,
};
 
+   printk("map request is is %pr\n",_res);
+
/* Detect a 64 bit address on a 32 bit system */
if (start != new_res.start)
return (void __iomem *) ERR_PTR(-EINVAL);
@@ -267,6 +269,8 @@ static int crb_map_io(struct acpi_device *device, struct 
crb_priv *priv,
return -EINVAL;
}
 
+   printk("ACPI resource is %pr\n",_res);
+
priv->iobase = devm_ioremap_resource(dev, _res);
if (IS_ERR(priv->iobase))
return PTR_ERR(priv->iobase);

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Davide Guerri]

Hey thanks for the prompt reply.

I think you are interested in this:

  fed4-fed4087f : MSFT0101:00


Thanks!

On 16 February 2017 at 18:01, Jason Gunthorpe <
jguntho...@obsidianresearch.com> wrote:

> On Thu, Feb 16, 2017 at 11:15:41AM +, Davide Guerri wrote:
> >Hello guys,
> >I am trying to get the fTPM on an Intel NUC working bit I get this
> >error
> >
> >[ 3.191376] tpm_crb MSFT0101:00: can't request region for resource
> >[mem 0xfed40080-0xfed40fff]
>
> What does /proc/iomem say?
>
> Jason
>



-- 


[image: Davide Guerri on about.me]

Davide Guerri
about.me/davide_guerri
  
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Jason Gunthorpe]

On Thu, Feb 16, 2017 at 11:15:41AM +, Davide Guerri wrote:
>Hello guys,
>I am trying to get the fTPM on an Intel NUC working bit I get this
>error
> 
>[ 3.191376] tpm_crb MSFT0101:00: can't request region for resource
>[mem 0xfed40080-0xfed40fff]

What does /proc/iomem say?

Jason

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[kbuild test robot]

Hi Peter,

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.10-rc8 next-20170216]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Peter-Huewe/Fix-whole-native-SPI-TPM-driver/20170217-010419
config: x86_64-randconfig-x001-201707 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/char/tpm/tpm_tis_spi.c: In function 'tpm_tis_spi_transfer':
>> drivers/char/tpm/tpm_tis_spi.c:135:9: warning: 'ret' may be used 
>> uninitialized in this function [-Wmaybe-uninitialized]
 return ret;
^~~

vim +/ret +135 drivers/char/tpm/tpm_tis_spi.c

dae9219c Peter Huewe   2017-02-16  119  spi_xfer.tx_buf 
= buffer;
dae9219c Peter Huewe   2017-02-16  120  spi_xfer.rx_buf 
= NULL;
dae9219c Peter Huewe   2017-02-16  121  }
0edbfea5 Christophe Ricard 2016-05-19  122  
0edbfea5 Christophe Ricard 2016-05-19  123  spi_message_init();
0edbfea5 Christophe Ricard 2016-05-19  124  
spi_message_add_tail(_xfer, );
0edbfea5 Christophe Ricard 2016-05-19  125  ret = 
spi_sync_locked(phy->spi_device, );
4dea582e Peter Huewe   2017-02-16  126  if (ret < 0)
4dea582e Peter Huewe   2017-02-16  127  goto exit;
4dea582e Peter Huewe   2017-02-16  128  
4dea582e Peter Huewe   2017-02-16  129  len -= transfer_len;
4dea582e Peter Huewe   2017-02-16  130  buffer += transfer_len;
4dea582e Peter Huewe   2017-02-16  131  }
0edbfea5 Christophe Ricard 2016-05-19  132  
0edbfea5 Christophe Ricard 2016-05-19  133  exit:
0edbfea5 Christophe Ricard 2016-05-19  134  
spi_bus_unlock(phy->spi_device->master);
0edbfea5 Christophe Ricard 2016-05-19 @135  return ret;
0edbfea5 Christophe Ricard 2016-05-19  136  }
0edbfea5 Christophe Ricard 2016-05-19  137  
dae9219c Peter Huewe   2017-02-16  138  static int 
tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
dae9219c Peter Huewe   2017-02-16  139u16 
len, u8 *result)
0edbfea5 Christophe Ricard 2016-05-19  140  {
dae9219c Peter Huewe   2017-02-16  141  return 
tpm_tis_spi_transfer(data, addr, len, result, 0x80);
0edbfea5 Christophe Ricard 2016-05-19  142  }
0edbfea5 Christophe Ricard 2016-05-19  143  

:: The code at line 135 was first introduced by commit
:: 0edbfea537d10c0de5505d0413368aad71027663 tpm/tpm_tis_spi: Add support 
for spi phy

:: TO: Christophe Ricard <christophe.ric...@gmail.com>
:: CC: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Testing the implementation with a Raspberry Pi 2 showed that under some
circumstances its SPI master erroneously releases the CS line before the
transfer is complete, i.e. before the end of the last clock. In this case
the TPM ignores the transfer and misses for example the GO command. The
driver is unable to detect this communication problem and will wait for a
command response that is never going to arrive, timing out eventually.

As a workaround, the small delay ensures that the CS line is held long
enough, even with a faulty SPI master. Other SPI masters are not affected,
except for a negligible performance penalty.

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index b50c5b072df3..685c51bf5d7e 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
 
spi_xfer.cs_change = 0;
spi_xfer.len = transfer_len;
+   spi_xfer.delay_usecs = 5;
 
if (direction) {
spi_xfer.tx_buf = NULL;
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
layers, as tpm_tis has no such limitation. Add a loop to hide that
limitation.

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_spi.c | 108 ++---
 1 file changed, 57 insertions(+), 51 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 16938e2253d2..b50c5b072df3 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
int ret;
-   struct spi_message m;
-   struct spi_transfer spi_xfer = {
-   .tx_buf = phy->tx_buf,
-   .rx_buf = phy->rx_buf,
-   .len = 4,
-   .cs_change = 1,
-   };
-
-   if (len > MAX_SPI_FRAMESIZE)
-   return -ENOMEM;
 
-   phy->tx_buf[0] = direction | (len - 1);
-   phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = addr >> 8;
-   phy->tx_buf[3] = addr;
+   spi_bus_lock(phy->spi_device->master);
 
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
+   while (len) {
+   struct spi_message m;
+   struct spi_transfer spi_xfer = {
+   .tx_buf = phy->tx_buf,
+   .rx_buf = phy->rx_buf,
+   .len = 4,
+   .cs_change = 1,
+   };
+   u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
+
+   phy->tx_buf[0] = direction | (transfer_len - 1);
+   phy->tx_buf[1] = 0xd4;
+   phy->tx_buf[2] = addr >> 8;
+   phy->tx_buf[3] = addr;
+
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
 
-   spi_bus_lock(phy->spi_device->master);
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
-   goto exit;
-
-   if ((phy->rx_buf[3] & 0x01) == 0) {
-   // handle SPI wait states
-   int i;
-
-   phy->tx_buf[0] = 0;
-
-   for (i = 0; i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
+   if ((phy->rx_buf[3] & 0x01) == 0) {
+   // handle SPI wait states
+   int i;
+
+   phy->tx_buf[0] = 0;
+
+   for (i = 0; i < TPM_RETRY; i++) {
+   spi_xfer.len = 1;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+   if (phy->rx_buf[0] & 0x01)
+   break;
+   }
+
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
goto exit;
-   if (phy->rx_buf[0] & 0x01)
-   break;
+   }
}
 
-   if (i == TPM_RETRY) {
-   ret = -ETIMEDOUT;
-   goto exit;
+   spi_xfer.cs_change = 0;
+   spi_xfer.len = transfer_len;
+
+   if (direction) {
+   spi_xfer.tx_buf = NULL;
+   spi_xfer.rx_buf = buffer;
+   } else {
+   spi_xfer.tx_buf = buffer;
+   spi_xfer.rx_buf = NULL;
}
-   }
 
-   spi_xfer.cs_change = 0;
-   spi_xfer.len = len;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
 
-   if (direction) {
-   spi_xfer.tx_buf = NULL;
-   spi_xfer.rx_buf = buffer;
-   } else {
-   spi_xfer.tx_buf = buffer;
-   spi_xfer.rx_buf = NULL;
+   len -= transfer_len;
+   buffer += transfer_len;
}
 
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-
 exit:
spi_bus_unlock(phy->spi_device->master);

[tpmdd-devel] [PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

During our testing it showed that unfortunately the whole native spi tpm driver
was more or less non-functional since it was merged, e.g. the wrong byte for
waitstate handling was used and transfers larger than 64 bytes did not work at 
all.

This was probably caused by the merging of the different approaches back then,
as the initial RFC patch did not have these problems, and also my sudden lack
of time/commitment back then.
I'm sorry that the final driver code went untested for that long time.

This patch set fixes these issues one by one.
In order to avoid duplication the read/write function was consolidated to one
transfer function, so we do not have to apply the same fix at two locations.
Maybe consider squashing it - we splitted it for easier review.
 
Affected Kernels: 4.8, 4.9, 4.10
Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)

Peter Huewe (5):
  tpm_tis_spi: Use single function to transfer data
  tpm_tis_spi: Abort transfer when too many wait states are signaled
  tpm_tis_spi: Check correct byte for wait state indicator
  tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes
  tpm_tis_spi: Add small delay after last transfer

 drivers/char/tpm/tpm_tis_spi.c | 163 +
 1 file changed, 69 insertions(+), 94 deletions(-)

-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 1/5] tpm_tis_spi: Use single function to transfer data

[Peter Huewe]

The algorithm for sending data to the TPM is mostly identical to the
algorithm for receiving data from the TPM, so a single function is
sufficient to handle both cases.

This is a prequisite for all the other fixes, so we don't have to fix
everything twice (send/receive)

Cc: 
[prerequisite for other fixes in this series]
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_spi.c | 87 --
 1 file changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 5292e5768a7e..6e1a3c43f621 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -47,8 +47,8 @@ struct tpm_tis_spi_phy {
struct tpm_tis_data priv;
struct spi_device *spi_device;
 
-   u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
-   u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
+   u8 tx_buf[4];
+   u8 rx_buf[4];
 };
 
 static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data 
*data)
@@ -56,8 +56,8 @@ static inline struct tpm_tis_spi_phy 
*to_tpm_tis_spi_phy(struct tpm_tis_data *da
return container_of(data, struct tpm_tis_spi_phy, priv);
 }
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
- u16 len, u8 *result)
+static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u8 len,
+   u8 *buffer, u8 direction)
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
int ret, i;
@@ -66,17 +66,17 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
.tx_buf = phy->tx_buf,
.rx_buf = phy->rx_buf,
.len = 4,
+   .cs_change = 1,
};
 
if (len > MAX_SPI_FRAMESIZE)
return -ENOMEM;
 
-   phy->tx_buf[0] = 0x80 | (len - 1);
+   phy->tx_buf[0] = direction | (len - 1);
phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-   phy->tx_buf[3] = addr & 0xFF;
+   phy->tx_buf[2] = addr >> 8;
+   phy->tx_buf[3] = addr;
 
-   spi_xfer.cs_change = 1;
spi_message_init();
spi_message_add_tail(_xfer, );
 
@@ -85,7 +85,7 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, 
u32 addr,
if (ret < 0)
goto exit;
 
-   memset(phy->tx_buf, 0, len);
+   phy->tx_buf[0] = 0;
 
/* According to TCG PTP specification, if there is no TPM present at
 * all, then the design has a weak pull-up on MISO. If a TPM is not
@@ -103,7 +103,14 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
 
spi_xfer.cs_change = 0;
spi_xfer.len = len;
-   spi_xfer.rx_buf = result;
+
+   if (direction) {
+   spi_xfer.tx_buf = NULL;
+   spi_xfer.rx_buf = buffer;
+   } else {
+   spi_xfer.tx_buf = buffer;
+   spi_xfer.rx_buf = NULL;
+   }
 
spi_message_init();
spi_message_add_tail(_xfer, );
@@ -114,62 +121,16 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
return ret;
 }
 
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+   return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
+}
+
 static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
   u16 len, u8 *value)
 {
-   struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-   int ret, i;
-   struct spi_message m;
-   struct spi_transfer spi_xfer = {
-   .tx_buf = phy->tx_buf,
-   .rx_buf = phy->rx_buf,
-   .len = 4,
-   };
-
-   if (len > MAX_SPI_FRAMESIZE)
-   return -ENOMEM;
-
-   phy->tx_buf[0] = len - 1;
-   phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-   phy->tx_buf[3] = addr & 0xFF;
-
-   spi_xfer.cs_change = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-
-   spi_bus_lock(phy->spi_device->master);
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
-   goto exit;
-
-   memset(phy->tx_buf, 0, len);
-
-   /* According to TCG PTP specification, if there is no TPM present at
-* all, then the design has a weak pull-up on MISO. If a TPM is not
-* present, a pull-up on MISO means that the SB controller sees a 1,
-* and will latch in 0xFF on the read.
-*/
-   for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = 

[tpmdd-devel] [PATCH 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Peter Huewe]

Abort the transfer with ETIMEDOUT when the TPM signals more than
TPM_RETRY wait states. Continuing with the transfer in this state
will only lead to arbitrary failures in other parts of the code.

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_spi.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 6e1a3c43f621..d782b9974c14 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
goto exit;
}
 
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
+   goto exit;
+   }
+
spi_xfer.cs_change = 0;
spi_xfer.len = len;
 
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Wait states are signaled in the last byte received from the TPM in
response to the header, not the first byte. Check rx_buf[3] instead of
rx_buf[0].

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_spi.c | 40 +---
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index d782b9974c14..16938e2253d2 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
u8 *buffer, u8 direction)
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-   int ret, i;
+   int ret;
struct spi_message m;
struct spi_transfer spi_xfer = {
.tx_buf = phy->tx_buf,
@@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
if (ret < 0)
goto exit;
 
-   phy->tx_buf[0] = 0;
-
-   /* According to TCG PTP specification, if there is no TPM present at
-* all, then the design has a weak pull-up on MISO. If a TPM is not
-* present, a pull-up on MISO means that the SB controller sees a 1,
-* and will latch in 0xFF on the read.
-*/
-   for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
+   if ((phy->rx_buf[3] & 0x01) == 0) {
+   // handle SPI wait states
+   int i;
+
+   phy->tx_buf[0] = 0;
+
+   for (i = 0; i < TPM_RETRY; i++) {
+   spi_xfer.len = 1;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+   if (phy->rx_buf[0] & 0x01)
+   break;
+   }
+
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
goto exit;
-   }
-
-   if (i == TPM_RETRY) {
-   ret = -ETIMEDOUT;
-   goto exit;
+   }
}
 
spi_xfer.cs_change = 0;
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH] tpm_tis_core: Choose appropriate timeout for reading burstcount

[Peter Huewe]

From: Alexander Steffen 

TIS v1.3 for TPM 1.2 and PTP for TPM 2.0 disagree about which timeout
value applies to reading a valid burstcount. It is TIMEOUT_D according to
TIS, but TIMEOUT_A according to PTP, so choose the appropriate value
depending on whether we deal with a TPM 1.2 or a TPM 2.0.

This is important since according to the PTP TIMEOUT_D is much smaller
than TIMEOUT_A. So the previous implementation could run into timeouts
with a TPM 2.0, even though the TPM was behaving perfectly fine.

During tpm2_probe TIMEOUT_D will be used even with a TPM 2.0, because
TPM_CHIP_FLAG_TPM2 is not yet set. This is fine, since the timeout values
will only be changed afterwards by tpm_get_timeouts. Until then
TIS_TIMEOUT_D_MAX applies, which is large enough.

Cc: sta...@vger.kernel.org
Fixes: aec04cbdf723 ("tpm: TPM 2.0 FIFO Interface")

Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c0f296b5d413..fc0e9a2734ed 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -160,8 +160,10 @@ static int get_burstcount(struct tpm_chip *chip)
u32 value;
 
/* wait for burstcount */
-   /* which timeout value, spec has 2 answers (c & d) */
-   stop = jiffies + chip->timeout_d;
+   if (chip->flags & TPM_CHIP_FLAG_TPM2)
+   stop = jiffies + chip->timeout_a;
+   else
+   stop = jiffies + chip->timeout_d;
do {
rc = tpm_tis_read32(priv, TPM_STS(priv->locality), );
if (rc < 0)
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

[Michal Suchánek]

On Wed, 1 Feb 2017 23:40:33 -0500
Vicky  wrote:

> > On Jan 26, 2017, at 5:58 PM, Ashley Lai 
> > wrote:
> > 
> > Adding Vicky from IBM.
> > 
> > 
> > On 01/26/2017 04:05 PM, Jason Gunthorpe wrote:  
> >> On Thu, Jan 26, 2017 at 09:22:48PM +0100, Michal Such??nek wrote:
> >>   
> >>> This is repeated a few times in the driver so I added memset to
> >>> quiet gcc and make behavior deterministic in case the unused
> >>> fields get some meaning in the future.  
> >> Yep, reserved certainly needs to be zeroed.. Can you send a patch?
> >> memset is overkill...
> >>   
> >>> However, in tpm_ibmvtpm_send the structure is initialized as
> >>> 
> >>>   struct ibmvtpm_crq crq;
> >>> __be64 *word = (__be64 *)
> >>> ...
> >>> crq.valid = (u8)IBMVTPM_VALID_CMD;
> >>> crq.msg = (u8)VTPM_TPM_COMMAND;
> >>> crq.len = cpu_to_be16(count);
> >>> crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
> >>> 
> >>> and submitted with
> >>> 
> >>>   rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
> >>>   be64_to_cpu(word[1]));
> >>> meaning it is swapped twice.  
> >> No idea, Nayna may know.
> >> 
> >> My guess is that '__be64 *word' should be 'u64 *word'...
> >> 
> >> Jason  
> >   
> 
> I don’t think we want ‘word' to be changed back to be of type
> ‘u64’.   Please see commit 62dfd912ab3b5405b6fe72d0135c37e9648071f1

The word is marked correctly as __be64 in that patch because count and
handle are swapped to BE when saved to it and the whole word is then
swapped again when loaded. If you just load ((u64)IBMVTPM_VALID_CMD <<
56 | ((u64)VTPM_TPM_COMMAND << 48) | ((u64)count << 32) |
ibmvtpm->rtce_dma_handle in a register it works equally well
without any __be and swaps involved.

Note however that __be64 and u64 are all the same to the compiler. It's
just a note for the reader and analysis tools.

Thanks

Michal

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

[Michael Ellerman]

Tyrel Datwyler  writes:
> On 01/29/2017 08:32 PM, Michael Ellerman wrote:
>> Tyrel Datwyler  writes:
>>>
>>> Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
>>> ---
>>> Word0 | Valid | Type  | Length|  Data
>>> ---
>>> Word1 | Reserved
>>> ---
>>>
>>> The following definition looks to match:
>>>
>>> struct ibmvtpm_crq {
>>> u8 valid;
>>> u8 msg;
>>> __be16 len;
>>> __be32 data;
>>> __be64 reserved;
>>> } __attribute__((packed, aligned(8)));
>> 
>> Well it's a partial match.
>> 
>> Your layout above doesn't define which byte of Length or Data is the MSB
>> or LSB. So going by that we still don't know the endianness of either
>
> I should have been explicit that PAPR uses Big Endian bit and byte
> numbering throughout the spec unless otherwise noted.

OK. I didn't actually remember that so yeah good to be explicit.

cheers

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH] tpm: ibmvtpm: simplify crq initialization and document crq format

[Michal Suchanek]

The crq is passed in registers so is the same on BE and LE hosts.
However, current implementation allocates a structure on-stack to
represent the crq, initializes the members swapping them to BE, and
loads the structure swapping it from BE. This is pointless and causes
GCC warnings about ununitialized members. Get rid of the structure and
the warnings.

Signed-off-by: Michal Suchanek 
---
 drivers/char/tpm/tpm_ibmvtpm.c | 92 +-
 1 file changed, 56 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 946025a7413b..b6402b1b76fe 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -39,18 +39,58 @@ static struct vio_device_id tpm_ibmvtpm_device_table[] = {
 MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
 
 /**
+ *
+ * ibmvtpm_send_crq_word - Send a CRQ request
+ * @vdev:  vio device struct
+ * @w1:pre-constructed first word of tmp crq (second word is 
reserved)
+ *
+ * Return value:
+ * 0 - Success
+ * Non-zero - Failure
+ */
+static int ibmvtpm_send_crq_word(struct vio_dev *vdev, u64 w1)
+{
+   return plpar_hcall_norets(H_SEND_CRQ, vdev->unit_address, w1, 0);
+}
+
+/**
+ *
  * ibmvtpm_send_crq - Send a CRQ request
  * @vdev:  vio device struct
- * @w1:first word
- * @w2:second word
+ *
+ * The ibmvtpm crq is defined as follows:
+ *
+ * Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
+ * ---
+ * Word0 | Valid | Type  | Length|  Data
+ * ---
+ * Word1 |Reserved
+ * ---
+ *
+ * Which matches the following structure (on bigendian host):
+ *
+ * struct ibmvtpm_crq {
+ * u8 valid;
+ * u8 msg;
+ * __be16 len;
+ * __be32 data;
+ * __be64 reserved;
+ * } __attribute__((packed, aligned(8)));
+ *
+ * However, the value is passed in a register so just compute the numeric value
+ * to load into the registar avoiding byteswap altogether. Endian only affects
+ * memory loads and stores - registers are internally represented the same.
  *
  * Return value:
- * 0 -Sucess
+ * 0 (H_SUCCESS) - Success
  * Non-zero - Failure
  */
-static int ibmvtpm_send_crq(struct vio_dev *vdev, u64 w1, u64 w2)
+static int ibmvtpm_send_crq(struct vio_dev *vdev,
+   u8 valid, u8 msg, u16 len, u32 data)
 {
-   return plpar_hcall_norets(H_SEND_CRQ, vdev->unit_address, w1, w2);
+   u64 w1 = ((u64)valid << 56) | ((u64)msg << 48) | ((u64)len << 32) |
+   (u64)data;
+   return ibmvtpm_send_crq_word(vdev, w1);
 }
 
 /**
@@ -106,8 +146,6 @@ static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, 
size_t count)
 static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 {
struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(>dev);
-   struct ibmvtpm_crq crq;
-   __be64 *word = (__be64 *)
int rc, sig;
 
if (!ibmvtpm->rtce_buf) {
@@ -134,10 +172,6 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 
*buf, size_t count)
spin_lock(>rtce_lock);
ibmvtpm->res_len = 0;
memcpy((void *)ibmvtpm->rtce_buf, (void *)buf, count);
-   crq.valid = (u8)IBMVTPM_VALID_CMD;
-   crq.msg = (u8)VTPM_TPM_COMMAND;
-   crq.len = cpu_to_be16(count);
-   crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
 
/*
 * set the processing flag before the Hcall, since we may get the
@@ -145,8 +179,9 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, 
size_t count)
 */
ibmvtpm->tpm_processing_cmd = true;
 
-   rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
- be64_to_cpu(word[1]));
+   rc = ibmvtpm_send_crq(ibmvtpm->vdev,
+   IBMVTPM_VALID_CMD, VTPM_TPM_COMMAND,
+   count, ibmvtpm->rtce_dma_handle);
if (rc != H_SUCCESS) {
dev_err(ibmvtpm->dev, "tpm_ibmvtpm_send failed rc=%d\n", rc);
rc = 0;
@@ -178,15 +213,10 @@ static u8 tpm_ibmvtpm_status(struct tpm_chip *chip)
  */
 static int ibmvtpm_crq_get_rtce_size(struct ibmvtpm_dev *ibmvtpm)
 {
-   struct ibmvtpm_crq crq;
-   u64 *buf = (u64 *) 
int rc;
 
-   crq.valid = (u8)IBMVTPM_VALID_CMD;
-   crq.msg = (u8)VTPM_GET_RTCE_BUFFER_SIZE;
-
-   rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
- cpu_to_be64(buf[1]));
+   rc = ibmvtpm_send_crq(ibmvtpm->vdev,
+   IBMVTPM_VALID_CMD, VTPM_GET_RTCE_BUFFER_SIZE, 0, 0);
if (rc != H_SUCCESS)
dev_err(ibmvtpm->dev,
"ibmvtpm_crq_get_rtce_size failed rc=%d\n", 

[tpmdd-devel] Intel NUC and fTPM issue on 4.9.2

[Davide Guerri]

Hello guys,
I am trying to get the fTPM on an Intel NUC working bit I get this error

[3.191376] tpm_crb MSFT0101:00: can't request region for resource [mem
0xfed40080-0xfed40fff]

[3.200378] tpm_crb: probe of MSFT0101:00 failed with error -16

and, of course, the TPM device is not registered in dev/sysfs

In my understanding this is a regression as it has been already fixed in
the past and merged in 4.7 or 4.8.
I also verified my source and tpm_crb.c contains this patch:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=422eac3f7deae34dbaffd08e03e27f37a5394a56

Is that a known issue? If so, are there workaround/patches?

Thanks in advance,
 Davide.
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

[David Laight]

From: Michal Suchánek
> Sent: 02 February 2017 11:30
...
> The word is marked correctly as __be64 in that patch because count and
> handle are swapped to BE when saved to it and the whole word is then
> swapped again when loaded. If you just load ((u64)IBMVTPM_VALID_CMD <<
> 56 | ((u64)VTPM_TPM_COMMAND << 48) | ((u64)count << 32) |
> ibmvtpm->rtce_dma_handle in a register it works equally well
> without any __be and swaps involved.

And that version will almost certainly generate much better code.

David

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel