| 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
| ||
|
Date: Thu, 26 Nov 2020 12:30:00 +0200
From: Adrian Ratiu <adrian.ratiu@...labora.com>
To: Ezequiel Garcia <ezequiel@...labora.com>,
Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org, Peter Huewe <peterhuewe@....de>,
Jason Gunthorpe <jgg@...pe.ca>,
Helen Koike <helen.koike@...labora.com>,
Duncan Laurie <dlaurie@...omium.org>,
Stephen Boyd <swboyd@...omium.org>, kernel@...labora.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] char: tpm: add i2c driver for cr50
On Thu, 26 Nov 2020, Ezequiel Garcia <ezequiel@...labora.com>
wrote:
> On Thu, 2020-11-26 at 05:30 +0200, Jarkko Sakkinen wrote:
>> On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote:
>> > Hi Jarkko, Thanks for your review. On Tue, 2020-11-24 at
>> > 00:06 +0200, Jarkko Sakkinen wrote:
>> > > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu
>> > > wrote:
>> > > > From: "dlaurie@...omium.org" <dlaurie@...omium.org> Add
>> > > > TPM 2.0 compatible I2C interface for chips with cr50
>> > > > firmware. The firmware running on the currently
>> > > > supported H1 MCU requires a special driver to handle its
>> > > > specific protocol, and this makes it unsuitable to use
>> > > > tpm_tis_core_* and instead it must implement the
>> > > > underlying TPM protocol similar to the other I2C TPM
>> > > > drivers. - All 4 byes of status register must be
>> > > > read/written at once. - FIFO and burst count is limited
>> > > > to 63 and must be drained by AP. - Provides an interrupt
>> > > > to indicate when read response data is ready and when the
>> > > > TPM is finished processing write data. This driver is
>> > > > based on the existing infineon I2C TPM driver, which most
>> > > > closely matches the cr50 i2c protocol behavior. Cc:
>> > > > Helen Koike <helen.koike@...labora.com> Signed-off-by:
>> > > > Duncan Laurie <dlaurie@...omium.org>
>> > > > [swboyd@...omium.org: Depend on i2c even if it's a
>> > > > module, replace boilier plate with SPDX tag, drop
>> > > > asm/byteorder.h include, simplify return from probe]
>> > > > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
>> > > > Signed-off-by: Fabien Lahoudere
>> > > > <fabien.lahoudere@...labora.com> Signed-off-by: Adrian
>> > > > Ratiu <adrian.ratiu@...labora.com> --- Changes in v2:
>> > > > - Various small fixes all over (reorder includes,
>> > > > MAX_BUFSIZE,
>> > > > comments, etc)
>> > > > - Reworked return values of i2c_wait_tpm_ready() to fix
>> > > > timeout
>> > > > mis-handling so ret == 0 now means success, the wait
>> > > > period jiffies is ignored because that number is
>> > > > meaningless and return a proper timeout error in case
>> > > > jiffies == 0.
>> > > > - Make i2c default to 1 message per transfer (requested
>> > > > by
>> > > > Helen)
>> > > > - Move -EIO error reporting to transfer function to
>> > > > cleanup
>> > > > transfer() itself and its R/W callers
>> > > > - Remove magic value hardcodings and introduce enum
>> > > > force_release. v1 posted at
>> > > > https://lkml.org/lkml/2020/2/25/349 Applies on
>> > > > next-20201120, tested on Chromebook EVE. ---
>> > > > drivers/char/tpm/Kconfig | 10 +
>> > > > drivers/char/tpm/Makefile | 2 +
>> > > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
>> > > > ++++++++++++++++++++++++++++
>> > > > 3 files changed, 780 insertions(+) create mode 100644
>> > > > drivers/char/tpm/tpm_tis_i2c_cr50.c
>> > > > diff --git a/drivers/char/tpm/Kconfig
>> > > > b/drivers/char/tpm/Kconfig index
>> > > > a18c314da211..4308f9ca7a43 100644 ---
>> > > > a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig
>> > > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
>> > > > To compile this driver as a module, choose M
>> > > > here; the module will be called
>> > > > tpm_tis_synquacer.
>> > > >
>> > > > +config TCG_TIS_I2C_CR50 + tristate "TPM Interface
>> > > > Specification 2.0 Interface (I2C - CR50)" + depends
>> > > > on I2C + select TCG_CR50 + help +
>> > > > This is a driver for the Google cr50 I2C TPM interface
>> > > > which is a + custom microcontroller and requires
>> > > > a custom i2c protocol interface + to handle the
>> > > > limitations of the hardware. To compile this driver +
>> > > > as a module, choose M here; the module will be called
>> > > > tcg_tis_i2c_cr50. +
>> > > > config TCG_TIS_I2C_ATMEL
>> > > > tristate "TPM Interface Specification 1.2
>> > > > Interface (I2C
>> > > > - Atmel)"
>> > > > depends on I2C
>> > > > diff --git a/drivers/char/tpm/Makefile
>> > > > b/drivers/char/tpm/Makefile index
>> > > > 84db4fb3a9c9..66d39ea6bd10 100644 ---
>> > > > a/drivers/char/tpm/Makefile +++
>> > > > b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@
>> > > > obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
>> > > > tpm_tis_spi-y := tpm_tis_spi_main.o
>> > > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) +=
>> > > > tpm_tis_spi_cr50.o
>> > > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o +
>> > > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>> > > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>> > > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
>> > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
>> > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode
>> > > > 100644 index 000000000000..37555dafdca0 --- /dev/null +++
>> > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,768 @@
>> > > > +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright
>> > > > 2016 Google Inc. + * + * Based on Linux Kernel TPM
>> > > > driver by + * Peter Huewe <peter.huewe@...ineon.com> + *
>> > > > Copyright (C) 2011 Infineon Technologies + */ + +/* + *
>> > > > cr50 is a firmware for H1 secure modules that requires
>> > > > special + * handling for the I2C interface. + * + * -
>> > > > Use an interrupt for transaction status instead of
>> > > > hardcoded delays + * - Must use write+wait+read read
>> > > > protocol + * - All 4 bytes of status register must be
>> > > > read/written at once + * - Burst count max is 63 bytes,
>> > > > and burst count behaves + * slightly differently than
>> > > > other I2C TPMs + * - When reading from FIFO the full
>> > > > burstcnt must be read + * instead of just reading
>> > > > header and determining the remainder + */ + +#include
>> > > > <linux/acpi.h> +#include <linux/completion.h> +#include
>> > > > <linux/i2c.h> +#include <linux/interrupt.h> +#include
>> > > > <linux/module.h> +#include <linux/pm.h> +#include
>> > > > <linux/slab.h> +#include <linux/wait.h> + +#include
>> > > > "tpm_tis_core.h" + +#define CR50_MAX_BUFSIZE 64
>> > > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout
>> > > > during transactions */ +#define CR50_TIMEOUT_NOIRQ_MS 20
>> > > > /* Timeout for TPM ready without IRQ */ +#define
>> > > > CR50_I2C_DID_VID 0x00281ae0L +#define
>> > > > CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C
>> > > > errors */ +#define CR50_I2C_RETRY_DELAY_LO 55
>> > > > /* Min usecs between retries on I2C */ +#define
>> > > > CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs
>> > > > between retries on I2C */
>> > > CR50_ -> TPM_CR50_
>> > > > + +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
>> > > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
>> > > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
>> > > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4)) +
>> > > > +struct priv_data { + int irq; + int
>> > > > locality; + struct completion tpm_ready; + u8
>> > > > buf[CR50_MAX_BUFSIZE]; +};
>> > > tpm_i2c_cr50_priv_data
>> > > > + +enum force_release { + CR50_NO_FORCE = 0x0, +
>> > > > CR50_FORCE = 0x1, +};
>> > > I'd just #define TPM_I2C_CR50_NO_FORCE 0 #define
>> > > TPM_I2C_CR50_FORCE 1
>> > A proper enumerated type has advantages over a preprocessor
>> > macro: even if the compiler won't warn you, static analyzers
>> > can warn about a misuse.
>> Why don't you just use "bool", "true" and "false"? I ignored
>> that this has nothing to do with the hardware last time.
>
> Well, boolean parameters are a known anti-pattern [1].
>
> [1] https://people.mpi-inf.mpg.de/~jblanche/api-design.pdf
Funny because that's what I wrote a few days ago in my v3 WIP
branch, #defines and a bool function parameter. XD
As Jarkko correctly observed these values are unrelated to the HW,
we just need to distinguish FORCE vs NO_FORCE and IMO any method
will do just as well for such a small and obvious use case.
So based on the arguments given, I'll stop bikeshedding and just
use enums :)
Thank you both,
Adrian
Powered by blists - more mailing lists