Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Mon, Aug 14, 2023 at 02:23:22PM -0700, Sean Edmond wrote:
> 
> On 2023-08-14 1:39 a.m., Ilias Apalodimas wrote:
> > Hi Sean
> > 
> > On Sat, 12 Aug 2023 at 03:28, <seanedmond@linux.microsoft.com> wrote:
> > > From: Stephen Carlson <stcarlso@linux.microsoft.com>
> > > 
> > > This implementation of the security uclass driver allows existing TPM2
> > > devices declared in the device tree to be referenced for storing the OS
> > > anti-rollback counter, using the TPM2 non-volatile storage API.
> > > 
> > > Signed-off-by: Stephen Carlson <stcarlso@linux.microsoft.com>
> > > ---
> > >   MAINTAINERS                     |   1 +
> > >   drivers/security/Makefile       |   1 +
> > >   drivers/security/security-tpm.c | 173 ++++++++++++++++++++++++++++++++
> > >   include/tpm-v2.h                |   1 +
> > >   4 files changed, 176 insertions(+)
> > >   create mode 100644 drivers/security/security-tpm.c
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > [...]
> > 
> > > +
> > > +struct security_state {
> > > +       u32 index_arbvn;
> > > +       struct udevice *tpm_dev;
> > > +};
> > > +
> > > +static int tpm_security_init(struct udevice *tpm_dev)
> > > +{
> > > +       int res;
> > > +
> > > +       /* Initialize TPM but allow reuse of existing session */
> > > +       res = tpm_open(tpm_dev);
> > > +       if (res == -EBUSY) {
> > > +               log(UCLASS_SECURITY, LOGL_DEBUG,
> > > +                   "Existing TPM session found, reusing\n");
> > > +       } else {
> > > +               if (res) {
> > > +                       log(UCLASS_SECURITY, LOGL_ERR,
> > > +                           "TPM initialization failed (ret=%d)\n", res);
> > > +                       return res;
> > > +               }
> > > +
> > > +               res = tpm2_startup(tpm_dev, TPM2_SU_CLEAR);
> > > +               if (res) {
> > > +                       log(UCLASS_SECURITY, LOGL_ERR,
> > > +                           "TPM startup failed (ret=%d)\n", res);
> > > +                       return res;
> > > +               }
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > There's nothing security related in that wrapper.  It looks like a
> > typical tpm startup sequence.  Any reason you can't use
> > tpm_auto_start()?
> 
> Good suggestion, I'll make this change.
> 
> > 
> > > +
> > > +static int tpm_security_arbvn_get(struct udevice *dev, u64 *arbvn)
> > > +{
> > > +       struct security_state *priv = dev_get_priv(dev);
> > > +       int ret;
> > > +
> > > +       if (!arbvn)
> > > +               return -EINVAL;
> > > +
> > > +       ret = tpm2_nv_read_value(priv->tpm_dev, priv->index_arbvn, arbvn,
> > > +                                sizeof(u64));
> > > +       if (ret == TPM2_RC_NV_UNINITIALIZED) {
> > > +               /* Expected if no OS image has been loaded before */
> > > +               log(UCLASS_SECURITY, LOGL_INFO,
> > > +                   "No previous OS image, defaulting ARBVN to 0\n");
> > > +               *arbvn = 0ULL;
> > Why aren't we returning an error here?  Looks like the code following
> > this is trying to reason with the validity of arbnv
> On the first boot (before ARBVN has been set in NV memory), it's expected
> that the NV index hasn't been initialized/written yet.  In this case,
> TPM2_RC_NV_UNINITIALIZED is expected.  A value of 0 is returned to ensure
> that the anti-rollback check always passes (which it should since there's
> nothing to check on the first boot).

Ok then I think it's better to add an 'init' function which will talk to
the TPM and try to read the value.  If you get an error or
TPM2_RC_NV_UNINITIALIZED(), we can then create the NV storage and
initialize it.  Note here that blindly returning 0 isn't correct either.  
When you define a TPM NV counter index it will hold any stored value (and
reuse it) even if you delete it and re-add it. 
I think doing it like this will make _get() a bit simpler, since you just
have to talk to the TPM and return whatever you read. 

> > 
> > > +       } else if (ret) {
> > > +               log(UCLASS_SECURITY, LOGL_ERR,
> > > +                   "Unable to read ARBVN from TPM (ret=%d)\n", ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int tpm_security_arbvn_set(struct udevice *dev, u64 arbvn)
> > > +{
> > > +       struct security_state *priv = dev_get_priv(dev);
> > > +       struct udevice *tpm_dev = priv->tpm_dev;
> > > +       u64 old_arbvn;
> > > +       int ret;
> > > +
> > > +       ret = tpm_security_arbvn_get(dev, &old_arbvn);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       if (arbvn < old_arbvn)
> > > +               return -EPERM;
> > > +
> > What happens if they are equal ?
> > 
> If they are equal, then we are booting the same OS that was previously
> booted (we are not moving the OS version forward or back).
> 
> Note the actual "anti-rollback" check is in fit_image_verify_arbvn().  If it
> make things more clear, we could remove the value checks here completely and
> just write the value.
> 

Ok, I think adding another statment would be a bit easier to read then.
Right after reading the stored TPM value, just return 0 if arbvn == old_arbvn

> > > +       if (arbvn > old_arbvn) {
> > You just check for this and exited
> > 
> > > +               ret = tpm2_nv_write_value(tpm_dev, priv->index_arbvn, &arbvn,
> > > +                                         sizeof(u64));
> > > +               if (ret) {
> > > +                       log(UCLASS_SECURITY, LOGL_ERR,
> > > +                           "Unable to write ARBVN to TPM (ret=%d)\n", ret);
> > > +                       return ret;
> > > +               }
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct dm_security_ops tpm_security_ops = {
> > > +       .arbvn_get              = tpm_security_arbvn_get,
> > > +       .arbvn_set              = tpm_security_arbvn_set,
> > > +};
> > > +
> > > +static int tpm_security_probe(struct udevice *dev)
> > > +{
> > > +       struct security_state *priv = dev_get_priv(dev);
> > > +       struct udevice *tpm_dev = priv->tpm_dev;
> > > +       u32 index = priv->index_arbvn;
> > > +       int ret;
> > > +
> > > +       if (!tpm_dev) {
> > > +               log(UCLASS_SECURITY, LOGL_ERR,
> > > +                   "TPM device not defined in DTS\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       ret = tpm_security_init(tpm_dev);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = tpm2_nv_define_space(tpm_dev, index, sizeof(u64), TPMA_NV_PPREAD |
> > > +                                  TPMA_NV_PPWRITE | TPMA_NV_PLATFORMCREATE,
> > > +                                  NULL, 0);
> > How secure is that ? Is it protected by a locality? We dont seem to be
> > using an auth value when creating the index
> On our platform, we're using a different security device driver to provide
> our secure storage (we aren't using this TPM backed driver).  I'm not an
> expert on authorization for NV indexes, but I'd welcome feedback on how we
> could make this driver more secure (and publicly available) for others.

IIRC we can use a 'NV Counter Index' for the counter.  That's only allowed
to increment and even if you delete it and reinitialize it, it will retain
it's (lost) value)

> > 
> > > +       /* NV_DEFINED is an expected error if ARBVN already initialized */
> > > +       if (ret == TPM2_RC_NV_DEFINED)
> > > +               log(UCLASS_SECURITY, LOGL_DEBUG,
> > > +                   "ARBVN index %u already defined\n", index);
> > I'd prefer returning 0 here. The rewrite the code below as
> >   if (ret)
> >     log().....
> > 
> > return ret;
> > 
> > > +       else if (ret) {
> > > +               log(UCLASS_SECURITY, LOGL_ERR,
> > > +                   "Unable to create ARBVN NV index (ret=%d)\n", ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int tpm_security_remove(struct udevice *dev)
> > > +{
> > > +       struct security_state *priv = dev_get_priv(dev);
> > > +
> > > +       return tpm_close(priv->tpm_dev);
> > > +}
> > > +
> > > +static int tpm_security_ofdata_to_platdata(struct udevice *dev)
> > > +{
> > > +       const u32 phandle = (u32)dev_read_u32_default(dev, "tpm", 0);
> > > +       struct security_state *priv = dev_get_priv(dev);
> > > +       struct udevice *tpm_dev;
> > > +       int ret;
> > > +
> > > +       ret = uclass_get_device_by_phandle_id(UCLASS_TPM, phandle, &tpm_dev);
> > > +       if (ret) {
> > > +               log(UCLASS_SECURITY, LOGL_ERR, "TPM node in DTS is invalid\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       priv->index_arbvn = (u32)dev_read_u32_default(dev, "arbvn-nv-index", 0);
> > > +       priv->tpm_dev = tpm_dev;
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct udevice_id tpm_security_ids[] = {
> > > +       { .compatible = "tpm,security" },
> > > +       { }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(security_tpm) = {
> > > +       .name   = "security_tpm",
> > > +       .id     = UCLASS_SECURITY,
> > > +       .priv_auto = sizeof(struct security_state),
> > > +       .of_match = tpm_security_ids,
> > > +       .of_to_plat = tpm_security_ofdata_to_platdata,
> > > +       .probe  = tpm_security_probe,
> > > +       .remove = tpm_security_remove,
> > > +       .ops    = &tpm_security_ops,
> > > +};
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index 2b6980e441..49bf0f0ba4 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -321,6 +321,7 @@ enum tpm2_return_codes {
> > >          TPM2_RC_COMMAND_CODE    = TPM2_RC_VER1 + 0x0043,
> > >          TPM2_RC_AUTHSIZE        = TPM2_RC_VER1 + 0x0044,
> > >          TPM2_RC_AUTH_CONTEXT    = TPM2_RC_VER1 + 0x0045,
> > > +       TPM2_RC_NV_UNINITIALIZED        = TPM2_RC_VER1 + 0x04a,
> > >          TPM2_RC_NV_DEFINED      = TPM2_RC_VER1 + 0x004c,
> > >          TPM2_RC_NEEDS_TEST      = TPM2_RC_VER1 + 0x0053,
> > >          TPM2_RC_WARN            = 0x0900,
> > > --
> > > 2.40.0
> > > 
> > Thanks
> > /Ilias
> > 
> > 

Thanks
/Ilias