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

[Ard Biesheuvel]

On Sat, 2 Nov 2024 at 07:29, Jarkko Sakkinen <jarkko at 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 at 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.