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