Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
Hi Sean, On Thu, 17 Aug 2023 at 17:29, Sean Edmond wrote: > > Hi Simon, > > On 2023-08-17 6:41 a.m., Simon Glass wrote: > > Hi Sean, > > > > On Fri, 11 Aug 2023 at 18:28, wrote: > >> From: Stephen Carlson > >> > >> 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 > >> --- > >> 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 > > This is a bit wonky w.r.t driver model. The TPM itself should > > implement this API, perhaps ina separate file shared with all v2 TPMs. > > You should not be opening the device, etc. in here. > > > > I hope that makes sense...you effectively need to turn the TPM into a > > multi-function device within driver model. Of course TPMs are > > multi-function devices anyway, but here you are making their functions > > available more explicitly with a nicer API. > > > > Then you can call the TPM API to do what you want, but at least you > > know that the TPM has been probed before you start. > > > > Regards, > > Simon > > > Let's step back for a moment to understand our intention with this feature. > > We want secure storage for the anti-rollback counter. The intention is > that this storage is provided by whatever "secure driver" (let's start > calling it the "rollback driver"). On our platform, we're using a > different "rollback driver" which accesses a non-TPM based secure > storage (which we can't upstream because it's proprietary). For the > purpose of making this feature publicly available, we've added the > TPM-backed "rollback driver" (this patch). I'll make this intention > more clear in the documentation, which should lead to less confusion? > > At the end of the day, all we require is dm_security_arbvn_get() and > dm_security_arbvn_set(), to get/set from the secure storage (we'll > rename these to something less ugly, because yes arbvn is gross). We > don't want to lock this feature to the TPM, because it doesn't enable > us/others to choose a different secure storage mechanism. It doesn't need to be locked to the TPM. But since you have a uclass you can have different drivers implementing the same UCLASS_ROLLBACK: - a 'stand-alone' one that does strage and secret things - a TPM-based one that makes TPM calls > > W.r.t opening/initializing the TPM. Someone has to open it? In our > case, it's being opened earlier with our measured boot, so "-EBUSY" in > returned on tpm_open() (we return and everyone is happy). My > understanding is that this is the currently available way for multiple > drivers to access the TPM. Ilias has recommended we use > tpm_auto_start(), which is an enhancement I'm planning to make. This > should clean thing up? If this doesn't meet your expectations, can you > describe in more detail how we should turn the TPM into a > "multi-function device"? The TPM will be probed automatically by probing its child device. Not opened, though...that would have to happen elsewhere. But it doesn't mean you need to turn the TPM into a multi-function device. It's just that, in most cases, others would use a TPM to provide the rollback counters. For testing purposes, you should probably create a device which is a child of the sandbox TPM2 and run the tests with that. Regards, Simon
Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
Hi Simon,
On 2023-08-17 6:41 a.m., Simon Glass wrote:
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, wrote:
From: Stephen Carlson
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
---
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
This is a bit wonky w.r.t driver model. The TPM itself should
implement this API, perhaps ina separate file shared with all v2 TPMs.
You should not be opening the device, etc. in here.
I hope that makes sense...you effectively need to turn the TPM into a
multi-function device within driver model. Of course TPMs are
multi-function devices anyway, but here you are making their functions
available more explicitly with a nicer API.
Then you can call the TPM API to do what you want, but at least you
know that the TPM has been probed before you start.
Regards,
Simon
Let's step back for a moment to understand our intention with this feature.
We want secure storage for the anti-rollback counter. The intention is
that this storage is provided by whatever "secure driver" (let's start
calling it the "rollback driver"). On our platform, we're using a
different "rollback driver" which accesses a non-TPM based secure
storage (which we can't upstream because it's proprietary). For the
purpose of making this feature publicly available, we've added the
TPM-backed "rollback driver" (this patch). I'll make this intention
more clear in the documentation, which should lead to less confusion?
At the end of the day, all we require is dm_security_arbvn_get() and
dm_security_arbvn_set(), to get/set from the secure storage (we'll
rename these to something less ugly, because yes arbvn is gross). We
don't want to lock this feature to the TPM, because it doesn't enable
us/others to choose a different secure storage mechanism.
W.r.t opening/initializing the TPM. Someone has to open it? In our
case, it's being opened earlier with our measured boot, so "-EBUSY" in
returned on tpm_open() (we return and everyone is happy). My
understanding is that this is the currently available way for multiple
drivers to access the TPM. Ilias has recommended we use
tpm_auto_start(), which is an enhancement I'm planning to make. This
should clean thing up? If this doesn't meet your expectations, can you
describe in more detail how we should turn the TPM into a
"multi-function device"?
diff --git a/MAINTAINERS b/MAINTAINERS
index 73b6943e03..257660a847 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1444,6 +1444,7 @@ S:Maintained
F: drivers/security/Kconfig
F: drivers/security/Makefile
F: drivers/security/sandbox_security.c
+F: drivers/security/security-tpm.c
F: drivers/security/security-uclass.c
SEMIHOSTING
diff --git a/drivers/security/Makefile b/drivers/security/Makefile
index ed10c3f234..e81966bb4a 100644
--- a/drivers/security/Makefile
+++ b/drivers/security/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_DM_SECURITY) += security-uclass.o
obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
+obj-$(CONFIG_SECURITY_TPM) += security-tpm.o
diff --git a/drivers/security/security-tpm.c b/drivers/security/security-tpm.c
new file mode 100644
index 00..9070dd49ac
--- /dev/null
+++ b/drivers/security/security-tpm.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 Microsoft, Inc
+ * Written by Stephen Carlson
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+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;
+}
+
+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;
+
+ r
Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
Hi Sean,
On Fri, 11 Aug 2023 at 18:28, wrote:
>
> From: Stephen Carlson
>
> 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
> ---
> 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
This is a bit wonky w.r.t driver model. The TPM itself should
implement this API, perhaps ina separate file shared with all v2 TPMs.
You should not be opening the device, etc. in here.
I hope that makes sense...you effectively need to turn the TPM into a
multi-function device within driver model. Of course TPMs are
multi-function devices anyway, but here you are making their functions
available more explicitly with a nicer API.
Then you can call the TPM API to do what you want, but at least you
know that the TPM has been probed before you start.
Regards,
Simon
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 73b6943e03..257660a847 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1444,6 +1444,7 @@ S:Maintained
> F: drivers/security/Kconfig
> F: drivers/security/Makefile
> F: drivers/security/sandbox_security.c
> +F: drivers/security/security-tpm.c
> F: drivers/security/security-uclass.c
>
> SEMIHOSTING
> diff --git a/drivers/security/Makefile b/drivers/security/Makefile
> index ed10c3f234..e81966bb4a 100644
> --- a/drivers/security/Makefile
> +++ b/drivers/security/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_DM_SECURITY) += security-uclass.o
> obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
> +obj-$(CONFIG_SECURITY_TPM) += security-tpm.o
> diff --git a/drivers/security/security-tpm.c b/drivers/security/security-tpm.c
> new file mode 100644
> index 00..9070dd49ac
> --- /dev/null
> +++ b/drivers/security/security-tpm.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 Microsoft, Inc
> + * Written by Stephen Carlson
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +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;
> +}
> +
> +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;
> + } 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;
> +
> + if (arbvn > old_arbvn) {
> + 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;
> + }
> +
Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
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, wrote:
> > > From: Stephen Carlson
> > >
> > > 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
> > > ---
> > > 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 thin
Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
On 2023-08-14 1:39 a.m., Ilias Apalodimas wrote:
Hi Sean
On Sat, 12 Aug 2023 at 03:28, wrote:
From: Stephen Carlson
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
---
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).
+ } 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.
+ 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_PP
Re: [PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
Hi Sean
On Sat, 12 Aug 2023 at 03:28, wrote:
>
> From: Stephen Carlson
>
> 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
> ---
> 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()?
> +
> +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
> + } 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 (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
> + /* 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
[PATCH 2/5] drivers: security: Add TPM2 implementation of security devices
From: Stephen Carlson
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
---
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
index 73b6943e03..257660a847 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1444,6 +1444,7 @@ S:Maintained
F: drivers/security/Kconfig
F: drivers/security/Makefile
F: drivers/security/sandbox_security.c
+F: drivers/security/security-tpm.c
F: drivers/security/security-uclass.c
SEMIHOSTING
diff --git a/drivers/security/Makefile b/drivers/security/Makefile
index ed10c3f234..e81966bb4a 100644
--- a/drivers/security/Makefile
+++ b/drivers/security/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_DM_SECURITY) += security-uclass.o
obj-$(CONFIG_SECURITY_SANDBOX) += sandbox_security.o
+obj-$(CONFIG_SECURITY_TPM) += security-tpm.o
diff --git a/drivers/security/security-tpm.c b/drivers/security/security-tpm.c
new file mode 100644
index 00..9070dd49ac
--- /dev/null
+++ b/drivers/security/security-tpm.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 Microsoft, Inc
+ * Written by Stephen Carlson
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+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;
+}
+
+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;
+ } 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;
+
+ if (arbvn > old_arbvn) {
+ 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,
+ N
