[RFC,v2,1/2] tpm, tpm_tis: Introduce TPM_IOC_SET_LOCALITY

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

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 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 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