| Message ID | 20241102062259.2521361-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [RFC,v2,1/2] tpm, tpm_tis: Introduce TPM_IOC_SET_LOCALITY | expand |
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 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
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
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 */
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