| Message ID | 20191203054619.20068-3-oohall@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] hw/p8-i2c: Don't print warnings when dumping registers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (d75e82dbfbb9443efeb3f9a5921ac23605aab469) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Mon, Dec 2, 2019, at 9:46 PM, Oliver O'Halloran wrote: > Our favourite TPM is capable of listening on multiple I2C bus addresses > and although this feature is supposed to be disabled by default we have > some systems in the wild where the TPM appears to be listening on these > secondary addresses. > > The secondary addresses are also susceptible to the bus-lockup problem > that we see with certain traffic patterns to the "main" TPM address. > We don't know what addresses the TPM might be listening on it's best to > take a conservitve approach and only allow traffic to I2C bus addresses > that we are explicitly told about by firmware. > > This is only required on the TPM bus, so this patch extends the existing > TPM workaround to also check that a DT node exists for any I2C bus > address the OS wants to talk to. If there isn't one, we don't forward > the I2C request to the bus and return an I2C timeout error to the OS. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> The gift that keeps on giving. Acked-by: Stewart Smith <stewart@flamingspork.com>
On 12/3/19 12:46 AM, Oliver O'Halloran wrote: > Our favourite TPM is capable of listening on multiple I2C bus addresses > and although this feature is supposed to be disabled by default we have > some systems in the wild where the TPM appears to be listening on these > secondary addresses. > > The secondary addresses are also susceptible to the bus-lockup problem > that we see with certain traffic patterns to the "main" TPM address. > We don't know what addresses the TPM might be listening on it's best to > take a conservitve approach and only allow traffic to I2C bus addresses > that we are explicitly told about by firmware. > > This is only required on the TPM bus, so this patch extends the existing > TPM workaround to also check that a DT node exists for any I2C bus > address the OS wants to talk to. If there isn't one, we don't forward > the I2C request to the bus and return an I2C timeout error to the OS. I think the userspace will talk to kernel tpm driver, which in turn will talk to Physical TPM via skiboot i2c driver. I don't think the userspace will ever reach the skiboot tpm driver. I am wondering if the fix has to be handled in the skiboot core i2c driver rather than in tpm_i2c_nuvoton driver. Am I missing something ? Thanks & Regards, - Nayna
On Tue, Dec 10, 2019 at 12:46 PM Nayna <nayna@linux.vnet.ibm.com> wrote: > > > On 12/3/19 12:46 AM, Oliver O'Halloran wrote: > > Our favourite TPM is capable of listening on multiple I2C bus addresses > > and although this feature is supposed to be disabled by default we have > > some systems in the wild where the TPM appears to be listening on these > > secondary addresses. > > > > The secondary addresses are also susceptible to the bus-lockup problem > > that we see with certain traffic patterns to the "main" TPM address. > > We don't know what addresses the TPM might be listening on it's best to > > take a conservitve approach and only allow traffic to I2C bus addresses > > that we are explicitly told about by firmware. > > > > This is only required on the TPM bus, so this patch extends the existing > > TPM workaround to also check that a DT node exists for any I2C bus > > address the OS wants to talk to. If there isn't one, we don't forward > > the I2C request to the bus and return an I2C timeout error to the OS. > > I think the userspace will talk to kernel tpm driver, which in turn will > talk to Physical TPM via skiboot i2c driver. > > I don't think the userspace will ever reach the skiboot tpm driver. I > am wondering if the fix has to be handled in the skiboot core i2c driver > rather than in tpm_i2c_nuvoton driver. The TPM driver sets a quirk callback on the TPM's I2C bus for the I2C core to use. When we get an I2C request from the kernel via OPAL call the core will allow or block the request based on what the quirk functions returns. It's in the right place.
On 12/9/19 9:48 PM, Oliver O'Halloran wrote: > On Tue, Dec 10, 2019 at 12:46 PM Nayna <nayna@linux.vnet.ibm.com> wrote: >> >> On 12/3/19 12:46 AM, Oliver O'Halloran wrote: >>> Our favourite TPM is capable of listening on multiple I2C bus addresses >>> and although this feature is supposed to be disabled by default we have >>> some systems in the wild where the TPM appears to be listening on these >>> secondary addresses. >>> >>> The secondary addresses are also susceptible to the bus-lockup problem >>> that we see with certain traffic patterns to the "main" TPM address. >>> We don't know what addresses the TPM might be listening on it's best to >>> take a conservitve approach and only allow traffic to I2C bus addresses >>> that we are explicitly told about by firmware. >>> >>> This is only required on the TPM bus, so this patch extends the existing >>> TPM workaround to also check that a DT node exists for any I2C bus >>> address the OS wants to talk to. If there isn't one, we don't forward >>> the I2C request to the bus and return an I2C timeout error to the OS. >> I think the userspace will talk to kernel tpm driver, which in turn will >> talk to Physical TPM via skiboot i2c driver. >> >> I don't think the userspace will ever reach the skiboot tpm driver. I >> am wondering if the fix has to be handled in the skiboot core i2c driver >> rather than in tpm_i2c_nuvoton driver. > The TPM driver sets a quirk callback on the TPM's I2C bus for the I2C > core to use. When we get an I2C request from the kernel via OPAL call > the core will allow or block the request based on what the quirk > functions returns. It's in the right place. Aah ok.. Sure, thanks for the clarification. Thanks & Regards, - Nayna
diff --git a/libstb/drivers/tpm_i2c_nuvoton.c b/libstb/drivers/tpm_i2c_nuvoton.c index ef32b79fd6f7..83ec1c81e485 100644 --- a/libstb/drivers/tpm_i2c_nuvoton.c +++ b/libstb/drivers/tpm_i2c_nuvoton.c @@ -493,12 +493,46 @@ static struct tpm_driver tpm_i2c_nuvoton_driver = { static int nuvoton_tpm_quirk(void *data, struct i2c_request *req, int *rc) { struct tpm_dev *tpm_device = data; + struct dt_node *dev; + uint16_t addr; + bool found; - /* If we're doing i2cdetect on the TPM, pretent we just NACKed - * it due to errata in nuvoton firmware where if we let this - * request go through, it would steal the bus and you'd end up - * in a nice world of pain. + /* + * The nuvoton TPM firmware has a problem where a single byte read or + * zero byte write to one of its I2C addresses causes the TPM to lock + * up the bus. Once locked up the bus can only be recovered by power + * cycling the TPM. Unfortunately, we don't have the ability to + * power cycle the TPM because allowing it to be reset at runtime + * would undermine the TPM's security model (we can reset it and + * send it whatever measurements we like to unlock it's secrets). + * So the best we can do here is try avoid triggering the problem + * in the first place. + * + * For a bit of added fun the TPM also appears to check for traffic + * on a few different I2C bus addresses. It does this even when not + * configured to respond on those addresses so you can trigger the + * bug by sending traffic... somwhere. To work around this we block + * sending I2C requests on the TPM's bus unless the DT explicitly + * tells us there is a device there. */ + + /* first, check if this a known address */ + addr = req->dev_addr; + found = false; + + dt_for_each_child(req->bus->dt_node, dev) { + if (dt_prop_get_u32(dev, "reg") == addr) { + found = true; + break; + } + } + + if (!found) { + *rc = OPAL_I2C_TIMEOUT; + return 1; + } + + /* second, check if it's a bad transaction to the TPM */ if (tpm_device->bus_id == req->bus->opal_id && tpm_device->i2c_addr == req->dev_addr && ((req->op == I2C_READ && req->rw_len == 1) || @@ -517,6 +551,7 @@ void tpm_i2c_nuvoton_probe(void) struct tpm_dev *tpm_device = NULL; struct dt_node *node = NULL; struct i2c_bus *bus; + const char *name; dt_for_each_compatible(dt_root, node, "nuvoton,npct650") { if (!dt_node_is_enabled(node)) @@ -562,6 +597,10 @@ void tpm_i2c_nuvoton_probe(void) assert(bus->check_quirk == NULL); bus->check_quirk = nuvoton_tpm_quirk; bus->check_quirk_data = tpm_device; + name = dt_prop_get(node, "ibm,port-name"); + + prlog(PR_NOTICE, "NUVOTON: TPM I2C workaround applied to %s\n", + name); /* * Tweak for linux. It doesn't have a driver compatible
Our favourite TPM is capable of listening on multiple I2C bus addresses and although this feature is supposed to be disabled by default we have some systems in the wild where the TPM appears to be listening on these secondary addresses. The secondary addresses are also susceptible to the bus-lockup problem that we see with certain traffic patterns to the "main" TPM address. We don't know what addresses the TPM might be listening on it's best to take a conservitve approach and only allow traffic to I2C bus addresses that we are explicitly told about by firmware. This is only required on the TPM bus, so this patch extends the existing TPM workaround to also check that a DT node exists for any I2C bus address the OS wants to talk to. If there isn't one, we don't forward the I2C request to the bus and return an I2C timeout error to the OS. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- libstb/drivers/tpm_i2c_nuvoton.c | 47 +++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 4 deletions(-)