| 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
| ||
|
Message-ID: <20170405133724.gfmyopgtpbrgycxl@intel.com>
Date: Wed, 5 Apr 2017 16:37:24 +0300
From: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To: Peter Huewe <peterhuewe@....de>
Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Andrew Lunn <andrew@...n.ch>,
tpmdd-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
wsa@...-dreams.de, Bryan Freed <bfreed@...omium.org>
Subject: Re: [PATCH v3] tpm: Apply a sane minimum adapterlimit value for
retransmission.
On Wed, Apr 05, 2017 at 12:05:32PM +0200, Peter Huewe wrote:
>
>
> Am 5. April 2017 11:03:27 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>:
> >On Tue, Mar 28, 2017 at 05:29:38PM +0200, 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.
> >>
> >> 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 v2:
> >> - Do not remove faster transfers when chip is SLB9645 (Peter Huewe)
> >> - Remember the adapterlimit length once it fails to not generate
> >extra
> >> i2c core messages (suggested by Andrew Lunn)
> >> 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 | 76
> >+++++++++++++++++++++++++++----------
> >> 1 file changed, 56 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
> >b/drivers/char/tpm/tpm_i2c_infineon.c
> >> index 62ee44e..fdefcdb 100644
> >> --- a/drivers/char/tpm/tpm_i2c_infineon.c
> >> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> >> @@ -70,6 +70,7 @@ struct tpm_inf_dev {
> >> u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
> >> struct tpm_chip *chip;
> >> enum i2c_chip_type chip_type;
> >> + unsigned int adapterlimit;
> >> };
> >>
> >> static struct tpm_inf_dev tpm_dev;
> >> @@ -111,6 +112,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
> >size_t len)
> >>
> >> int rc = 0;
> >> int count;
> >> + unsigned int msglen = len;
> >>
> >> /* Lock the adapter for the duration of the whole sequence. */
> >> if (!tpm_dev.client->adapter->algo->master_xfer)
> >> @@ -131,27 +133,61 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
> >size_t len)
> >> usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> >> }
> >> } else {
> >> - /* 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 */
> >> -
> >> - usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> >> - }
> >> -
> >> - if (rc <= 0)
> >> - goto out;
> >> -
> >> - /* After the TPM has successfully received the register address
> >> - * it needs some time, thus we're sleeping here again, before
> >> - * retrieving the data
> >> + /* Expect to send one command message and one data message, but
> >> + * support looping over each or both if necessary.
> >> */
> >> - for (count = 0; count < MAX_COUNT; count++) {
> >> - usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> >> - rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
> >> - if (rc > 0)
> >> - break;
> >> + 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 */
> >> +
> >> + usleep_range(SLEEP_DURATION_LOW,
> >> + SLEEP_DURATION_HI);
> >> + }
> >> +
> >> + if (rc <= 0)
> >> + goto out;
> >> +
> >> + /* After the TPM has successfully received the register
> >> + * address it needs some time, thus we're sleeping here
> >> + * again, before retrieving the data
> >> + */
> >> + for (count = 0; count < MAX_COUNT; count++) {
> >> + if (tpm_dev.adapterlimit) {
> >> + msglen = min_t(unsigned int,
> >> + tpm_dev.adapterlimit,
> >> + len);
> >> + msg2.len = msglen;
> >> + }
> >> + usleep_range(SLEEP_DURATION_LOW,
> >> + SLEEP_DURATION_HI);
> >> + rc = __i2c_transfer(tpm_dev.client->adapter,
> >> + &msg2, 1);
> >> + 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)
> >> + tpm_dev.adapterlimit =
> >> + I2C_SMBUS_BLOCK_MAX;
> >> + }
> >> +
> >> + if (rc <= 0)
> >> + goto out;
> >> }
> >> }
> >>
> >> --
> >> 2.9.3
> >>
> >
> >Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> >
> >Peter, Andrew, anyone: Tested-by?
> >
>
> Not yet, I'll put it on my list to test.
> Hopefully by next tuesday.
> Peter
Ok, thanks. I can push this to my master branch if that would help
to ease the testig?
/Jarkko
Powered by blists - more mailing lists