diff mbox series

tpm_i2c_nuvoton: check TPM vendor id register during probe

Message ID 20200305185430.31566-1-erichte@linux.ibm.com
State Superseded
Headers show
Series tpm_i2c_nuvoton: check TPM vendor id register during probe | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (82aed17a5468aff6b600ee1694a10a60f942c018)

Commit Message

Eric Richter March 5, 2020, 6:54 p.m. UTC
The driver for the nuvoton i2c TPM does not currently check if there is
a functional TPM at the bus and address given by the device tree.

This patch adds a simple check of the TPM vendor id register, compares
against the known expected value for the chip, skips registering it if
the chip is inaccessible or returns an unexpected id.

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
 libstb/drivers/tpm_i2c_nuvoton.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Klaus Heinrich Kiwi March 5, 2020, 9:16 p.m. UTC | #1
On 3/5/2020 3:54 PM, Eric Richter wrote:
> +		/* ensure there's really the TPM we expect at that address */
> +		if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
> +					 1, &vendor, sizeof(vendor)) ||
> +		    (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
if tpm_i2c_request_send() fails (negative error code), you skip this 
block and still tries to tpm_register_chip()?

> +			prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
> +			      "expected vendor id mismatch\n");
> +			if (vendor) {
> +				prlog(PR_ERR, "NUVOTON: expected 0x%X, "
> +				      "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
> +			}
> +			free(tpm_device);
> +			continue;
> +		}
>   		if (tpm_register_chip(node, tpm_device,
>   				      &tpm_i2c_nuvoton_driver)) {
>   			free(tpm_device);
> 

Perhaps registering it would then fail? Since tpm_register_chip() sounds 
both like an action as well as a predicate, perhaps be more explicit 
about what we are testing? (I know this is outside the proposed patch 
but still...)

  -Klaus
Oliver O'Halloran March 6, 2020, 2:34 a.m. UTC | #2
On Fri, Mar 6, 2020 at 5:55 AM Eric Richter <erichte@linux.ibm.com> wrote:
>
> The driver for the nuvoton i2c TPM does not currently check if there is
> a functional TPM at the bus and address given by the device tree.
>
> This patch adds a simple check of the TPM vendor id register, compares
> against the known expected value for the chip, skips registering it if
> the chip is inaccessible or returns an unexpected id.
>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>  libstb/drivers/tpm_i2c_nuvoton.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/libstb/drivers/tpm_i2c_nuvoton.c b/libstb/drivers/tpm_i2c_nuvoton.c
> index 3679ddaf..882018e4 100644
> --- a/libstb/drivers/tpm_i2c_nuvoton.c
> +++ b/libstb/drivers/tpm_i2c_nuvoton.c
> @@ -30,6 +30,7 @@
>  #define TPM_BURST_COUNT                0x01
>  #define TPM_DATA_FIFO_W                0x20
>  #define TPM_DATA_FIFO_R                0x40
> +#define TPM_VID_DID            0x60
>
>  /* Bit masks for the TPM STATUS register */
>  #define TPM_STS_VALID          0x80
> @@ -42,6 +43,8 @@
>  /* TPM Driver values */
>  #define MAX_STSVALID_POLLS     5
>  #define TPM_TIMEOUT_INTERVAL   10
> +#define TPM_650_VENDOR_ID      0x5010FE00

If only we knew the name of the vendor...

> +#define TPM_VENDOR_ID_MASK     0xFFFFFF00
>
>  static struct tpm_dev *tpm_device = NULL;
>
> @@ -552,6 +555,7 @@ void tpm_i2c_nuvoton_probe(void)
>         struct dt_node *node = NULL;
>         struct i2c_bus *bus;
>         const char *name;
> +       uint32_t vendor = 0;
>
>         dt_for_each_compatible(dt_root, node, "nuvoton,npct650") {
>                 if (!dt_node_is_enabled(node))
> @@ -588,6 +592,19 @@ void tpm_i2c_nuvoton_probe(void)
>                               "found, tpm node parent %p\n", node->parent);
>                         goto disable;
>                 }
> +               /* ensure there's really the TPM we expect at that address */
> +               if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
> +                                        1, &vendor, sizeof(vendor)) ||
> +                   (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
> +                       prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
> +                             "expected vendor id mismatch\n");
> +                       if (vendor) {
> +                               prlog(PR_ERR, "NUVOTON: expected 0x%X, "
> +                                     "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
> +                       }

The usual pattern for this is something like:

rc = tpm_i2c_request(...);
if (rc) {
    prlog("i2c failed");
    continue;
}

if (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
    prlog("vendor id mismatch")
    continue;
}

The control flow is cleaner, there's less indent stacking, and there's
no multi-line if conditionals which are always a mess. The way
tpm_i2c_nuvoton_probe() is structed makes it slightly awkward since
you need to duplicate the cleanup, but it's still better IMO.

> +                       free(tpm_device);
> +                       continue;
> +               }
>                 if (tpm_register_chip(node, tpm_device,
>                                       &tpm_i2c_nuvoton_driver)) {
>                         free(tpm_device);
> --
> 2.21.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Oliver O'Halloran March 6, 2020, 2:39 a.m. UTC | #3
On Fri, Mar 6, 2020 at 8:17 AM Klaus Heinrich Kiwi
<klaus@linux.vnet.ibm.com> wrote:
>
>
>
> On 3/5/2020 3:54 PM, Eric Richter wrote:
> > +             /* ensure there's really the TPM we expect at that address */
> > +             if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
> > +                                      1, &vendor, sizeof(vendor)) ||
> > +                 (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
> if tpm_i2c_request_send() fails (negative error code), you skip this
> block and still tries to tpm_register_chip()?

Any non-zero value is considered true so it'll go down the error path
if the I2C transaction fails. I'm not 100% sure I understand what
you're saying here though...

> > +                     prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
> > +                           "expected vendor id mismatch\n");
> > +                     if (vendor) {
> > +                             prlog(PR_ERR, "NUVOTON: expected 0x%X, "
> > +                                   "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
> > +                     }
> > +                     free(tpm_device);
> > +                     continue;
> > +             }

> >               if (tpm_register_chip(node, tpm_device,
> >                                     &tpm_i2c_nuvoton_driver)) {
> >                       free(tpm_device);
> >
>
> Perhaps registering it would then fail? Since tpm_register_chip() sounds
> both like an action as well as a predicate, perhaps be more explicit
> about what we are testing? (I know this is outside the proposed patch
> but still...)

It doesn't fail, that's the problem. Right now we just keep trucking
along until skiboot tries to write a measurement to the TPM at which
point we notice it's not there.

>
>   -Klaus
>
> --
> Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Oliver O'Halloran March 6, 2020, 2:40 a.m. UTC | #4
On Fri, Mar 6, 2020 at 1:34 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> On Fri, Mar 6, 2020 at 5:55 AM Eric Richter <erichte@linux.ibm.com> wrote:
> >
> > The driver for the nuvoton i2c TPM does not currently check if there is
> > a functional TPM at the bus and address given by the device tree.
> >
> > This patch adds a simple check of the TPM vendor id register, compares
> > against the known expected value for the chip, skips registering it if
> > the chip is inaccessible or returns an unexpected id.
> >
> > Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> > ---
> >  libstb/drivers/tpm_i2c_nuvoton.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/libstb/drivers/tpm_i2c_nuvoton.c b/libstb/drivers/tpm_i2c_nuvoton.c
> > index 3679ddaf..882018e4 100644
> > --- a/libstb/drivers/tpm_i2c_nuvoton.c
> > +++ b/libstb/drivers/tpm_i2c_nuvoton.c
> > @@ -30,6 +30,7 @@
> >  #define TPM_BURST_COUNT                0x01
> >  #define TPM_DATA_FIFO_W                0x20
> >  #define TPM_DATA_FIFO_R                0x40
> > +#define TPM_VID_DID            0x60
> >
> >  /* Bit masks for the TPM STATUS register */
> >  #define TPM_STS_VALID          0x80
> > @@ -42,6 +43,8 @@
> >  /* TPM Driver values */
> >  #define MAX_STSVALID_POLLS     5
> >  #define TPM_TIMEOUT_INTERVAL   10
> > +#define TPM_650_VENDOR_ID      0x5010FE00
>
> If only we knew the name of the vendor...
>
> > +#define TPM_VENDOR_ID_MASK     0xFFFFFF00
> >
> >  static struct tpm_dev *tpm_device = NULL;
> >
> > @@ -552,6 +555,7 @@ void tpm_i2c_nuvoton_probe(void)
> >         struct dt_node *node = NULL;
> >         struct i2c_bus *bus;
> >         const char *name;
> > +       uint32_t vendor = 0;
> >
> >         dt_for_each_compatible(dt_root, node, "nuvoton,npct650") {
> >                 if (!dt_node_is_enabled(node))
> > @@ -588,6 +592,19 @@ void tpm_i2c_nuvoton_probe(void)
> >                               "found, tpm node parent %p\n", node->parent);
> >                         goto disable;
> >                 }
> > +               /* ensure there's really the TPM we expect at that address */
> > +               if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
> > +                                        1, &vendor, sizeof(vendor)) ||
> > +                   (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
> > +                       prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
> > +                             "expected vendor id mismatch\n");
> > +                       if (vendor) {
> > +                               prlog(PR_ERR, "NUVOTON: expected 0x%X, "
> > +                                     "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
> > +                       }
>
> The usual pattern for this is something like:
>
> rc = tpm_i2c_request(...);
> if (rc) {
>     prlog("i2c failed");
>     continue;
> }
>
> if (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
>     prlog("vendor id mismatch")
>     continue;
> }
>
> The control flow is cleaner, there's less indent stacking, and there's
> no multi-line if conditionals which are always a mess. The way
> tpm_i2c_nuvoton_probe() is structed makes it slightly awkward since
> you need to duplicate the cleanup, but it's still better IMO.
>
> > +                       free(tpm_device);
> > +                       continue;

One more thing: Should we be doing goto disable; here rather than
continue? The kernel will try to use the TPM later on since it's still
in the DT.

> > +               }
> >                 if (tpm_register_chip(node, tpm_device,
> >                                       &tpm_i2c_nuvoton_driver)) {
> >                         free(tpm_device);
> > --
> > 2.21.1
> >
> > _______________________________________________
> > Skiboot mailing list
> > Skiboot@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/skiboot
Klaus Heinrich Kiwi March 6, 2020, 11:22 a.m. UTC | #5
On 3/5/2020 11:39 PM, Oliver O'Halloran wrote:
> On Fri, Mar 6, 2020 at 8:17 AM Klaus Heinrich Kiwi
> <klaus@linux.vnet.ibm.com> wrote:
>>
>>
>>
>> On 3/5/2020 3:54 PM, Eric Richter wrote:
>>> +             /* ensure there's really the TPM we expect at that address */
>>> +             if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
>>> +                                      1, &vendor, sizeof(vendor)) ||
>>> +                 (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
>> if tpm_i2c_request_send() fails (negative error code), you skip this
>> block and still tries to tpm_register_chip()?
> 
> Any non-zero value is considered true so it'll go down the error path
> if the I2C transaction fails. I'm not 100% sure I understand what
> you're saying here though...

I misread the logic here, which I guess reinforces the point you are 
also making in your review..

>>> +                     prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
>>> +                           "expected vendor id mismatch\n");
>>> +                     if (vendor) {
>>> +                             prlog(PR_ERR, "NUVOTON: expected 0x%X, "
>>> +                                   "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
>>> +                     }
>>> +                     free(tpm_device);
>>> +                     continue;
>>> +             }
> 
>>>                if (tpm_register_chip(node, tpm_device,
>>>                                      &tpm_i2c_nuvoton_driver)) {
>>>                        free(tpm_device);
>>>
>>
>> Perhaps registering it would then fail? Since tpm_register_chip() sounds
>> both like an action as well as a predicate, perhaps be more explicit
>> about what we are testing? (I know this is outside the proposed patch
>> but still...)
> 
> It doesn't fail, that's the problem. Right now we just keep trucking
> along until skiboot tries to write a measurement to the TPM at which
> point we notice it's not there.

Took a look at the function and it can only return 0 or -1, kinda 
reinforcing that this should be a predicate (but again, not this patch - 
perhaps I could propose something sometime)
Klaus Heinrich Kiwi March 6, 2020, 11:30 a.m. UTC | #6
On 3/5/2020 11:40 PM, Oliver O'Halloran wrote:
>>> +                       free(tpm_device);
>>> +                       continue;
> 
> One more thing: Should we be doing goto disable; here rather than
> continue? The kernel will try to use the TPM later on since it's still
> in the DT.

This also caught my attention, as everywhere else we are jumping to 
outside the loop and aborting..

but in this case, and if there are multiple "compatible=nuvoton,npct650" 
nodes, wouldn't we abort mid-probe when we encounter the first 
considered invalid? is that really what we want?

  -Klaus
Eric Richter March 6, 2020, 4:38 p.m. UTC | #7
On 3/6/20 5:30 AM, Klaus Heinrich Kiwi wrote:
> 
> 
> On 3/5/2020 11:40 PM, Oliver O'Halloran wrote:
>>>> +                       free(tpm_device);
>>>> +                       continue;
>>
>> One more thing: Should we be doing goto disable; here rather than
>> continue? The kernel will try to use the TPM later on since it's still
>> in the DT.
> 
> This also caught my attention, as everywhere else we are jumping to outside the loop and aborting..
> 
> but in this case, and if there are multiple "compatible=nuvoton,npct650" nodes, wouldn't we abort mid-probe when we encounter the first considered invalid? is that really what we want?
> 

I don't think that behavior is desired, though at the moment I'm not aware of anything that may supply multiple TPMs. If we actually want to support multiple TPMs, the status should be set for each node. Probably out of scope for this patch though.

For now, I'll change the error state to jump to disable.
diff mbox series

Patch

diff --git a/libstb/drivers/tpm_i2c_nuvoton.c b/libstb/drivers/tpm_i2c_nuvoton.c
index 3679ddaf..882018e4 100644
--- a/libstb/drivers/tpm_i2c_nuvoton.c
+++ b/libstb/drivers/tpm_i2c_nuvoton.c
@@ -30,6 +30,7 @@ 
 #define TPM_BURST_COUNT		0x01
 #define TPM_DATA_FIFO_W		0x20
 #define TPM_DATA_FIFO_R		0x40
+#define TPM_VID_DID		0x60
 
 /* Bit masks for the TPM STATUS register */
 #define TPM_STS_VALID		0x80
@@ -42,6 +43,8 @@ 
 /* TPM Driver values */
 #define MAX_STSVALID_POLLS 	5
 #define TPM_TIMEOUT_INTERVAL	10
+#define TPM_650_VENDOR_ID	0x5010FE00
+#define TPM_VENDOR_ID_MASK	0xFFFFFF00
 
 static struct tpm_dev *tpm_device = NULL;
 
@@ -552,6 +555,7 @@  void tpm_i2c_nuvoton_probe(void)
 	struct dt_node *node = NULL;
 	struct i2c_bus *bus;
 	const char *name;
+	uint32_t vendor = 0;
 
 	dt_for_each_compatible(dt_root, node, "nuvoton,npct650") {
 		if (!dt_node_is_enabled(node))
@@ -588,6 +592,19 @@  void tpm_i2c_nuvoton_probe(void)
 			      "found, tpm node parent %p\n", node->parent);
 			goto disable;
 		}
+		/* ensure there's really the TPM we expect at that address */
+		if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
+					 1, &vendor, sizeof(vendor)) ||
+		    (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
+			prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
+			      "expected vendor id mismatch\n");
+			if (vendor) {
+				prlog(PR_ERR, "NUVOTON: expected 0x%X, "
+				      "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
+			}
+			free(tpm_device);
+			continue;
+		}
 		if (tpm_register_chip(node, tpm_device,
 				      &tpm_i2c_nuvoton_driver)) {
 			free(tpm_device);