linux-kernel - Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission.

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

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

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

Date:   Thu, 02 Mar 2017 14:43:45 +0100
From:   Peter Huewe <peterhuewe@....de>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>
CC:     Bryan Freed <bfreed@...omium.org>,
        tpmdd-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        wsa@...-dreams.de
Subject: Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission.



Am 2. März 2017 13:55:43 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>:
>On Wed, Mar 01, 2017 at 04:36:17PM +0100, Enric Balletbo i Serra wrote:
>> From: Bryan Freed <bfreed@...omium.org>
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail with -EOPNOTSUPP. Retry
>> them with a sane minimum size without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>
>What is 0x05 command?
0x05 is the address of the fifo.
I honestly think that it needs toll be repeated after a stop condition.
I'll look that up.

>
>/Jarkko
>
>> Signed-off-by: Bryan Freed <bfreed@...omium.org>
>> [rework the patch to adapt to the feedback received]
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
>> ---
>> This is a reworked version of the original patch based on the
>> suggestion made by Wolfram Sang to simply fall back to a sane minimum
>> when the maximum fails.
>> 
>> Changes since v1:
>>  - Check the correct return value (-EOPNOTSUPP instead of -EINVAL)
>>  - Fall back len to I2C_SMBUS_BLOCK_MAX if fails.
>> 
>>  drivers/char/tpm/tpm_i2c_infineon.c | 45
>+++++++++++++++++++++----------------
>>  1 file changed, 26 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..88bf947 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -107,39 +107,27 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  		.len = len,
>>  		.buf = buffer
>>  	};
>> -	struct i2c_msg msgs[] = {msg1, msg2};
>>  
>>  	int rc = 0;
>>  	int count;
>> +	unsigned int adapterlimit = len;
>>  
>>  	/* Lock the adapter for the duration of the whole sequence. */
>>  	if (!tpm_dev.client->adapter->algo->master_xfer)
>>  		return -EOPNOTSUPP;
>>  	i2c_lock_adapter(tpm_dev.client->adapter);
>>  
>> -	if (tpm_dev.chip_type == SLB9645) {
Why are you / bryan removing this code path here?
I put it there for a good reason (i.e. faster transfers)

Thanks,
Peter
>> -		/* use a combined read for newer chips
>> -		 * unfortunately the smbus functions are not suitable due to
>> -		 * the 32 byte limit of the smbus.
>> -		 * retries should usually not be needed, but are kept just to
>> -		 * be on the safe side.
>> -		 */
>> -		for (count = 0; count < MAX_COUNT; count++) {
>> -			rc = __i2c_transfer(tpm_dev.client->adapter, msgs, 2);
>> -			if (rc > 0)
>> -				break;	/* break here to skip sleep */
>> -			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -		}
>> -	} else {
>> +	/* Expect to send one command message and one data message, but
>> +	 * support looping over each or both if necessary.
>> +	 */
>> +	while (len > 0) {
>>  		/* slb9635 protocol should work in all cases */
>>  		for (count = 0; count < MAX_COUNT; count++) {
>>  			rc = __i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
>>  			if (rc > 0)
>> -				break;	/* break here to skip sleep */
>> -
>> +				break;
>>  			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  		}
>> -
>>  		if (rc <= 0)
>>  			goto out;
>>  
>> @@ -148,11 +136,30 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  		 * retrieving the data
>>  		 */
>>  		for (count = 0; count < MAX_COUNT; count++) {
>> +			unsigned int msglen = msg2.len =
>> +					min_t(unsigned int, adapterlimit, len);
>>  			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  			rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
>> -			if (rc > 0)
>> +			if (rc > 0) {
>> +				/* Since len is unsigned, make doubly sure we
>> +				 * do not underflow it.
>> +				 */
>> +				if (msglen > len)
>> +					len = 0;
>> +				else
>> +					len -= msglen;
>> +				msg2.buf += msglen;
>>  				break;
>> +			}
>> +			/* If the I2C adapter rejected the request (e.g when
>> +			 * the quirk read_max_len < len) fall back to a sane
>> +			 * minimum value and try again.
>> +			 */
>> +			if (rc == -EOPNOTSUPP)
>> +				adapterlimit = I2C_SMBUS_BLOCK_MAX;
>>  		}
>> +		if (rc <= 0)
>> +			goto out;
>>  	}
>>  
>>  out:
>> -- 
>> 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@...ts.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile