Curently the locality is hard coded to 0 but for DRTM support, access
is needed to localities 1 through 4.
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
drivers/char/tpm/tpm-chip.c | 24 +++++++++++++++++++++++-
include/linux/tpm.h | 4 ++++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 854546000c92..1ca390a742ed 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -44,7 +44,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
if (!chip->ops->request_locality)
return 0;
- rc = chip->ops->request_locality(chip, 0);
+ rc = chip->ops->request_locality(chip, chip->default_locality);
if (rc < 0)
return rc;
@@ -143,6 +143,27 @@ void tpm_chip_stop(struct tpm_chip *chip)
}
EXPORT_SYMBOL_GPL(tpm_chip_stop);
+/**
+ * tpm_chip_set_default_locality() - set the TPM chip default locality to open
+ * @chip: a TPM chip to use
+ * @locality: the default locality to set
+ *
+ * Return:
+ * * true - Preferred locality set
+ * * false - Invalid locality specified
+ */
+bool tpm_chip_set_default_locality(struct tpm_chip *chip, int locality)
+{
+ if (locality < 0 || locality >=TPM_MAX_LOCALITY)
+ return false;
+
+ mutex_lock(&chip->tpm_mutex);
+ chip->default_locality = locality;
+ mutex_unlock(&chip->tpm_mutex);
+ return true;
+}
+EXPORT_SYMBOL_GPL(tpm_chip_set_default_locality);
+
/**
* tpm_try_get_ops() - Get a ref to the tpm_chip
* @chip: Chip to ref
@@ -374,6 +395,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
}
chip->locality = -1;
+ chip->default_locality = 0;
return chip;
out:
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 98f2c7c1c52e..83e94b2f0cef 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -219,6 +219,9 @@ struct tpm_chip {
u8 null_ec_key_y[EC_PT_SZ];
struct tpm2_auth *auth;
#endif
+
+ /* preferred locality - default 0 */
+ int default_locality;
};
#define TPM_HEADER_SIZE 10
@@ -446,6 +449,7 @@ static inline u32 tpm2_rc_value(u32 rc)
extern int tpm_is_tpm2(struct tpm_chip *chip);
extern __must_check int tpm_try_get_ops(struct tpm_chip *chip);
extern void tpm_put_ops(struct tpm_chip *chip);
+extern bool tpm_chip_set_default_locality(struct tpm_chip *chip, int locality);
extern ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
size_t min_rsp_body_length, const char *desc);
extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
--
2.39.3
On Fri Sep 13, 2024 at 11:05 PM EEST, Ross Philipson wrote: > Curently the locality is hard coded to 0 but for DRTM support, access > is needed to localities 1 through 4. Why? > > Signed-off-by: Ross Philipson <ross.philipson@oracle.com> BR, Jarkko
DRTM needs to be able to set the locality used by kernel. Provide
TPM_IOC_SET_LOCALITY operation for this purpose. It is enabled only if
the kernel command-line has 'tpm.set_locality_enabled=1'. The operation
is one-shot allowed only for tpm_tis for the moment.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
NOTE: Only compile-tested.
---
.../admin-guide/kernel-parameters.txt | 5 ++
.../userspace-api/ioctl/ioctl-number.rst | 2 +
drivers/char/tpm/tpm-chip.c | 2 +-
drivers/char/tpm/tpm-dev.c | 70 +++++++++++++++++++
drivers/char/tpm/tpm_tis_core.c | 2 +
include/linux/tpm.h | 10 +++
include/uapi/linux/tpm.h | 11 +++
7 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 include/uapi/linux/tpm.h
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..9e760de6c307 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6727,6 +6727,11 @@
torture.verbose_sleep_duration= [KNL]
Duration of each verbose-printk() sleep in jiffies.
+ tpm.set_locality_enabled= [HW,TPM]
+ Enable one-shot locality setting after the boot. It can
+ can be set with the TPM_IOC_SET_LOCALE ioctl applied to
+ /dev/tpm0. The parameter is set by default as '0'.
+
tpm_suspend_pcr=[HW,TPM]
Format: integer pcr id
Specify that at suspend time, the tpm driver
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index e4be1378ba26..3eba57ab2fb1 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -338,6 +338,8 @@ Code Seq# Include File Comments
0xA3 90-9F linux/dtlk.h
0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
0xA4 00-1F uapi/asm/sgx.h <mailto:linux-sgx@vger.kernel.org>
+0xA4 00-1F uapi/linux/tpm.h TPM
+ <mailto:linux-integrity@vger.kernel.org>
0xA5 01-05 linux/surface_aggregator/cdev.h Microsoft Surface Platform System Aggregator
<mailto:luzmaximilian@gmail.com>
0xA5 20-2F linux/surface_aggregator/dtx.h Microsoft Surface DTX driver
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 7df7abaf3e52..c8016342352a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -44,7 +44,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
if (!chip->ops->request_locality)
return 0;
- rc = chip->ops->request_locality(chip, 0);
+ rc = chip->ops->request_locality(chip, chip->default_locality);
if (rc < 0)
return rc;
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 97c94b5e9340..bb9c346947aa 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -13,8 +13,74 @@
* Device file system interface to the TPM
*/
#include <linux/slab.h>
+#include <uapi/linux/tpm.h>
#include "tpm-dev.h"
+static bool set_locality_enabled;
+module_param(set_locality_enabled, bool, 0644);
+
+/*
+ * Set a locality as a one-shot operation. A chip must declare support for it
+ * with %TPM_CHIP_FLAG_SET_LOCALITY_ENABLED, which will cleared after setting
+ * the locality.
+ */
+static long tpm_ioc_set_locality(struct tpm_chip *chip, u8 __user *localityp)
+{
+ u8 locality;
+
+ if (!set_locality_enabled)
+ return -ENOIOCTLCMD;
+
+ if (chip->flags & TPM_CHIP_FLAG_SET_LOCALITY_ENABLED)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(&locality, localityp, sizeof(locality)))
+ return -EFAULT;
+
+ if (locality >= TPM_MAX_LOCALITY)
+ return -EINVAL;
+
+ chip->default_locality = locality;
+ chip->flags &= ~TPM_CHIP_FLAG_SET_LOCALITY_ENABLED;
+ return 0;
+}
+
+static long tpm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct file_priv *priv = file->private_data;
+ void __user *argp = (void __user *)arg;
+ struct tpm_chip *chip = priv->chip;
+ int ret;
+
+ mutex_lock(&priv->buffer_mutex);
+
+ ret = tpm_try_get_ops(chip);
+ if (ret)
+ goto out;
+
+ switch (cmd) {
+ case TPM_IOC_SET_LOCALITY:
+ tpm_ioc_set_locality(chip, argp);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
+ break;
+ }
+
+ tpm_put_ops(chip);
+
+out:
+ mutex_unlock(&priv->buffer_mutex);
+ return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static long tpm_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+ return tpm_ioctl(filep, cmd, arg);
+}
+#endif
+
static int tpm_open(struct inode *inode, struct file *file)
{
struct tpm_chip *chip;
@@ -59,6 +125,10 @@ static int tpm_release(struct inode *inode, struct file *file)
const struct file_operations tpm_fops = {
.owner = THIS_MODULE,
+ .unlocked_ioctl = tpm_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = tpm_compat_ioctl,
+#endif
.open = tpm_open,
.read = tpm_common_read,
.write = tpm_common_write,
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fdef214b9f6b..3517db710423 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1111,6 +1111,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (IS_ERR(chip))
return PTR_ERR(chip);
+ chip->flags |= TPM_CHIP_FLAG_SET_LOCALITY_ENABLED;
+
#ifdef CONFIG_ACPI
chip->acpi_dev_handle = acpi_dev_handle;
#endif
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 587b96b4418e..27071ef13b7a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -147,6 +147,12 @@ struct tpm_chip_seqops {
*/
#define TPM2_MAX_CONTEXT_SIZE 4096
+/*
+ * The maximum locality (0 - 4) for a TPM, as defined in section 3.2 of the
+ * Client Platform Profile Specification.
+ */
+#define TPM_MAX_LOCALITY 4
+
struct tpm_chip {
struct device dev;
struct device devs;
@@ -202,6 +208,9 @@ struct tpm_chip {
/* active locality */
int locality;
+ /* the default locality used by the kernel (default 0) */
+ u8 default_locality;
+
#ifdef CONFIG_TCG_TPM2_HMAC
/* details for communication security via sessions */
@@ -348,6 +357,7 @@ enum tpm_chip_flags {
TPM_CHIP_FLAG_SUSPENDED = BIT(8),
TPM_CHIP_FLAG_HWRNG_DISABLED = BIT(9),
TPM_CHIP_FLAG_DISABLE = BIT(10),
+ TPM_CHIP_FLAG_SET_LOCALITY_ENABLED = BIT(11),
};
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
diff --git a/include/uapi/linux/tpm.h b/include/uapi/linux/tpm.h
new file mode 100644
index 000000000000..654080e1b1e5
--- /dev/null
+++ b/include/uapi/linux/tpm.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_TPM_H
+#define _UAPI_TPM_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define TPM_MAGIC 0xA4
+#define TPM_IOC_SET_LOCALITY _IOW(TPM_MAGIC, 0x00, u8)
+
+#endif /* _UAPI_TPM_H */
--
2.47.0
On Sat Nov 2, 2024 at 3:37 AM EET, Jarkko Sakkinen wrote: > DRTM needs to be able to set the locality used by kernel. Provide > TPM_IOC_SET_LOCALITY operation for this purpose. It is enabled only if > the kernel command-line has 'tpm.set_locality_enabled=1'. The operation > is one-shot allowed only for tpm_tis for the moment. > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> I hope you could take it from there if this works for you in the big pic, and address possible comments, thanks. BR, Jarkko
DRTM needs to be able to set the locality used by kernel. Provide
TPM_IOC_SET_LOCALITY operation for this purpose. It is enabled only if
the kernel command-line has 'tpm.set_locality_enabled=1'. The operation
is one-shot allowed only for tpm_tis for the moment.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- Do not ignore the return value of tpm_ioc_set_locality().
- if (!(chip->flags & TPM_CHIP_FLAG_SET_LOCALITY_ENABLED))
- Refined kernel-parameters.txt description.
- Use __u8 instead of u8 in the uapi.
- Tested with https://codeberg.org/jarkko/tpm-set-locality-test/src/branch/main/src/main.rs
v1:
- NOTE: Only compile-tested.
---
.../admin-guide/kernel-parameters.txt | 5 ++
.../userspace-api/ioctl/ioctl-number.rst | 2 +
drivers/char/tpm/tpm-chip.c | 2 +-
drivers/char/tpm/tpm-dev.c | 70 +++++++++++++++++++
drivers/char/tpm/tpm_tis_core.c | 2 +
include/linux/tpm.h | 10 +++
include/uapi/linux/tpm.h | 11 +++
7 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 include/uapi/linux/tpm.h
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..8fd9fc94c883 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6727,6 +6727,11 @@
torture.verbose_sleep_duration= [KNL]
Duration of each verbose-printk() sleep in jiffies.
+ tpm.set_locality_enabled= [HW,TPM]
+ Enable one-shot locality setting after the boot. The
+ operation can be performed with TPM_IOC_SET_LOCALITY
+ applied to /dev/tpm0. The default value is 0.
+
tpm_suspend_pcr=[HW,TPM]
Format: integer pcr id
Specify that at suspend time, the tpm driver
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index e4be1378ba26..3eba57ab2fb1 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -338,6 +338,8 @@ Code Seq# Include File Comments
0xA3 90-9F linux/dtlk.h
0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
0xA4 00-1F uapi/asm/sgx.h <mailto:linux-sgx@vger.kernel.org>
+0xA4 00-1F uapi/linux/tpm.h TPM
+ <mailto:linux-integrity@vger.kernel.org>
0xA5 01-05 linux/surface_aggregator/cdev.h Microsoft Surface Platform System Aggregator
<mailto:luzmaximilian@gmail.com>
0xA5 20-2F linux/surface_aggregator/dtx.h Microsoft Surface DTX driver
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 7df7abaf3e52..c8016342352a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -44,7 +44,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
if (!chip->ops->request_locality)
return 0;
- rc = chip->ops->request_locality(chip, 0);
+ rc = chip->ops->request_locality(chip, chip->default_locality);
if (rc < 0)
return rc;
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 97c94b5e9340..d07aec98f894 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -13,8 +13,74 @@
* Device file system interface to the TPM
*/
#include <linux/slab.h>
+#include <uapi/linux/tpm.h>
#include "tpm-dev.h"
+static bool set_locality_enabled;
+module_param(set_locality_enabled, bool, 0644);
+
+/*
+ * Set a locality as a one-shot operation. A chip must declare support for it
+ * with %TPM_CHIP_FLAG_SET_LOCALITY_ENABLED, which will cleared after setting
+ * the locality.
+ */
+static long tpm_ioc_set_locality(struct tpm_chip *chip, u8 __user *localityp)
+{
+ u8 locality;
+
+ if (!set_locality_enabled)
+ return -ENOIOCTLCMD;
+
+ if (!(chip->flags & TPM_CHIP_FLAG_SET_LOCALITY_ENABLED))
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(&locality, localityp, sizeof(locality)))
+ return -EFAULT;
+
+ if (locality >= TPM_MAX_LOCALITY)
+ return -EINVAL;
+
+ chip->default_locality = locality;
+ chip->flags &= ~TPM_CHIP_FLAG_SET_LOCALITY_ENABLED;
+ return 0;
+}
+
+static long tpm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct file_priv *priv = file->private_data;
+ void __user *argp = (void __user *)arg;
+ struct tpm_chip *chip = priv->chip;
+ int ret;
+
+ mutex_lock(&priv->buffer_mutex);
+
+ ret = tpm_try_get_ops(chip);
+ if (ret)
+ goto out;
+
+ switch (cmd) {
+ case TPM_IOC_SET_LOCALITY:
+ ret = tpm_ioc_set_locality(chip, argp);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
+ break;
+ }
+
+ tpm_put_ops(chip);
+
+out:
+ mutex_unlock(&priv->buffer_mutex);
+ return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static long tpm_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+ return tpm_ioctl(filep, cmd, arg);
+}
+#endif
+
static int tpm_open(struct inode *inode, struct file *file)
{
struct tpm_chip *chip;
@@ -59,6 +125,10 @@ static int tpm_release(struct inode *inode, struct file *file)
const struct file_operations tpm_fops = {
.owner = THIS_MODULE,
+ .unlocked_ioctl = tpm_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = tpm_compat_ioctl,
+#endif
.open = tpm_open,
.read = tpm_common_read,
.write = tpm_common_write,
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fdef214b9f6b..3517db710423 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1111,6 +1111,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (IS_ERR(chip))
return PTR_ERR(chip);
+ chip->flags |= TPM_CHIP_FLAG_SET_LOCALITY_ENABLED;
+
#ifdef CONFIG_ACPI
chip->acpi_dev_handle = acpi_dev_handle;
#endif
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 587b96b4418e..27071ef13b7a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -147,6 +147,12 @@ struct tpm_chip_seqops {
*/
#define TPM2_MAX_CONTEXT_SIZE 4096
+/*
+ * The maximum locality (0 - 4) for a TPM, as defined in section 3.2 of the
+ * Client Platform Profile Specification.
+ */
+#define TPM_MAX_LOCALITY 4
+
struct tpm_chip {
struct device dev;
struct device devs;
@@ -202,6 +208,9 @@ struct tpm_chip {
/* active locality */
int locality;
+ /* the default locality used by the kernel (default 0) */
+ u8 default_locality;
+
#ifdef CONFIG_TCG_TPM2_HMAC
/* details for communication security via sessions */
@@ -348,6 +357,7 @@ enum tpm_chip_flags {
TPM_CHIP_FLAG_SUSPENDED = BIT(8),
TPM_CHIP_FLAG_HWRNG_DISABLED = BIT(9),
TPM_CHIP_FLAG_DISABLE = BIT(10),
+ TPM_CHIP_FLAG_SET_LOCALITY_ENABLED = BIT(11),
};
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
diff --git a/include/uapi/linux/tpm.h b/include/uapi/linux/tpm.h
new file mode 100644
index 000000000000..73485a184f14
--- /dev/null
+++ b/include/uapi/linux/tpm.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_TPM_H
+#define _UAPI_TPM_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define TPM_MAGIC 0xA4
+#define TPM_IOC_SET_LOCALITY _IOW(TPM_MAGIC, 0x00, __u8)
+
+#endif /* _UAPI_TPM_H */
--
2.47.0
On Sat Nov 2, 2024 at 8:22 AM EET, Jarkko Sakkinen wrote: > DRTM needs to be able to set the locality used by kernel. Provide > TPM_IOC_SET_LOCALITY operation for this purpose. It is enabled only if > the kernel command-line has 'tpm.set_locality_enabled=1'. The operation > is one-shot allowed only for tpm_tis for the moment. > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > v2: > - Do not ignore the return value of tpm_ioc_set_locality(). > - if (!(chip->flags & TPM_CHIP_FLAG_SET_LOCALITY_ENABLED)) > - Refined kernel-parameters.txt description. > - Use __u8 instead of u8 in the uapi. > - Tested with https://codeberg.org/jarkko/tpm-set-locality-test/src/branch/main/src/main.rs This version has been also tested (and encountered bugs fixed). I wrote a small test program to verify that it works linked above. After the boot, the new ioctl can reset exactly once the locality. Other benefit is that the feature can be selected per driver (at this point tpm_tis drivers) and protection of the access with DAC, SELinux etc. And thanks to the kernel command-line parameter, it is an opt-in feature like it should because vast majority of users will probably never use trenchboot. I.e. set 'tpm.set_locality_enable=1' to have the ioctl available. I think this is a solution that at least I could live with. It has somewhat rigid commmon-sense constraints. BR, Jarkko
On Sat, 2 Nov 2024 at 07:29, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Sat Nov 2, 2024 at 8:22 AM EET, Jarkko Sakkinen wrote: > > DRTM needs to be able to set the locality used by kernel. Provide > > TPM_IOC_SET_LOCALITY operation for this purpose. It is enabled only if > > the kernel command-line has 'tpm.set_locality_enabled=1'. The operation > > is one-shot allowed only for tpm_tis for the moment. > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > v2: > > - Do not ignore the return value of tpm_ioc_set_locality(). > > - if (!(chip->flags & TPM_CHIP_FLAG_SET_LOCALITY_ENABLED)) > > - Refined kernel-parameters.txt description. > > - Use __u8 instead of u8 in the uapi. > > - Tested with https://codeberg.org/jarkko/tpm-set-locality-test/src/branch/main/src/main.rs > > This version has been also tested (and encountered bugs fixed). I wrote > a small test program to verify that it works linked above. > > After the boot, the new ioctl can reset exactly once the locality. Other > benefit is that the feature can be selected per driver (at this point > tpm_tis drivers) and protection of the access with DAC, SELinux etc. > > And thanks to the kernel command-line parameter, it is an opt-in > feature like it should because vast majority of users will probably > never use trenchboot. I.e. set 'tpm.set_locality_enable=1' to have > the ioctl available. > > I think this is a solution that at least I could live with. It has > somewhat rigid commmon-sense constraints. > Before adding a kernel command line parameter, please ask yourself who is going to set it and where, and whether there is any risk of abuse. The kernel command line is external input that is not signed, and the only known user of this set_locality feature is internal to the kernel. Same for the ioctl() [as well as the read-write sysfs node]: looking at the code (patch 19/20) it doesn't seem like user space needs to be able to modify this at all, at least not for the patch set as presented. So for now, can we just stick with making the sysfs node read-only? The only thing I would recommend is exporting the set_locality() symbol in a module namespace, so that it is obvious that it is not intended for general use by other modules (although not impossible). E.g., CRYPTO_INTERNAL does something similar, and if MODULE_IMPORT_NS(CRYPTO_INTERNAL) appears in new code, reviewers are alerted that it accesses internal APIs rather than ones intended for other subsystems to use.
On Sat Nov 2, 2024 at 11:02 AM EET, Ard Biesheuvel wrote: > Same for the ioctl() [as well as the read-write sysfs node]: looking > at the code (patch 19/20) it doesn't seem like user space needs to be > able to modify this at all, at least not for the patch set as > presented. So for now, can we just stick with making the sysfs node > read-only? Short answer: I have no idea. I would not mind that but neither the commit message for TPM give a clue on this. Actually, I *do not care* if it is RO and RW but I'm neither good at guessing random shit. I haad to assume it was *needed* for reason that I do not know given that sysfs attribute was RW. BR, Jarkko
On Sat, 2 Nov 2024 at 11:38, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Sat Nov 2, 2024 at 11:02 AM EET, Ard Biesheuvel wrote: > > Same for the ioctl() [as well as the read-write sysfs node]: looking > > at the code (patch 19/20) it doesn't seem like user space needs to be > > able to modify this at all, at least not for the patch set as > > presented. So for now, can we just stick with making the sysfs node > > read-only? > > Short answer: I have no idea. I would not mind that but neither > the commit message for TPM give a clue on this. Actually, I *do > not care* if it is RO and RW but I'm neither good at guessing > random shit. > You were cc'ed on the rest of the series, no? Shall we clarify this first, before proposing patches that introduce new ioctls() and kernel command line parameters to a security sensitive subsystem? My reading of 19/20 is that the secure launch module sets the default locality, and given that it can be built as a module, setting the default locality needs to be exported to modules (but as I indicated, this should probably be in a TPM internal module namespace) If setting the default locality from user space is a requirement down the road, we can discuss it then. For now, let's not go off into the weeds and derail this series even more.
On Sat Nov 2, 2024 at 12:52 PM EET, Ard Biesheuvel wrote: > > Short answer: I have no idea. I would not mind that but neither > > the commit message for TPM give a clue on this. Actually, I *do > > not care* if it is RO and RW but I'm neither good at guessing > > random shit. > > > > You were cc'ed on the rest of the series, no? Yeah, but that does not make sysfs attribute having store operation less confusing. At minimum 2/2 should replace the current sysfs patch, if store operation is not required. > Shall we clarify this first, before proposing patches that introduce > new ioctls() and kernel command line parameters to a security > sensitive subsystem? > > My reading of 19/20 is that the secure launch module sets the default > locality, and given that it can be built as a module, setting the > default locality needs to be exported to modules (but as I indicated, > this should probably be in a TPM internal module namespace) > > If setting the default locality from user space is a requirement down > the road, we can discuss it then. For now, let's not go off into the > weeds and derail this series even more. If sysfs store is not required after all, and only thing that touches the locality is slmodule, tweaking 17/20's set operation to this would be good enough for me: int tpm_chip_set_locality(struct tpm_chip *chip, u8 locality) { int ret; if (locality >= TPM_MAX_LOCALITY) return false; ret = tpm_try_get_ops(chip); if (ret) return ret; chip->default_locality = locality; tpm_put_ops(chip); return 0; } EXPORT_SYMBOL_GPL(tpm_chip_set_locality); Now that I've worked on this issue I think also 15/20 and 16/20 are pretty clear I can suggest some tweaks to the commit messages later to make then more self-explatonery. BR, Jarkko
On Sat Nov 2, 2024 at 3:39 PM EET, Jarkko Sakkinen wrote: > int tpm_chip_set_locality(struct tpm_chip *chip, u8 locality) > { > int ret; > > if (locality >= TPM_MAX_LOCALITY) > return false; > > ret = tpm_try_get_ops(chip); > if (ret) > return ret; > > chip->default_locality = locality; > > tpm_put_ops(chip); > return 0; > } > EXPORT_SYMBOL_GPL(tpm_chip_set_locality); Other things to take from 1/2 of my RFC to this: 1. Must be one-shot. 2. Must be only for tpm_tis as this is made to work only with that driver. E.g. 15/20 is only for tpm_tis. I guess that is the main target anyway here. Future patch sets can extend this to other drivers. TPM_CHIP_FLAG_SET_LOCALITY_ENABLED use in 1/2 can be referenced for a solution. Kernel command-line parameter: I agree not having it if no need for ioctl, so that is addressed too. BR, Jarkko
On Sat Nov 2, 2024 at 12:38 PM EET, Jarkko Sakkinen wrote: > On Sat Nov 2, 2024 at 11:02 AM EET, Ard Biesheuvel wrote: > > Same for the ioctl() [as well as the read-write sysfs node]: looking > > at the code (patch 19/20) it doesn't seem like user space needs to be > > able to modify this at all, at least not for the patch set as > > presented. So for now, can we just stick with making the sysfs node > > read-only? > > Short answer: I have no idea. I would not mind that but neither > the commit message for TPM give a clue on this. Actually, I *do > not care* if it is RO and RW but I'm neither good at guessing > random shit. > > I haad to assume it was *needed* for reason that I do not know > given that sysfs attribute was RW. Let's put it this way: *if* write is needed this the way to do it now and also in the future (or along the lines). Or least harmful at least (single additional locality change per boot). BR, Jarkko
From: Ross Philipson <ross.philipson@oracle.com>
Expose the default locality as read-only attribute through sysfs.
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- A new patch.
- Import from the original Trenchboot patch set.
- Drop the store operation.
- Tested with https://codeberg.org/jarkko/tpm-set-locality-test/src/branch/main/src/main.rs
---
drivers/char/tpm/tpm-sysfs.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 94231f052ea7..4f5e60b1e4c9 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -309,6 +309,14 @@ static ssize_t tpm_version_major_show(struct device *dev,
}
static DEVICE_ATTR_RO(tpm_version_major);
+static ssize_t locality_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct tpm_chip *chip = to_tpm_chip(dev);
+
+ return sprintf(buf, "%d\n", chip->default_locality);
+}
+static DEVICE_ATTR_RO(locality);
+
#ifdef CONFIG_TCG_TPM2_HMAC
static ssize_t null_name_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -336,6 +344,7 @@ static struct attribute *tpm1_dev_attrs[] = {
&dev_attr_durations.attr,
&dev_attr_timeouts.attr,
&dev_attr_tpm_version_major.attr,
+ &dev_attr_locality.attr,
NULL,
};
@@ -344,6 +353,7 @@ static struct attribute *tpm2_dev_attrs[] = {
#ifdef CONFIG_TCG_TPM2_HMAC
&dev_attr_null_name.attr,
#endif
+ &dev_attr_locality.attr,
NULL
};
--
2.47.0
© 2016 - 2024 Red Hat, Inc.