* tpm2sh 0.12.3
From: Jarkko Sakkinen @ 2025-10-27 8:56 UTC (permalink / raw)
To: tpm2; +Cc: linux-integrity
tpm2sh 0.12.3 loads and processes multi-level ancestor chains correctly,
policy expressions have now a more stable custom-built implemented
parser:
https://crates.io/crates/tpm2sh
The functionality is still mostly focused on key management and TPMKey
ASN.1 generation from various internal and external sources, making
it a tool with kernel testing focus.
I've added optional parentPubkey field to the original TPMKey ASN.1
because that allows "tpm2sh load" to be ergonomic as it can traverse
persistent and cached keys and discover parent, instead of requiring
explicit '--parent' argument.
This transcript demonstrates well the added ergonomics:
~ main ≡
❯ tpm2sh create-primary ecc-nist-p256:sha256
vtpm:80000000
~ main ≡
❯ set DIGEST (tpm2sh policy --mode software 'pcr(sha256:7) or pcr(sha256:15)')
~ main ≡
❯ tpm2sh create -P vtpm:80000000 --data deadbeef --policy $DIGEST keyedhash:sha256 | tpm2sh load
vtpm:80000001
~ main ≡
❯ tpm2sh policy --mode session 'pcr(sha256:7) or pcr(sha256:15)'
vtpm:03000000
~ main ≡
❯ tpm2sh unseal -A vtpm:03000000 vtpm:80000001
deadbeef
~ main ≡
❯ openssl genrsa -out private.pem 2048
~ main ≡
❯ tpm2sh convert -P vtpm:80000000 -I private.pem | tpm2sh load
vtpm:80000002
Without the extra field, also load commands would need to have
"-P vtpm:80000000".
BR, Jarkko
^ permalink raw reply
* [PATCH AUTOSEL 6.17-6.6] ima: don't clear IMA_DIGSIG flag when setting or removing non-IMA xattr
From: Sasha Levin @ 2025-10-25 15:57 UTC (permalink / raw)
To: patches, stable
Cc: Coiby Xu, Mimi Zohar, Sasha Levin, roberto.sassu, dmitry.kasatkin,
linux-integrity
In-Reply-To: <20251025160905.3857885-1-sashal@kernel.org>
From: Coiby Xu <coxu@redhat.com>
[ Upstream commit 88b4cbcf6b041ae0f2fc8a34554a5b6a83a2b7cd ]
Currently when both IMA and EVM are in fix mode, the IMA signature will
be reset to IMA hash if a program first stores IMA signature in
security.ima and then writes/removes some other security xattr for the
file.
For example, on Fedora, after booting the kernel with "ima_appraise=fix
evm=fix ima_policy=appraise_tcb" and installing rpm-plugin-ima,
installing/reinstalling a package will not make good reference IMA
signature generated. Instead IMA hash is generated,
# getfattr -m - -d -e hex /usr/bin/bash
# file: usr/bin/bash
security.ima=0x0404...
This happens because when setting security.selinux, the IMA_DIGSIG flag
that had been set early was cleared. As a result, IMA hash is generated
when the file is closed.
Similarly, IMA signature can be cleared on file close after removing
security xattr like security.evm or setting/removing ACL.
Prevent replacing the IMA file signature with a file hash, by preventing
the IMA_DIGSIG flag from being reset.
Here's a minimal C reproducer which sets security.selinux as the last
step which can also replaced by removing security.evm or setting ACL,
#include <stdio.h>
#include <sys/xattr.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
int main() {
const char* file_path = "/usr/sbin/test_binary";
const char* hex_string = "030204d33204490066306402304";
int length = strlen(hex_string);
char* ima_attr_value;
int fd;
fd = open(file_path, O_WRONLY|O_CREAT|O_EXCL, 0644);
if (fd == -1) {
perror("Error opening file");
return 1;
}
ima_attr_value = (char*)malloc(length / 2 );
for (int i = 0, j = 0; i < length; i += 2, j++) {
sscanf(hex_string + i, "%2hhx", &ima_attr_value[j]);
}
if (fsetxattr(fd, "security.ima", ima_attr_value, length/2, 0) == -1) {
perror("Error setting extended attribute");
close(fd);
return 1;
}
const char* selinux_value= "system_u:object_r:bin_t:s0";
if (fsetxattr(fd, "security.selinux", selinux_value, strlen(selinux_value), 0) == -1) {
perror("Error setting extended attribute");
close(fd);
return 1;
}
close(fd);
return 0;
}
Signed-off-by: Coiby Xu <coxu@redhat.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
YES – this is a focused bug fix that should go to stable.
- The core issue is that `ima_reset_appraise_flags()` used to treat the
`digsig` argument as boolean, so any non-IMA xattr update passed the
default `0` and cleared `IMA_DIGSIG`, triggering `ima_update_xattr()`
to overwrite an existing signature with a hash in fix mode (see
`security/integrity/ima/ima_appraise.c:628-646`). The reported Fedora
reproducer shows this breaks IMA/EVM deployments running
`ima_appraise=fix`.
- The patch makes `digsig` tri-state: `ima_reset_appraise_flags()` now
only toggles the bit when given `0`/`1`, leaving it untouched for `-1`
(`security/integrity/ima/ima_appraise.c:706-721`). All non-IMA paths
(generic xattrs, ACL set/remove, non-IMA removals) now pass `-1`, so
they still force re-appraisal via `IMA_CHANGE_XATTR` but stop
clobbering the signature
(`security/integrity/ima/ima_appraise.c:788-835`).
- Actual signature operations keep their old behavior: setting
`security.ima` still sets `IMA_DIGSIG`, and removing it still clears
the flag (`security/integrity/ima/ima_appraise.c:793-835`), so there’s
no functional change for legitimate signature updates or removals.
- I checked older releases (e.g. `v6.1`) and they still have the pre-
patch boolean handling, so the regression affects long-term stable
trees. No new APIs or dependencies are introduced; the change is
confined to one file and keeps existing call sites in sync.
- Risk is low: the patch just stops clearing the digital-signature bit
for unrelated xattr/ACL changes, which is precisely what breakage
reports show is required. It preserves hash-update behavior for
unsigned files and keeps the IMA/EVM synchronization logic intact.
Given the user-visible security impact (signed packages ending up with
only hashes) and the minimal, targeted fix, this commit is a strong
stable backport candidate. Suggested follow-up: validate on a stable
branch with `ima_appraise=fix evm=fix` to ensure reference signatures
persist across SELinux xattr/ACL churn.
security/integrity/ima/ima_appraise.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index f435eff4667f8..5149ff4fd50d2 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -694,6 +694,15 @@ static int ima_protect_xattr(struct dentry *dentry, const char *xattr_name,
return 0;
}
+/*
+ * ima_reset_appraise_flags - reset ima_iint_cache flags
+ *
+ * @digsig: whether to clear/set IMA_DIGSIG flag, tristate values
+ * 0: clear IMA_DIGSIG
+ * 1: set IMA_DIGSIG
+ * -1: don't change IMA_DIGSIG
+ *
+ */
static void ima_reset_appraise_flags(struct inode *inode, int digsig)
{
struct ima_iint_cache *iint;
@@ -706,9 +715,9 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
return;
iint->measured_pcrs = 0;
set_bit(IMA_CHANGE_XATTR, &iint->atomic_flags);
- if (digsig)
+ if (digsig == 1)
set_bit(IMA_DIGSIG, &iint->atomic_flags);
- else
+ else if (digsig == 0)
clear_bit(IMA_DIGSIG, &iint->atomic_flags);
}
@@ -794,6 +803,8 @@ static int ima_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG);
} else if (!strcmp(xattr_name, XATTR_NAME_EVM) && xattr_value_len > 0) {
digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
+ } else {
+ digsig = -1;
}
if (result == 1 || evm_revalidate_status(xattr_name)) {
ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
@@ -807,7 +818,7 @@ static int ima_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
const char *acl_name, struct posix_acl *kacl)
{
if (evm_revalidate_status(acl_name))
- ima_reset_appraise_flags(d_backing_inode(dentry), 0);
+ ima_reset_appraise_flags(d_backing_inode(dentry), -1);
return 0;
}
@@ -815,11 +826,13 @@ static int ima_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
static int ima_inode_removexattr(struct mnt_idmap *idmap, struct dentry *dentry,
const char *xattr_name)
{
- int result;
+ int result, digsig = -1;
result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
if (result == 1 || evm_revalidate_status(xattr_name)) {
- ima_reset_appraise_flags(d_backing_inode(dentry), 0);
+ if (!strcmp(xattr_name, XATTR_NAME_IMA))
+ digsig = 0;
+ ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
if (result == 1)
result = 0;
}
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>
From: Jarkko Sakkinen @ 2025-10-24 18:55 UTC (permalink / raw)
To: Jonathan McDowell
Cc: Peter Huewe, Jason Gunthorpe, Linus Torvalds, James Bottomley,
linux-integrity, linux-kernel
In-Reply-To: <cover.1760958898.git.noodles@meta.com>
On Mon, Oct 20, 2025 at 12:30:32PM +0100, Jonathan McDowell wrote:
> I hit a problem where ~ 1% of TPM firmware upgrades were failing.
> Investigating revealed the issue was that although the upgrade tool uses
> /dev/tpm0 this does not actually prevent access via /dev/tpmrm0, nor
> internal kernel users. It *does* prevent access to others via /dev/tpm0
>
> So the upgrade process started, the HW RNG came in to get some
> randomness in the middle, did the HMAC context dance, and confused
> everything to the point the TPM was no longer visible to the OS even
> after a reboot.
>
> Thankfully I've been able to recover those devices, but really what I'd
> like is the ability for a userspace tool to exclusively access the TPM
> without something coming in behind it. Given the lightweight attempt at
> locking that already exists I think this was the original intention.
>
> I've reworked this series based on feedback received.
>
> Firstly, it's been reordered TPM sharing functionality doesn't break
> during bisection.
>
> Secondly, the O_EXCL check has been tightend up to ensure the caller is
> also opening the device O_RDWR. Callers shouldn't really be opening the
> TPM except for reading + writing, but this should help guard against
> unexpected flags usage a bit more.
>
> Finally, this revision keeps the prohibition on more than one user of
> /dev/tpm#, to avoid unexpected breakages for clients that expect this to
> guard against multiple invocations. A client only then needs to use
> O_EXCL if it wants to prevent *all* other access, even with
> ContextSaves, such as the firmware upgrade case.
>
> (I've sent a separate standalone patch that allows the TPM HW RNG to be
> disabled at run time, and it's now in -next, but even with that I think
> something like this is a good idea as well.)
>
> Jonathan McDowell (4):
> tpm: Remove tpm_find_get_ops
> tpm: Add O_EXCL for exclusive /dev/tpm access
> tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM
> access
> tpm: Allow for exclusive TPM access when using /dev/tpm<n>
>
> drivers/char/tpm/tpm-chip.c | 90 +++++++++++++++----------------
> drivers/char/tpm/tpm-dev-common.c | 8 +--
> drivers/char/tpm/tpm-dev.c | 35 ++++++++++--
> drivers/char/tpm/tpm-dev.h | 1 +
> drivers/char/tpm/tpm-interface.c | 20 +++++--
> drivers/char/tpm/tpm.h | 3 +-
> drivers/char/tpm/tpm2-space.c | 5 +-
> drivers/char/tpm/tpm_tis_core.c | 3 +-
> drivers/char/tpm/tpmrm-dev.c | 20 ++++++-
> include/linux/tpm.h | 4 +-
> 10 files changed, 124 insertions(+), 65 deletions(-)
>
> --
> 2.51.0
>
I will put to queue with my tags but I just want to make first sure that we do not
break anything.
I'll upgrade my test suite first to have TPM 1.2 tests (which is also
needed for my own series) and run it in bunch of configurations. And on
TPM2 I check the behavior with TSS2 daemon on / off.
I have no doubts on the code changes, and it is most importantly a
security improvement, given that "who has the access and how long"
can be deduced for a system configuration. I just feel that with
this code change it is better to check and verify everything :-)
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v6 01/10] tpm: Cap the number of PCR banks
From: Jarkko Sakkinen @ 2025-10-24 18:50 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, keyring, dpsmith, ross.philipson, Roberto Sassu,
Stefano Garzarella, Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe,
linux-kernel
In-Reply-To: <aPYg1N0TvrkG6AJI@earth.li>
On Mon, Oct 20, 2025 at 12:45:24PM +0100, Jonathan McDowell wrote:
> On Sat, Oct 18, 2025 at 02:17:16PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > tpm2_get_pcr_allocation() does not cap any upper limit for the number of
> > banks. Cap the limit to four banks so that out of bounds values coming
> > from external I/O cause on only limited harm.
>
> Comment no longer matches code - we cap at 8. With the comment fixed:
>
> Reviewed-By: Jonathan McDowell <noodles@meta.com>
Oops, sorry I'll address that and add your tag! Thank you.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v6 10/10] tpm-buf: Enable managed and stack allocations.
From: Jarkko Sakkinen @ 2025-10-24 18:49 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-integrity, keyring, dpsmith, ross.philipson,
Jonathan McDowell, Roberto Sassu, Jarkko Sakkinen, Stefan Berger,
Peter Huewe, Jason Gunthorpe, James Bottomley, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
linux-kernel, keyrings, linux-security-module
In-Reply-To: <yynqxoqux5whcbsnticikhwmupqh57xfbll5egzkn42kj7gkaf@s4kwxfmto5ia>
On Mon, Oct 20, 2025 at 11:04:51AM +0200, Stefano Garzarella wrote:
> On Sat, Oct 18, 2025 at 02:17:25PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Decouple kzalloc from buffer creation, so that a managed allocation can be
> > done trivially:
> >
> > struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUFSIZE,
> > GFP_KERNEL);
> > if (!buf)
> > return -ENOMEM;
> > tpm_buf_init(buf, TPM_BUFSIZE);
> >
> > Alternatively, stack allocations are also possible:
> >
> > u8 buf_data[512];
> > struct tpm_buf *buf = (struct tpm_buf *)buf_data;
> > tpm_buf_init(buf, sizeof(buf_data));
> >
> > Given that every single tpm_transmit_cmd() call site needs to be changed,
> > place command names from TCG 1.2 and 2.0 specifications to the @dest
> > parameter, which will e.g., help tracing bugs.
>
> Perhaps my previous message fell through the cracks, but I still have a
> couple of comments (perhaps trivial, sorry in that case) that have not been
> answered about this patch:
I think what happened is that there was enough time that I forgot what
I had or hadn't done :-) I'll address your comments.
>
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> > v6
> > - Update commit message.
> > v5:
> > - There was a spurious change in tpm2_seal_trusted() error
> > code handling introduce by this patch.
> > v4:
> > - Since every single tpm_transmit_cmd() call site needs to be
> > changed anyhow, use 'dest' parameter more structured and
> > actually useful way, and pick the string TCG 1.2 and 2.0
> > specifications.
> > - tpm1-cmd: Remove useless rc declarations and repliace them
> > with trivial "return tpm_transmit_cmd" statement.
> > - Reverted spurious changes in include/linux/tpm.h.
> > - Use concisely TPM_BUFSIZE instead of PAGE_SIZE.
> > v3:
> > - A new patch from the earlier series with more scoped changes and
> > less abstract commit message.
> > ---
> > drivers/char/tpm/tpm-buf.c | 122 +++++----
> > drivers/char/tpm/tpm-sysfs.c | 21 +-
> > drivers/char/tpm/tpm.h | 1 -
> > drivers/char/tpm/tpm1-cmd.c | 162 +++++-------
> > drivers/char/tpm/tpm2-cmd.c | 299 ++++++++++------------
> > drivers/char/tpm/tpm2-sessions.c | 122 +++++----
> > drivers/char/tpm/tpm2-space.c | 44 ++--
> > drivers/char/tpm/tpm_vtpm_proxy.c | 30 +--
> > include/linux/tpm.h | 18 +-
> > security/keys/trusted-keys/trusted_tpm1.c | 34 ++-
> > security/keys/trusted-keys/trusted_tpm2.c | 175 ++++++-------
> > 11 files changed, 484 insertions(+), 544 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index 1b9dee0d0681..a3bf3c3d0c48 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
>
> [...]
>
> > @@ -92,6 +119,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> > */
> > u32 tpm_buf_length(struct tpm_buf *buf)
>
> Should we update the return value to u16?
Ack.
>
>
> > {
> > + if (buf->flags & TPM_BUF_INVALID)
> > + return 0;
> > +
> > return buf->length;
> > }
> > EXPORT_SYMBOL_GPL(tpm_buf_length);
>
> [...]
>
> > diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> > index 636acb66a4f6..3ac204a902de 100644
> > --- a/security/keys/trusted-keys/trusted_tpm1.c
> > +++ b/security/keys/trusted-keys/trusted_tpm1.c
> > @@ -310,9 +310,8 @@ static int TSS_checkhmac2(unsigned char *buffer,
> > * For key specific tpm requests, we will generate and send our
> > * own TPM command packets using the drivers send function.
> > */
> > -static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > +static int trusted_tpm_send(void *cmd, size_t buflen)
> > {
> > - struct tpm_buf buf;
> > int rc;
> >
> > if (!chip)
> > @@ -322,15 +321,12 @@ static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > if (rc)
> > return rc;
> >
> > - buf.flags = 0;
> > - buf.length = buflen;
> > - buf.data = cmd;
> > dump_tpm_buf(cmd);
> > - rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
> > + rc = tpm_transmit_cmd(chip, cmd, 4, "sending data");
>
> Is it fine here to remove the intermediate tpm_buf ?
>
> IIUC tpm_transmit_cmd() needs a tpm_buf, while here we are passing just
> the "data", or in some way it's a nested tpm_buf?
This does not look right at all. I'll fix it and add automated test
suite for TPM 1.x to my standard test shenanigans so that I can
continuously test its correctness [1]. It anyhow should have TPM 1.2
tests (especially for trusted keys). As it becomes more rare
accidental bugs could easily hover in.
Thanks, this was really good catch. I'll address both bug and make
sure that I don't do the same mistake twice :-)
>
> > dump_tpm_buf(cmd);
> >
> > + /* Convert TPM error to -EPERM. */
> > if (rc > 0)
> > - /* TPM error */
> > rc = -EPERM;
> >
> > tpm_put_ops(chip);
>
> Thanks,
> Stefano
>
[1] https://codeberg.org/jarkko/linux-tpmdd-test
BR, Jarkko
^ permalink raw reply
* [PATCH v2] tpm_crb: Add idle support for the Arm FF-A start method
From: Stuart Yoder @ 2025-10-24 17:42 UTC (permalink / raw)
To: linux-integrity, jarkko, peterhuewe, jgg, sudeep.holla
Cc: Prachotan.Bathi, linux-kernel
According to the CRB over FF-A specification [1], a TPM that implements
the ABI must comply with the TCG PTP specification. This requires support
for the Idle and Ready states.
This patch implements CRB control area requests for goIdle and
cmdReady on FF-A based TPMs.
The FF-A message used to notify the TPM of CRB updates includes a
locality parameter, which provides a hint to the TPM about which
locality modified the CRB. This patch adds a locality parameter
to __crb_go_idle() and __crb_cmd_ready() to support this.
[1] https://developer.arm.com/documentation/den0138/latest/
Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
-v2: add kernel doc inf for new loc parameter in __crb_go_idle
and __crb_cmd_ready
drivers/char/tpm/tpm_crb.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 876edf2705ab..6b89bd183a3a 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -133,8 +133,7 @@ static inline bool tpm_crb_has_idle(u32 start_method)
{
return !(start_method == ACPI_TPM2_START_METHOD ||
start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
- start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC ||
- start_method == ACPI_TPM2_CRB_WITH_ARM_FFA);
+ start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC);
}
static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
@@ -180,6 +179,7 @@ static int crb_try_pluton_doorbell(struct crb_priv *priv, bool wait_for_complete
*
* @dev: crb device
* @priv: crb private data
+ * @loc: locality
*
* Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
* The device should respond within TIMEOUT_C by clearing the bit.
@@ -191,7 +191,7 @@ static int crb_try_pluton_doorbell(struct crb_priv *priv, bool wait_for_complete
*
* Return: 0 always
*/
-static int __crb_go_idle(struct device *dev, struct crb_priv *priv)
+static int __crb_go_idle(struct device *dev, struct crb_priv *priv, int loc)
{
int rc;
@@ -200,6 +200,12 @@ static int __crb_go_idle(struct device *dev, struct crb_priv *priv)
iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
+ if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+ rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_COMMAND, loc);
+ if (rc)
+ return rc;
+ }
+
rc = crb_try_pluton_doorbell(priv, true);
if (rc)
return rc;
@@ -220,7 +226,7 @@ static int crb_go_idle(struct tpm_chip *chip)
struct device *dev = &chip->dev;
struct crb_priv *priv = dev_get_drvdata(dev);
- return __crb_go_idle(dev, priv);
+ return __crb_go_idle(dev, priv, chip->locality);
}
/**
@@ -228,6 +234,7 @@ static int crb_go_idle(struct tpm_chip *chip)
*
* @dev: crb device
* @priv: crb private data
+ * @loc: locality
*
* Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
* and poll till the device acknowledge it by clearing the bit.
@@ -238,7 +245,7 @@ static int crb_go_idle(struct tpm_chip *chip)
*
* Return: 0 on success -ETIME on timeout;
*/
-static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv)
+static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv, int loc)
{
int rc;
@@ -247,6 +254,12 @@ static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv)
iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
+ if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+ rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_COMMAND, loc);
+ if (rc)
+ return rc;
+ }
+
rc = crb_try_pluton_doorbell(priv, true);
if (rc)
return rc;
@@ -267,7 +280,7 @@ static int crb_cmd_ready(struct tpm_chip *chip)
struct device *dev = &chip->dev;
struct crb_priv *priv = dev_get_drvdata(dev);
- return __crb_cmd_ready(dev, priv);
+ return __crb_cmd_ready(dev, priv, chip->locality);
}
static int __crb_request_locality(struct device *dev,
@@ -444,7 +457,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
/* Seems to be necessary for every command */
if (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON)
- __crb_cmd_ready(&chip->dev, priv);
+ __crb_cmd_ready(&chip->dev, priv, chip->locality);
memcpy_toio(priv->cmd, buf, len);
@@ -672,7 +685,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
* PTT HW bug w/a: wake up the device to access
* possibly not retained registers.
*/
- ret = __crb_cmd_ready(dev, priv);
+ ret = __crb_cmd_ready(dev, priv, 0);
if (ret)
goto out_relinquish_locality;
@@ -744,7 +757,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
if (!ret)
priv->cmd_size = cmd_size;
- __crb_go_idle(dev, priv);
+ __crb_go_idle(dev, priv, 0);
out_relinquish_locality:
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-24 15:16 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <559f6ebf4a19da321fffc2a3ca180dc3d6216a22.camel@linux.ibm.com>
On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
> On Sat, 2025-10-18 at 07:19 +0800, Coiby Xu wrote:
> > > > > 2. Instead of defining an additional process_measurement() argument to identify
> > > > > compressed kernel modules, to simplify the code it might be possible to define a
> > > > > new "func" named COMPRESSED_MODULE_CHECK.
> > > > >
> > > > > + [READING_COMPRESSED_MODULE] = MODULE_CHECK, -> COMPRESSED_MODULE_CHECK
> > > >
> > > > I also thought about this approach. But IMA rule maps kernel module
> > > > loading to MODULE_CHECK. If we define a new rule and ask users to use
> > > > this new rule, ima_policy=secure_boot still won't work.
> > >
> > > I don't have a problem with extending the "secure-boot" policy to support
> > > uncompressed kernel modules appended signatures, based on whether
> > > CONFIG_MODULE_SIG is enabled. The new rule would be in addition to the existing
> > > MODULE_CHECK rule.
> >
> > I assume once the new rule get added, we can't remove it for userspace
> > backward compatibility, right? And with CPIO xattr supported, it seems
> > there is no need to keep this rule. So if this concern is valid, do you
> > think we shall switch to another approach i.e. to make IMA support
> > verifying decompressed module and then make "secure-boot" to allow
> > appended module signature?
>
> Yes, once the rule is added, it wouldn't be removed. As for "to make IMA
> support verifying decompressed module", yes that might be a better solution,
> than relying on "sig_enforce" being enabled. IMA already supports verifying the
> appended signatures. A new IMA specific or LSM hook would need to be defined
> after module_decompress().
Looking at the code further, decompressing the kernel module in IMA is
redundant. Instead I think the best approach would be to:
- define DECOMPRESSED_MODULE, in addition to COMPRESSED_MODULE.
id(COMPRESSED_MODULE, compressed-kernel-module) \
id(DECOMPRESSED_MODULE, decompressed-kernel-module) \
- instead of passing a boolean indicating whether the module is compressed, pass
the kernel_read_file_id enumeration to differentiate between the compressed and
decompressed module.
- define a new IMA hook, probably LSM hook as well, named
ima_decompressed_module().
- call the new ima_decompressed_module() from init_module_from_file()
immediately after decompressing the kernel module. Something along the lines
of:
err = ima_decompressed_module(f, (char *)info.hdr, info.len,
READING_DECOMPRESSED_MODULE);
For testing purposes to see the decompressed appended signature in the
measurement list, modify the MODULE_CHECK measure rule to include "template=ima-
modsig" in ima_efi.c.
--
Mimi
^ permalink raw reply
* Re: [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: Ahmad Fatoum @ 2025-10-24 8:10 UTC (permalink / raw)
To: yebin, kernel, James.Bottomley, jarkko, zohar, dhowells, paul,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
yebin10
In-Reply-To: <68FB2470.4000206@huaweicloud.com>
Hello,
On 10/24/25 9:02 AM, yebin wrote:
> Ignore this patch as 275a9a3f9b6a（“KEYS: trusted: Pass argument by
> pointer in dump_options”）already fix this issue.
What tree are you looking at? I can't find this commit in my git and the
code you are purportedly patching never existed upstream.
If you run into issues exclusive to a vendor fork, you need to submit
your patches to the vendor. The upstream mailing lists are for upstream.
Thanks,
Ahmad
>
> On 2025/10/24 14:11, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> There's issue as follows:
>> security/keys/trusted-keys/trusted_caam.c: In function ‘dump_options’:
>> security/keys/trusted-keys/trusted_caam.c:37:20: note: the ABI of
>> passing struct with a flexible array member has changed in GCC 4.4
>> 37 | static inline void dump_options(struct caam_pkey_info pkey_info)
>> | ^~~~~~~~~~~~
>>
>> To solve the above problem, pass 'struct caam_pkey_info*' type parameter
>> to the dump_options() function.
>>
>> Fixes: 9eb25ca6c973 ("KEYS: trusted: caam based protected key")
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>> security/keys/trusted-keys/trusted_caam.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/
>> keys/trusted-keys/trusted_caam.c
>> index 090099d1b04d..dd7a69bcf6a3 100644
>> --- a/security/keys/trusted-keys/trusted_caam.c
>> +++ b/security/keys/trusted-keys/trusted_caam.c
>> @@ -29,12 +29,12 @@ static const match_table_t key_tokens = {
>> };
>>
>> #ifdef CAAM_DEBUG
>> -static inline void dump_options(struct caam_pkey_info pkey_info)
>> +static inline void dump_options(struct caam_pkey_info *pkey_info)
>> {
>> - pr_info("key encryption algo %d\n", pkey_info.key_enc_algo);
>> + pr_info("key encryption algo %d\n", pkey_info->key_enc_algo);
>> }
>> #else
>> -static inline void dump_options(struct caam_pkey_info pkey_info)
>> +static inline void dump_options(struct caam_pkey_info *pkey_info)
>> {
>> }
>> #endif
>> @@ -108,7 +108,7 @@ static int trusted_caam_seal(struct
>> trusted_key_payload *p, char *datablob)
>> ret = get_pkey_options(datablob, &info.pkey_info);
>> if (ret < 0)
>> return 0;
>> - dump_options(info.pkey_info);
>> + dump_options(&info.pkey_info);
>> }
>>
>> ret = caam_encap_blob(blobifier, &info);
>> @@ -140,7 +140,7 @@ static int trusted_caam_unseal(struct
>> trusted_key_payload *p, char *datablob)
>> ret = get_pkey_options(datablob, &info.pkey_info);
>> if (ret < 0)
>> return 0;
>> - dump_options(info.pkey_info);
>> + dump_options(&info.pkey_info);
>>
>> p->key_len = p->blob_len + sizeof(struct caam_pkey_info);
>> memcpy(p->key, &info.pkey_info, sizeof(struct caam_pkey_info));
>>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: yebin @ 2025-10-24 7:02 UTC (permalink / raw)
To: a.fatoum, kernel, James.Bottomley, jarkko, zohar, dhowells, paul,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
yebin10
In-Reply-To: <20251024061153.61470-1-yebin@huaweicloud.com>
Ignore this patch as 275a9a3f9b6a（“KEYS: trusted: Pass argument by
pointer in dump_options”）already fix this issue.
On 2025/10/24 14:11, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> There's issue as follows:
> security/keys/trusted-keys/trusted_caam.c: In function ‘dump_options’:
> security/keys/trusted-keys/trusted_caam.c:37:20: note: the ABI of passing struct with a flexible array member has changed in GCC 4.4
> 37 | static inline void dump_options(struct caam_pkey_info pkey_info)
> | ^~~~~~~~~~~~
>
> To solve the above problem, pass 'struct caam_pkey_info*' type parameter
> to the dump_options() function.
>
> Fixes: 9eb25ca6c973 ("KEYS: trusted: caam based protected key")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> security/keys/trusted-keys/trusted_caam.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/keys/trusted-keys/trusted_caam.c
> index 090099d1b04d..dd7a69bcf6a3 100644
> --- a/security/keys/trusted-keys/trusted_caam.c
> +++ b/security/keys/trusted-keys/trusted_caam.c
> @@ -29,12 +29,12 @@ static const match_table_t key_tokens = {
> };
>
> #ifdef CAAM_DEBUG
> -static inline void dump_options(struct caam_pkey_info pkey_info)
> +static inline void dump_options(struct caam_pkey_info *pkey_info)
> {
> - pr_info("key encryption algo %d\n", pkey_info.key_enc_algo);
> + pr_info("key encryption algo %d\n", pkey_info->key_enc_algo);
> }
> #else
> -static inline void dump_options(struct caam_pkey_info pkey_info)
> +static inline void dump_options(struct caam_pkey_info *pkey_info)
> {
> }
> #endif
> @@ -108,7 +108,7 @@ static int trusted_caam_seal(struct trusted_key_payload *p, char *datablob)
> ret = get_pkey_options(datablob, &info.pkey_info);
> if (ret < 0)
> return 0;
> - dump_options(info.pkey_info);
> + dump_options(&info.pkey_info);
> }
>
> ret = caam_encap_blob(blobifier, &info);
> @@ -140,7 +140,7 @@ static int trusted_caam_unseal(struct trusted_key_payload *p, char *datablob)
> ret = get_pkey_options(datablob, &info.pkey_info);
> if (ret < 0)
> return 0;
> - dump_options(info.pkey_info);
> + dump_options(&info.pkey_info);
>
> p->key_len = p->blob_len + sizeof(struct caam_pkey_info);
> memcpy(p->key, &info.pkey_info, sizeof(struct caam_pkey_info));
>
^ permalink raw reply
* [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: Ye Bin @ 2025-10-24 6:11 UTC (permalink / raw)
To: a.fatoum, kernel, James.Bottomley, jarkko, zohar, dhowells, paul,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
yebin, yebin10
From: Ye Bin <yebin10@huawei.com>
There's issue as follows:
security/keys/trusted-keys/trusted_caam.c: In function ‘dump_options’:
security/keys/trusted-keys/trusted_caam.c:37:20: note: the ABI of passing struct with a flexible array member has changed in GCC 4.4
37 | static inline void dump_options(struct caam_pkey_info pkey_info)
| ^~~~~~~~~~~~
To solve the above problem, pass 'struct caam_pkey_info*' type parameter
to the dump_options() function.
Fixes: 9eb25ca6c973 ("KEYS: trusted: caam based protected key")
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
security/keys/trusted-keys/trusted_caam.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/keys/trusted-keys/trusted_caam.c
index 090099d1b04d..dd7a69bcf6a3 100644
--- a/security/keys/trusted-keys/trusted_caam.c
+++ b/security/keys/trusted-keys/trusted_caam.c
@@ -29,12 +29,12 @@ static const match_table_t key_tokens = {
};
#ifdef CAAM_DEBUG
-static inline void dump_options(struct caam_pkey_info pkey_info)
+static inline void dump_options(struct caam_pkey_info *pkey_info)
{
- pr_info("key encryption algo %d\n", pkey_info.key_enc_algo);
+ pr_info("key encryption algo %d\n", pkey_info->key_enc_algo);
}
#else
-static inline void dump_options(struct caam_pkey_info pkey_info)
+static inline void dump_options(struct caam_pkey_info *pkey_info)
{
}
#endif
@@ -108,7 +108,7 @@ static int trusted_caam_seal(struct trusted_key_payload *p, char *datablob)
ret = get_pkey_options(datablob, &info.pkey_info);
if (ret < 0)
return 0;
- dump_options(info.pkey_info);
+ dump_options(&info.pkey_info);
}
ret = caam_encap_blob(blobifier, &info);
@@ -140,7 +140,7 @@ static int trusted_caam_unseal(struct trusted_key_payload *p, char *datablob)
ret = get_pkey_options(datablob, &info.pkey_info);
if (ret < 0)
return 0;
- dump_options(info.pkey_info);
+ dump_options(&info.pkey_info);
p->key_len = p->blob_len + sizeof(struct caam_pkey_info);
memcpy(p->key, &info.pkey_info, sizeof(struct caam_pkey_info));
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: Jonathan McDowell @ 2025-10-23 14:24 UTC (permalink / raw)
To: Roberto Sassu
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Linus Torvalds,
James Bottomley, linux-integrity, linux-kernel, zohar
In-Reply-To: <cec499d5130f37a7887d39b44efd8538dd361fe3.camel@huaweicloud.com>
On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
>On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
>> From: Jonathan McDowell <noodles@meta.com>
>>
>> There are situations where userspace might reasonably desire exclusive
>> access to the TPM, or the kernel's internal context saving + flushing
>> may cause issues, for example when performing firmware upgrades. Extend
>> the locking already used for avoiding concurrent userspace access to
>> prevent internal users of the TPM when /dev/tpm<n> is in use.
>>
>> The few internal users who already hold the open_lock are changed to use
>> tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
>> functions changing to obtain read access to the open_lock. We return
>> -EBUSY when another user has exclusive access, rather than adding waits.
>>
>> Signed-off-by: Jonathan McDowell <noodles@meta.com>
>> ---
>> v2: Switch to _locked instead of _internal_ for function names.
>> v3: Move to end of patch series.
>>
>> drivers/char/tpm/tpm-chip.c | 53 +++++++++++++++++++++++++------
>> drivers/char/tpm/tpm-dev-common.c | 8 ++---
>> drivers/char/tpm/tpm.h | 2 ++
>> drivers/char/tpm/tpm2-space.c | 5 ++-
>> 4 files changed, 52 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index ba906966721a..687f6d8cd601 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>> EXPORT_SYMBOL_GPL(tpm_chip_stop);
>>
>> /**
>> - * tpm_try_get_ops() - Get a ref to the tpm_chip
>> + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
>> * @chip: Chip to ref
>> *
>> * The caller must already have some kind of locking to ensure that chip is
>> @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
>> *
>> * Returns -ERRNO if the chip could not be got.
>> */
>> -int tpm_try_get_ops(struct tpm_chip *chip)
>> +int tpm_try_get_ops_locked(struct tpm_chip *chip)
>> {
>> int rc = -EIO;
>>
>> @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>> put_device(&chip->dev);
>> return rc;
>> }
>> -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>>
>> /**
>> - * tpm_put_ops() - Release a ref to the tpm_chip
>> + * tpm_put_ops_locked() - Release a ref to the tpm_chip
>> * @chip: Chip to put
>> *
>> - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
>> - * be kfree'd.
>> + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
>> + * chip may be kfree'd.
>> */
>> -void tpm_put_ops(struct tpm_chip *chip)
>> +void tpm_put_ops_locked(struct tpm_chip *chip)
>> {
>> tpm_chip_stop(chip);
>> mutex_unlock(&chip->tpm_mutex);
>> up_read(&chip->ops_sem);
>> put_device(&chip->dev);
>> }
>> +
>> +/**
>> + * tpm_try_get_ops() - Get a ref to the tpm_chip
>> + * @chip: Chip to ref
>> + *
>> + * The caller must already have some kind of locking to ensure that chip is
>> + * valid. This function will attempt to get the open_lock for the chip,
>> + * ensuring no other user is expecting exclusive access, before locking the
>> + * chip so that the ops member can be accessed safely. The locking prevents
>> + * tpm_chip_unregister from completing, so it should not be held for long
>> + * periods.
>> + *
>> + * Returns -ERRNO if the chip could not be got.
>> + */
>> +int tpm_try_get_ops(struct tpm_chip *chip)
>> +{
>> + if (!down_read_trylock(&chip->open_lock))
>> + return -EBUSY;
>
>Hi Jonathan
>
>do I understand it correctly, that a process might open the TPM with
>O_EXCL, and this will prevent IMA from extending a PCR until that
>process closes the file descriptor?
>
>If yes, this might be a concern, and I think an additional API to
>prevent such behavior would be needed (for example when IMA is active,
>i.e. there is a measurement policy loaded).
Yes, this definitely provides a path where userspace could prevent a
measurement hitting the TPM from IMA. I did think about this when
working out what to do if the lock was held elsewhere, but punted on
making any changes because there are several other avenues where that
can already happen.
>> +
>> + return tpm_try_get_ops_locked(chip);
>> +}
>> +EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>> +
>> +/**
>> + * tpm_put_ops() - Release a ref to the tpm_chip
>> + * @chip: Chip to put
>> + *
>> + * This is the opposite pair to tpm_try_get_ops(). After this returns
>> + * chip may be kfree'd.
>> + */
>> +void tpm_put_ops(struct tpm_chip *chip)
>> +{
>> + tpm_put_ops_locked(chip);
>> +
>> + up_read(&chip->open_lock);
>> +}
>> EXPORT_SYMBOL_GPL(tpm_put_ops);
>>
>> /**
>> @@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>> #ifdef CONFIG_TCG_TPM2_HMAC
>> int rc;
>>
>> - rc = tpm_try_get_ops(chip);
>> + rc = tpm_try_get_ops_locked(chip);
>> if (!rc) {
>> tpm2_end_auth_session(chip);
>> - tpm_put_ops(chip);
>> + tpm_put_ops_locked(chip);
>> }
>> #endif
>>
>> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
>> index f2a5e09257dd..0f5bc63411aa 100644
>> --- a/drivers/char/tpm/tpm-dev-common.c
>> +++ b/drivers/char/tpm/tpm-dev-common.c
>> @@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>>
>> mutex_lock(&priv->buffer_mutex);
>> priv->command_enqueued = false;
>> - ret = tpm_try_get_ops(priv->chip);
>> + ret = tpm_try_get_ops_locked(priv->chip);
>> if (ret) {
>> priv->response_length = ret;
>> goto out;
>> @@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>>
>> ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>> sizeof(priv->data_buffer));
>> - tpm_put_ops(priv->chip);
>> + tpm_put_ops_locked(priv->chip);
>>
>> /*
>> * If ret is > 0 then tpm_dev_transmit returned the size of the
>> @@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>> * lock during this period so that the tpm can be unregistered even if
>> * the char dev is held open.
>> */
>> - if (tpm_try_get_ops(priv->chip)) {
>> + if (tpm_try_get_ops_locked(priv->chip)) {
>> ret = -EPIPE;
>> goto out;
>> }
>>
>> ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>> sizeof(priv->data_buffer));
>> - tpm_put_ops(priv->chip);
>> + tpm_put_ops_locked(priv->chip);
>>
>> if (ret > 0) {
>> priv->response_length = ret;
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 02c07fef41ba..57ef8589f5f5 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>> const struct tpm_class_ops *ops);
>> struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
>> const struct tpm_class_ops *ops);
>> +int tpm_try_get_ops_locked(struct tpm_chip *chip);
>> +void tpm_put_ops_locked(struct tpm_chip *chip);
>> int tpm_chip_register(struct tpm_chip *chip);
>> void tpm_chip_unregister(struct tpm_chip *chip);
>>
>> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
>> index 60354cd53b5c..0ad5e18355e0 100644
>> --- a/drivers/char/tpm/tpm2-space.c
>> +++ b/drivers/char/tpm/tpm2-space.c
>> @@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>>
>> void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>> {
>> -
>> - if (tpm_try_get_ops(chip) == 0) {
>> + if (tpm_try_get_ops_locked(chip) == 0) {
>> tpm2_flush_sessions(chip, space);
>> - tpm_put_ops(chip);
>> + tpm_put_ops_locked(chip);
>> }
>>
>> kfree(space->context_buf);
>
J.
--
"I'm a compsci. I don't write code." -- Noodles. "I'm a DB coder.
Neither do I." -- Adrian.
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: Roberto Sassu @ 2025-10-23 13:35 UTC (permalink / raw)
To: James Bottomley, Thorsten Blum
Cc: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <e60f6a07d00c1fd87b4509947e8738ecab9560b4.camel@HansenPartnership.com>
On Thu, 2025-10-09 at 09:51 -0400, James Bottomley wrote:
> On Thu, 2025-10-09 at 15:30 +0200, Thorsten Blum wrote:
> > On 9. Oct 2025, at 14:44, James Bottomley wrote:
> > > On Thu, 2025-10-09 at 13:58 +0200, Thorsten Blum wrote:
> > > > Use designated initializers for 'key_format_tokens' and
> > > > 'key_tokens' to allow struct fields to be reordered more easily
> > >
> > > How does it improve that? The key,value pairs are surrounded by
> > > braces so we just cut and paste the lot anyway.
> >
> > Using designated initializers (especially for global structs) allows
> > the fields of struct match_token from linux/parser.h to be reordered
> > or extended more easily, improving overall maintainability.
>
> Why would we ever want to reorder them? The reason the ordering is
> {token, parser} string is because that's the nicest order to read them
> in.
I also join James regarding this. I find it fine as it is.
Consider also that there might be patches depending on this change that
cannot be automatically ported to stable kernels. Then, extra work is
required to find which dependencies are needed and backport them as
well.
Thanks
Roberto
> > > > and to improve readability.
> > >
> > > I don't think I agree with this when looking through the code,
> > > especially because this is the way it's done for *every* option in
> > > the entire key subsystem. So firstly I really don't think it's
> > > helpful for only encrypted keys to be different from everything
> > > else and secondly when I read the code (as I often do to figure out
> > > what the options mean), the additional .token and .pattern just get
> > > in the way of what I'm looking for.
> >
> > I just stumbled upon this and didn't check any other files.
>
> jejb@lingrow:~/git/linux> git grep 'match_table_t'|wc -l
> 49
>
> I'll leave it as an exercise to you to figure out how many use the
> style you're proposing.
>
> There's definite advantage in uniformity and even if I accepted the
> readability argument, which I don't, it's too small a reason to churn
> nearly 50 files one at a time.
>
> Regards,
>
> James
>
^ permalink raw reply
* Re: [PATCH v5 0/34] Rework the LSM initialization
From: Paul Moore @ 2025-10-22 23:34 UTC (permalink / raw)
To: linux-security-module, linux-integrity, selinux
Cc: John Johansen, Mimi Zohar, Roberto Sassu, Fan Wu,
Mickaël Salaün, Günther Noack, Kees Cook,
Micah Morton, Casey Schaufler, Tetsuo Handa, Nicolas Bouchinet,
Xiu Jianfeng
In-Reply-To: <20251017202456.484010-36-paul@paul-moore.com>
On Fri, Oct 17, 2025 at 4:28 PM Paul Moore <paul@paul-moore.com> wrote:
>
> This is the fifth, and likely final, revision of the LSM rework patchset.
> The number of changes in this revision are very minor and barring any
> surprises I expect to merge this into the lsm/dev branch next week; I'll
> send a notice when I do.
Here is that notice. This patchset is now merged into lsm/dev and
should be in the next linux-next release; if anyone notices anything
odd, please let me know.
As a FYI, I also moved the base of lsm/dev up to v6.18-rc2 to grab the
fix below (it was affecting testing).
https://lore.kernel.org/netdev/20251015052715.4140493-1-edumazet@google.com/
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH] hwrng: tpm: Do not enable by default
From: Jan Kiszka @ 2025-10-22 5:05 UTC (permalink / raw)
To: James Bottomley, Peter Huewe, Jarkko Sakkinen, linux-integrity
Cc: Linux Kernel Mailing List, Ilias Apalodimas, Jens Wiklander,
OP-TEE TrustedFirmware, linux-crypto
In-Reply-To: <212818188b192db3b852ec69fde174fd887eafac.camel@HansenPartnership.com>
On 21.10.25 18:15, James Bottomley wrote:
> On Tue, 2025-10-21 at 14:46 +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> As seen with optee_ftpm, which uses ms-tpm-20-ref [1], a TPM may
>> write the current time epoch to its NV storage every 4 seconds if
>> there are commands sent to it. The 60 seconds periodic update of the
>> entropy pool that the hwrng kthread does triggers this, causing about
>> 4 writes per requests. Makes 2 millions per year for a 24/7 device,
>> and that is a lot for its backing NV storage.
>
> The Reference implementation does this because it's NV ram is main
> memory and thus not subject to wear. A physical TPM can defer these
"NV" strongly suggests that a real implementation should permanently
store whatever is written to it, no?
> writes and condition them to the lifespan expectancy of its NV store.
> If you've simply copied over the reference implementation backed by
> wearable NV, then that might be the thing to fix.
>
My impression is that this is exactly what at least half of the fTPM
world does, starting with [1] and now via [2]. I started a discussion
with security experts about how often a write back is actually needed
but have no answer yet.
>> It is therefore better to make the user intentionally enable this,
>> providing a chance to read the warning.
>
> A standard TPM expects to be a secure RNG source, so is this merely
> speculation or have you found a physical TPM that has failed due to NV
> wear because of this?
I have not worn out any real TPM so far, only debugged the de-facto
standard fTPM in QEMU - and found this unexpected property.
At the same time, what should be different for a real TPM? It will not
have a battery-backed RTC either, thus will live from a clock source
which is reset after power-off. In order to avoid jumping back in its
own time, becoming vulnerable this way, I would expect a real TPM to
record the last seen time as well. Maybe it can do that smarter if it
can still write some bits after detecting power-loss, but that is also
speculation.
>
> Even if this were a problem, wouldn't a better solution be not to
> gather entropy if the kernel pool is full enough? We don't drain the
> pool the whole time after all.
>
That is a valid question, but at least I'm not deep enough into all of
this to answer it.
Jan
[1]
https://github.com/microsoft/ms-tpm-20-ref/commit/0ebdda848e16d5ef78d1342c2fdfdd6dffb1004e
[2] https://github.com/OP-TEE/optee_ftpm
--
Siemens AG, Foundational Technologies
Linux Expert Center
^ permalink raw reply
* Re: [PATCH] hwrng: tpm: Do not enable by default
From: James Bottomley @ 2025-10-21 16:15 UTC (permalink / raw)
To: Jan Kiszka, Peter Huewe, Jarkko Sakkinen, linux-integrity
Cc: Linux Kernel Mailing List, Ilias Apalodimas, Jens Wiklander,
OP-TEE TrustedFirmware, linux-crypto
In-Reply-To: <bbc41534-a2d9-42dc-ac8a-ff8a0b4fd41f@siemens.com>
On Tue, 2025-10-21 at 14:46 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> As seen with optee_ftpm, which uses ms-tpm-20-ref [1], a TPM may
> write the current time epoch to its NV storage every 4 seconds if
> there are commands sent to it. The 60 seconds periodic update of the
> entropy pool that the hwrng kthread does triggers this, causing about
> 4 writes per requests. Makes 2 millions per year for a 24/7 device,
> and that is a lot for its backing NV storage.
The Reference implementation does this because it's NV ram is main
memory and thus not subject to wear. A physical TPM can defer these
writes and condition them to the lifespan expectancy of its NV store.
If you've simply copied over the reference implementation backed by
wearable NV, then that might be the thing to fix.
> It is therefore better to make the user intentionally enable this,
> providing a chance to read the warning.
A standard TPM expects to be a secure RNG source, so is this merely
speculation or have you found a physical TPM that has failed due to NV
wear because of this?
Even if this were a problem, wouldn't a better solution be not to
gather entropy if the kernel pool is full enough? We don't drain the
pool the whole time after all.
Regards,
James
^ permalink raw reply
* Re: [PATCH 1/2] vfs: Allow filesystems with foreign owner IDs to override UID checks
From: David Howells @ 2025-10-21 13:20 UTC (permalink / raw)
To: Christian Brauner
Cc: dhowells, Al Viro, Christian Brauner, Marc Dionne, Jeffrey Altman,
Steve French, linux-afs, openafs-devel, linux-cifs, linux-nfs,
linux-fsdevel, linux-kernel, Etienne Champetier, Chet Ramey,
Cheyenne Wills, Mimi Zohar, linux-integrity
In-Reply-To: <20251021-agieren-spruch-65c107748c09@brauner>
Christian Brauner <brauner@kernel.org> wrote:
> > + if (unlikely(inode->i_op->have_same_owner)) {
>
> Same, as above: similar to IOP_FASTPERM this should use a flag to avoid pointer derefs.
Can we do these IOP_* flags better? Surely we can determine at the point the
inode has its ->i_op assigned that these things are provided? This optimises
the case where they don't exist at the expense of the case where they do (we
still have to check the pointer every time).
> > + if (unlikely(inode->i_op->have_same_owner)) {
>
> Same, as above: similar to IOP_FASTPERM this should use a flag to avoid pointer derefs.
>
> Really, we should very properly bias this towards the common case where
> the filesystem will not have a custom ownership comparison callback at all.
Hence the unlikely().
> > + struct dentry *parent;
> > + struct inode *dir;
> > + int ret;
> > +
> > + if (inode != nd->inode) {
> > + dir = nd->inode;
> > + ret = inode->i_op->have_same_owner(idmap, inode, dir);
> > + } else if (nd->flags & LOOKUP_RCU) {
> > + parent = READ_ONCE(nd->path.dentry);
> > + dir = READ_ONCE(parent->d_inode);
> > + if (!dir)
> > + return -ECHILD;
> > + ret = inode->i_op->have_same_owner(idmap, inode, dir);
> > + } else {
> > + parent = dget_parent(nd->path.dentry);
> > + dir = parent->d_inode;
> > + ret = inode->i_op->have_same_owner(idmap, inode, dir);
> > + dput(parent);
> > + }
> > + return ret;
> > + }
>
> This about as ugly as it can get and costly...
I can break this out into a helper, but it should make no difference to the
actual code generated.
> > + ret = vfs_inode_and_dir_have_same_owner(idmap, inode, nd);
> > + if (ret <= 0)
> > + return ret;
>
> Ok, so while that doesn't exactly surface the error it's still weird.
> Please make that consistent. Either have those two new helper functions
> return negative error codes and zero on success or have it be a proper
> boolean instead so there's no possible confusion. This is just begging
> for someone to do if (ret) return ret and bubble up that positive return
> value.
The problem is that you have three available returns: Yes they do, no they
don't and some arbitrary error was encountered. The first two are not error
cases, and potentially any error you pick to represent, say, "no" could also
be returned by the underlying filesystem.
David
^ permalink raw reply
* [PATCH] hwrng: tpm: Do not enable by default
From: Jan Kiszka @ 2025-10-21 12:46 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen, linux-integrity
Cc: Linux Kernel Mailing List, Ilias Apalodimas, Jens Wiklander,
OP-TEE TrustedFirmware, linux-crypto
From: Jan Kiszka <jan.kiszka@siemens.com>
As seen with optee_ftpm, which uses ms-tpm-20-ref [1], a TPM may write
the current time epoch to its NV storage every 4 seconds if there are
commands sent to it. The 60 seconds periodic update of the entropy pool
that the hwrng kthread does triggers this, causing about 4 writes per
requests. Makes 2 millions per year for a 24/7 device, and that is a lot
for its backing NV storage.
It is therefore better to make the user intentionally enable this,
providing a chance to read the warning.
[1] https://github.com/Microsoft/ms-tpm-20-ref
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
drivers/char/tpm/Kconfig | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 8a8f692b6088..d64c929cacbe 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -45,13 +45,17 @@ config TCG_TPM2_HMAC
config HW_RANDOM_TPM
bool "TPM HW Random Number Generator support"
depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m)
- default y
help
This setting exposes the TPM's Random Number Generator as a hwrng
device. This allows the kernel to collect randomness from the TPM at
boot, and provides the TPM randomines in /dev/hwrng.
- If unsure, say Y.
+ WARNING: Specifically firmware-based TPMs, possibly also hardware
+ variants, can wear-out from the frequent requests issued by the
+ Hardware Random Number Generator Core when filling the kernel's
+ entropy pool. These requests are sent once every minute by default,
+ and the TPM may write the current time to its NV storage for each of
+ them.
config TCG_TIS_CORE
tristate
--
2.51.0
^ permalink raw reply related
* Re: [PATCH 1/2] vfs: Allow filesystems with foreign owner IDs to override UID checks
From: Christian Brauner @ 2025-10-21 12:38 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Christian Brauner, Marc Dionne, Jeffrey Altman,
Steve French, linux-afs, openafs-devel, linux-cifs, linux-nfs,
linux-fsdevel, linux-kernel, Etienne Champetier, Chet Ramey,
Cheyenne Wills, Mimi Zohar, linux-integrity
In-Reply-To: <20251014133551.82642-2-dhowells@redhat.com>
On Tue, Oct 14, 2025 at 02:35:44PM +0100, David Howells wrote:
> A number of ownership checks made by the VFS make a number of assumptions:
>
> (1) that it is meaningful to compare inode->i_uid to a second ->i_uid or
> to current_fsuid(),
>
> (2) that current_fsuid() represents the subject of the action,
>
> (3) that the number in ->i_uid belong to the system's ID space and
>
> (4) that the IDs can be represented by 32-bit integers.
>
> Network filesystems, however, may violate all four of these assumptions.
> Indeed, a network filesystem may not even have an actual concept of a UNIX
> integer UID (cifs without POSIX extensions, for example). Plug-in block
> filesystems (e.g. USB drives) may also violate this assumption.
>
> In particular, AFS implements its own ACL security model with its own
> per-cell user ID space with 64-bit IDs for some server variants. The
> subject is represented by a token in a key, not current_fsuid(). The AFS
> user IDs and the system user IDs for a cell may be numerically equivalent,
> but that's matter of administrative policy and should perhaps be noted in
> the cell definition or by mount option. A subsequent patch will address
> AFS.
>
> To help fix this, three functions are defined to perform UID comparison
> within the VFS:
>
> (1) vfs_inode_is_owned_by_me(). This defaults to comparing i_uid to
> current_fsuid(), with appropriate namespace mapping, assuming that the
> fsuid identifies the subject of the action. The filesystem may
> override it by implementing an inode op:
>
> int (*is_owned_by_me)(struct mnt_idmap *idmap, struct inode *inode);
>
> This should return 0 if owned, 1 if not or an error if there's some
> sort of lookup failure. It may use a means of identifying the subject
> of the action other than fsuid, for example by using an authentication
> token stored in a key.
>
> (2) vfs_inodes_have_same_owner(). This defaults to comparing the i_uids
> of two different inodes with appropriate namespace mapping. The
> filesystem may override it by implementing another inode op:
>
> int (*have_same_owner)(struct mnt_idmap *idmap, struct inode *inode1,
> struct inode *inode2);
>
> Again, this should return 0 if matching, 1 if not or an error if
> there's some sort of lookup failure.
>
> (3) vfs_inode_and_dir_have_same_owner(). This is similar to (2), but
> assumes that the second inode is the parent directory to the first and
> takes a nameidata struct instead of a second inode pointer.
>
> Fix a number of places within the VFS where such UID checks are made that
> should be deferring interpretation to the filesystem.
>
> (*) chown_ok()
> (*) chgrp_ok()
>
> Call vfs_inode_is_owned_by_me(). Possibly these need to defer all
> their checks to the network filesystem as the interpretation of the
> new UID/GID depends on the netfs too, but the ->setattr() method gets
> a chance to deal with that.
>
> (*) coredump_file()
>
> Call vfs_is_owned_by_me() to check that the file created is owned by
> the caller - but the check that's there might be sufficient.
>
> (*) inode_owner_or_capable()
>
> Call vfs_is_owned_by_me(). I'm not sure whether the namespace mapping
> makes sense in such a case, but it probably could be used.
>
> (*) vfs_setlease()
>
> Call vfs_is_owned_by_me(). Actually, it should query if leasing is
> permitted.
>
> Also, setting locks could perhaps do with a permission call to the
> filesystem driver as AFS, for example, has a lock permission bit in
> the ACL, but since the AFS server checks that when the RPC call is
> made, it's probably unnecessary.
>
> (*) acl_permission_check()
> (*) posix_acl_permission()
>
> Unchanged. These functions are only used by generic_permission()
> which is overridden if ->permission() is supplied, and when evaluating
> a POSIX ACL, it should arguably be checking the UID anyway.
>
> AFS, for example, implements its own ACLs and evaluates them in
> ->permission() and on the server.
>
> (*) may_follow_link()
>
> Call vfs_inode_and_dir_have_same_owner() and vfs_is_owned_by_me() on
> the the link and its parent dir.
>
> (*) may_create_in_sticky()
>
> Call vfs_is_owned_by_me() and also vfs_inode_and_dir_have_same_owner()
> both.
>
> [?] Should this return ok immediately if the open call we're in
> created the file being checked.
>
> (*) __check_sticky()
>
> Call vfs_is_owned_by_me() on both the dir and the inode, but for AFS
> vfs_is_owned_by_me() on a directory doesn't work, so call
> vfs_inodes_have_same_owner() instead to check the directory (as is
> done in may_create_in_sticky()).
>
> (*) may_dedupe_file()
>
> Call vfs_is_owned_by_me().
>
> (*) IMA policy ops.
>
> Unchanged for now. I'm not sure what the best way to deal with this
> is - if, indeed, it needs any changes.
>
> Note that wrapping stuff up into vfs_inode_is_owned_by_me() isn't
> necessarily the most efficient as it means we may end up doing the uid
> idmapping an extra time - though this is only done in three places, all to
> do with world-writable sticky dir checks.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Etienne Champetier <champetier.etienne@gmail.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: Jeffrey Altman <jaltman@auristor.com>
> cc: Chet Ramey <chet.ramey@case.edu>
> cc: Cheyenne Wills <cwills@sinenomine.net>
> cc: Alexander Viro <viro@zeniv.linux.org.uk>
> cc: Christian Brauner <brauner@kernel.org>
> cc: Steve French <sfrench@samba.org>
> cc: Mimi Zohar <zohar@linux.ibm.com>
> cc: linux-afs@lists.infradead.org
> cc: openafs-devel@openafs.org
> cc: linux-cifs@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> cc: linux-integrity@vger.kernel.org
> Link: https://groups.google.com/g/gnu.bash.bug/c/6PPTfOgFdL4/m/2AQU-S1N76UJ
> Link: https://git.savannah.gnu.org/cgit/bash.git/tree/redir.c?h=bash-5.3-rc1#n733
> ---
> Documentation/filesystems/vfs.rst | 21 ++++
> fs/attr.c | 58 ++++++-----
> fs/coredump.c | 2 +-
> fs/inode.c | 11 +-
> fs/internal.h | 1 +
> fs/locks.c | 7 +-
> fs/namei.c | 161 ++++++++++++++++++++++++------
> fs/remap_range.c | 20 ++--
> include/linux/fs.h | 6 +-
> 9 files changed, 216 insertions(+), 71 deletions(-)
>
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index 4f13b01e42eb..5acbad3be4fd 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -495,6 +495,9 @@ As of kernel 2.6.22, the following members are defined:
> struct dentry *dentry, struct file_kattr *fa);
> int (*fileattr_get)(struct dentry *dentry, struct file_kattr *fa);
> struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> + int (*is_owned_by_me)(struct mnt_idmap *idmap, struct inode *inode);
> + int (*have_same_owner)(struct mnt_idmap *idmap, struct inode *inode,
> + struct dentry *dentry);
> };
>
> Again, all methods are called without any locks being held, unless
> @@ -679,6 +682,24 @@ otherwise noted.
> filesystem must define this operation to use
> simple_offset_dir_operations.
>
> +``is_owned_by_me``
> + called to determine if the file can be considered to be 'owned' by
> + the owner of the process or if the process has a token that grants
> + it ownership privileges. If unset, the default is to compare i_uid
> + to current_fsuid() - but this may give incorrect results for some
> + network or plug-in block filesystems. For example, AFS determines
> + ownership entirely according to an obtained token and i_uid may not
> + even be from the same ID space as current_uid().
> +
> +``have_same_owner``
> + called to determine if an inode has the same owner as its immediate
> + parent on the path walked. If unset, the default is to simply
> + compare the i_uid of both. For example, AFS compares the owner IDs
> + of both - but these are a 64-bit values on some variants that might
> + not fit into a kuid_t and cifs has GUIDs that cannot be compared to
> + kuid_t.
> +
> +
> The Address Space Object
> ========================
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 795f231d00e8..096401a4815d 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -16,6 +16,7 @@
> #include <linux/fcntl.h>
> #include <linux/filelock.h>
> #include <linux/security.h>
> +#include "internal.h"
>
> /**
> * setattr_should_drop_sgid - determine whether the setgid bit needs to be
> @@ -91,19 +92,21 @@ EXPORT_SYMBOL(setattr_should_drop_suidgid);
> * permissions. On non-idmapped mounts or if permission checking is to be
> * performed on the raw inode simply pass @nop_mnt_idmap.
> */
> -static bool chown_ok(struct mnt_idmap *idmap,
> - const struct inode *inode, vfsuid_t ia_vfsuid)
> +static int chown_ok(struct mnt_idmap *idmap,
> + struct inode *inode, vfsuid_t ia_vfsuid)
> {
> vfsuid_t vfsuid = i_uid_into_vfsuid(idmap, inode);
> - if (vfsuid_eq_kuid(vfsuid, current_fsuid()) &&
> - vfsuid_eq(ia_vfsuid, vfsuid))
> - return true;
> + int ret;
> +
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret <= 0)
> + return ret;
> if (capable_wrt_inode_uidgid(idmap, inode, CAP_CHOWN))
> - return true;
> + return 0;
> if (!vfsuid_valid(vfsuid) &&
> ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN))
> - return true;
> - return false;
> + return 0;
> + return -EPERM;
> }
>
> /**
> @@ -118,23 +121,27 @@ static bool chown_ok(struct mnt_idmap *idmap,
> * permissions. On non-idmapped mounts or if permission checking is to be
> * performed on the raw inode simply pass @nop_mnt_idmap.
> */
> -static bool chgrp_ok(struct mnt_idmap *idmap,
> - const struct inode *inode, vfsgid_t ia_vfsgid)
> +static int chgrp_ok(struct mnt_idmap *idmap,
> + struct inode *inode, vfsgid_t ia_vfsgid)
> {
> vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> - vfsuid_t vfsuid = i_uid_into_vfsuid(idmap, inode);
> - if (vfsuid_eq_kuid(vfsuid, current_fsuid())) {
> + int ret;
> +
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret < 0)
> + return ret;
> + if (ret == 0) {
> if (vfsgid_eq(ia_vfsgid, vfsgid))
> - return true;
> + return 0;
> if (vfsgid_in_group_p(ia_vfsgid))
> - return true;
> + return 0;
> }
> if (capable_wrt_inode_uidgid(idmap, inode, CAP_CHOWN))
> - return true;
> + return 0;
> if (!vfsgid_valid(vfsgid) &&
> ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN))
> - return true;
> - return false;
> + return 0;
> + return -EPERM;
> }
>
> /**
> @@ -163,6 +170,7 @@ int setattr_prepare(struct mnt_idmap *idmap, struct dentry *dentry,
> {
> struct inode *inode = d_inode(dentry);
> unsigned int ia_valid = attr->ia_valid;
> + int ret;
>
> /*
> * First check size constraints. These can't be overriden using
> @@ -179,14 +187,18 @@ int setattr_prepare(struct mnt_idmap *idmap, struct dentry *dentry,
> goto kill_priv;
>
> /* Make sure a caller can chown. */
> - if ((ia_valid & ATTR_UID) &&
> - !chown_ok(idmap, inode, attr->ia_vfsuid))
> - return -EPERM;
> + if (ia_valid & ATTR_UID) {
> + ret = chown_ok(idmap, inode, attr->ia_vfsuid);
> + if (ret < 0)
> + return ret;
> + }
>
> /* Make sure caller can chgrp. */
> - if ((ia_valid & ATTR_GID) &&
> - !chgrp_ok(idmap, inode, attr->ia_vfsgid))
> - return -EPERM;
> + if (ia_valid & ATTR_GID) {
> + ret = chgrp_ok(idmap, inode, attr->ia_vfsgid);
> + if (ret < 0)
> + return ret;
> + }
>
> /* Make sure a caller can chmod. */
> if (ia_valid & ATTR_MODE) {
> diff --git a/fs/coredump.c b/fs/coredump.c
> index b5fc06a092a4..ac113e41d090 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -951,7 +951,7 @@ static bool coredump_file(struct core_name *cn, struct coredump_params *cprm,
> * filesystem.
> */
> idmap = file_mnt_idmap(file);
> - if (!vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode), current_fsuid())) {
> + if (vfs_inode_is_owned_by_me(idmap, inode) != 0) {
> coredump_report_failure("Core dump to %s aborted: cannot preserve file owner", cn->corename);
> return false;
> }
> diff --git a/fs/inode.c b/fs/inode.c
> index ec9339024ac3..61e6b1d71e86 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2628,16 +2628,19 @@ EXPORT_SYMBOL(inode_init_owner);
> * On non-idmapped mounts or if permission checking is to be performed on the
> * raw inode simply pass @nop_mnt_idmap.
> */
> -bool inode_owner_or_capable(struct mnt_idmap *idmap,
> - const struct inode *inode)
> +bool inode_owner_or_capable(struct mnt_idmap *idmap, struct inode *inode)
> {
> vfsuid_t vfsuid;
> struct user_namespace *ns;
> + int ret;
>
> - vfsuid = i_uid_into_vfsuid(idmap, inode);
> - if (vfsuid_eq_kuid(vfsuid, current_fsuid()))
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret == 0)
> return true;
> + if (ret < 0)
> + return false;
>
> + vfsuid = i_uid_into_vfsuid(idmap, inode);
> ns = current_user_ns();
> if (vfsuid_has_mapping(ns, vfsuid) && ns_capable(ns, CAP_FOWNER))
> return true;
> diff --git a/fs/internal.h b/fs/internal.h
> index 9b2b4d116880..29682c6edecd 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -52,6 +52,7 @@ extern int finish_clean_context(struct fs_context *fc);
> /*
> * namei.c
> */
> +int vfs_inode_is_owned_by_me(struct mnt_idmap *idmap, struct inode *inode);
> extern int filename_lookup(int dfd, struct filename *name, unsigned flags,
> struct path *path, const struct path *root);
> int do_rmdir(int dfd, struct filename *name);
> diff --git a/fs/locks.c b/fs/locks.c
> index 04a3f0e20724..b710bf0733b0 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -68,6 +68,7 @@
> #include <trace/events/filelock.h>
>
> #include <linux/uaccess.h>
> +#include "internal.h"
>
> static struct file_lock *file_lock(struct file_lock_core *flc)
> {
> @@ -2013,10 +2014,12 @@ int
> vfs_setlease(struct file *filp, int arg, struct file_lease **lease, void **priv)
> {
> struct inode *inode = file_inode(filp);
> - vfsuid_t vfsuid = i_uid_into_vfsuid(file_mnt_idmap(filp), inode);
> int error;
>
> - if ((!vfsuid_eq_kuid(vfsuid, current_fsuid())) && !capable(CAP_LEASE))
> + error = vfs_inode_is_owned_by_me(file_mnt_idmap(filp), inode);
> + if (error < 0)
> + return error;
> + if (error != 0 && !capable(CAP_LEASE))
> return -EACCES;
> if (!S_ISREG(inode->i_mode))
> return -EINVAL;
> diff --git a/fs/namei.c b/fs/namei.c
> index 7377020a2cba..7dbcb5d50339 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
> * The new code replaces the old recursive symlink resolution with
> * an iterative one (in case of non-nested symlink chains). It does
> * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all
> * the special cases of the former code.
> *
> * With the new dcache, the pathname is stored at each inode, at least as
> @@ -1149,6 +1149,72 @@ fs_initcall(init_fs_namei_sysctls);
>
> #endif /* CONFIG_SYSCTL */
>
> +/*
> + * Determine if an inode is owned by the process (allowing for fsuid override),
> + * returning 0 if so, 1 if not and a negative error code if there was a problem
> + * making the determination.
> + */
> +int vfs_inode_is_owned_by_me(struct mnt_idmap *idmap, struct inode *inode)
> +{
> + if (unlikely(inode->i_op->is_owned_by_me))
> + return inode->i_op->is_owned_by_me(idmap, inode);
Similar to IOP_FASTPERM this should use a flag to avoid pointer derefs.
> + if (vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode), current_fsuid()))
> + return 0;
> + return 1; /* Not same. */
> +}
> +
> +/*
> + * Determine if an inode has the same owner as its parent, returning 0 if so, 1
> + * if not and a negative error code if there was a problem making the
> + * determination.
> + */
> +static int vfs_inode_and_dir_have_same_owner(struct mnt_idmap *idmap, struct inode *inode,
> + const struct nameidata *nd)
> +{
> + if (unlikely(inode->i_op->have_same_owner)) {
Same, as above: similar to IOP_FASTPERM this should use a flag to avoid pointer derefs.
Really, we should very properly bias this towards the common case where
the filesystem will not have a custom ownership comparison callback at all.
> + struct dentry *parent;
> + struct inode *dir;
> + int ret;
> +
> + if (inode != nd->inode) {
> + dir = nd->inode;
> + ret = inode->i_op->have_same_owner(idmap, inode, dir);
> + } else if (nd->flags & LOOKUP_RCU) {
> + parent = READ_ONCE(nd->path.dentry);
> + dir = READ_ONCE(parent->d_inode);
> + if (!dir)
> + return -ECHILD;
> + ret = inode->i_op->have_same_owner(idmap, inode, dir);
> + } else {
> + parent = dget_parent(nd->path.dentry);
> + dir = parent->d_inode;
> + ret = inode->i_op->have_same_owner(idmap, inode, dir);
> + dput(parent);
> + }
> + return ret;
> + }
This about as ugly as it can get and costly...
> +
> + if (vfsuid_valid(nd->dir_vfsuid) &&
> + vfsuid_eq(i_uid_into_vfsuid(idmap, inode), nd->dir_vfsuid))
> + return 0;
> + return 1; /* Not same. */
Why is this returning 1? This will cause may_follow_link() to return 1
and that will bubble up as an error code? So shouldn't this return a
negative error code instead?
> +}
> +
> +/*
> + * Determine if two inodes have the same owner, returning 0 if so, 1 if not and
> + * a negative error code if there was a problem making the determination.
> + */
> +static int vfs_inodes_have_same_owner(struct mnt_idmap *idmap, struct inode *inode,
> + struct inode *dir)
> +{
> + if (unlikely(inode->i_op->have_same_owner))
> + return inode->i_op->have_same_owner(idmap, inode, dir);
> + if (vfsuid_eq(i_uid_into_vfsuid(idmap, inode),
> + i_uid_into_vfsuid(idmap, dir)))
> + return 0;
> + return 1; /* Not same. */
> +}
> +
> /**
> * may_follow_link - Check symlink following for unsafe situations
> * @nd: nameidata pathwalk data
> @@ -1165,27 +1231,28 @@ fs_initcall(init_fs_namei_sysctls);
> *
> * Returns 0 if following the symlink is allowed, -ve on error.
> */
> -static inline int may_follow_link(struct nameidata *nd, const struct inode *inode)
> +static inline int may_follow_link(struct nameidata *nd, struct inode *inode)
> {
> struct mnt_idmap *idmap;
> - vfsuid_t vfsuid;
> + int ret;
>
> if (!sysctl_protected_symlinks)
> return 0;
>
> - idmap = mnt_idmap(nd->path.mnt);
> - vfsuid = i_uid_into_vfsuid(idmap, inode);
> - /* Allowed if owner and follower match. */
> - if (vfsuid_eq_kuid(vfsuid, current_fsuid()))
> - return 0;
> -
> /* Allowed if parent directory not sticky and world-writable. */
> if ((nd->dir_mode & (S_ISVTX|S_IWOTH)) != (S_ISVTX|S_IWOTH))
> return 0;
>
> + idmap = mnt_idmap(nd->path.mnt);
> + /* Allowed if owner and follower match. */
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret <= 0)
> + return ret;
> +
> /* Allowed if parent directory and link owner match. */
> - if (vfsuid_valid(nd->dir_vfsuid) && vfsuid_eq(nd->dir_vfsuid, vfsuid))
> - return 0;
> + ret = vfs_inode_and_dir_have_same_owner(idmap, inode, nd);
> + if (ret <= 0)
> + return ret;
Ok, so while that doesn't exactly surface the error it's still weird.
Please make that consistent. Either have those two new helper functions
return negative error codes and zero on success or have it be a proper
boolean instead so there's no possible confusion. This is just begging
for someone to do if (ret) return ret and bubble up that positive return
value.
>
> if (nd->flags & LOOKUP_RCU)
> return -ECHILD;
> @@ -1283,12 +1350,12 @@ int may_linkat(struct mnt_idmap *idmap, const struct path *link)
> * @inode: the inode of the file to open
> *
> * Block an O_CREAT open of a FIFO (or a regular file) when:
> - * - sysctl_protected_fifos (or sysctl_protected_regular) is enabled
> - * - the file already exists
> - * - we are in a sticky directory
> - * - we don't own the file
> + * - sysctl_protected_fifos (or sysctl_protected_regular) is enabled,
> + * - the file already exists,
> + * - we are in a sticky directory,
> + * - the directory is world writable,
> + * - we don't own the file and
> * - the owner of the directory doesn't own the file
> - * - the directory is world writable
> * If the sysctl_protected_fifos (or sysctl_protected_regular) is set to 2
> * the directory doesn't have to be world writable: being group writable will
> * be enough.
> @@ -1299,13 +1366,45 @@ int may_linkat(struct mnt_idmap *idmap, const struct path *link)
> * On non-idmapped mounts or if permission checking is to be performed on the
> * raw inode simply pass @nop_mnt_idmap.
> *
> + * For a filesystem (e.g. a network filesystem) that has a separate ID space
> + * and has foreign IDs (maybe even non-integer IDs), i_uid cannot be compared
> + * to current_fsuid() and may not be directly comparable to another i_uid.
> + * Instead, the filesystem is asked to perform the comparisons. With network
> + * filesystems, there also exists the possibility of doing anonymous
> + * operations and having anonymously-owned objects.
> + *
> + * We have the following scenarios:
> + *
> + * USER DIR FILE FILE ALLOWED
> + * OWNER OWNER STATE
> + * ======= ======= ======= ======= =======
> + * A A - New Yes
> + * A A A Exists Yes
> + * A A C Exists No
> + * A B - New Yes
> + * A B A Exists Yes, FO==U
> + * A B B Exists Yes, FO==DO
> + * A B C Exists No
> + * A anon[1] - New Yes
> + * A anon[1] A Exists Yes
> + * A anon[1] C Exists No
> + * anon A - New Yes
> + * anon A A Exists Yes, FO==DO
> + * anon anon[1] - New Yes
> + * anon anon[1] - Exists No
> + * anon A A Exists Yes, FO==DO
> + * anon A C Exists No
> + * anon A anon Exists No
> + *
> + * [1] Can anonymously-owned dirs be sticky?
> + *
> * Returns 0 if the open is allowed, -ve on error.
> */
> static int may_create_in_sticky(struct mnt_idmap *idmap, struct nameidata *nd,
> - struct inode *const inode)
> + struct inode *inode)
> {
> umode_t dir_mode = nd->dir_mode;
> - vfsuid_t dir_vfsuid = nd->dir_vfsuid, i_vfsuid;
> + int ret;
>
> if (likely(!(dir_mode & S_ISVTX)))
> return 0;
> @@ -1316,13 +1415,13 @@ static int may_create_in_sticky(struct mnt_idmap *idmap, struct nameidata *nd,
> if (S_ISFIFO(inode->i_mode) && !sysctl_protected_fifos)
> return 0;
>
> - i_vfsuid = i_uid_into_vfsuid(idmap, inode);
> -
> - if (vfsuid_eq(i_vfsuid, dir_vfsuid))
> - return 0;
> + ret = vfs_inode_and_dir_have_same_owner(idmap, inode, nd);
> + if (ret <= 0)
> + return ret;
>
> - if (vfsuid_eq_kuid(i_vfsuid, current_fsuid()))
> - return 0;
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret <= 0)
> + return ret;
>
> if (likely(dir_mode & 0002)) {
> audit_log_path_denied(AUDIT_ANOM_CREAT, "sticky_create");
> @@ -3222,12 +3321,14 @@ EXPORT_SYMBOL(user_path_at);
> int __check_sticky(struct mnt_idmap *idmap, struct inode *dir,
> struct inode *inode)
> {
> - kuid_t fsuid = current_fsuid();
> + int ret;
>
> - if (vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode), fsuid))
> - return 0;
> - if (vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, dir), fsuid))
> - return 0;
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret <= 0)
> + return ret;
> + ret = vfs_inodes_have_same_owner(idmap, inode, dir);
> + if (ret <= 0)
> + return ret;
> return !capable_wrt_inode_uidgid(idmap, inode, CAP_FOWNER);
> }
> EXPORT_SYMBOL(__check_sticky);
> diff --git a/fs/remap_range.c b/fs/remap_range.c
> index 26afbbbfb10c..9eee93c27001 100644
> --- a/fs/remap_range.c
> +++ b/fs/remap_range.c
> @@ -413,20 +413,22 @@ loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
> EXPORT_SYMBOL(vfs_clone_file_range);
>
> /* Check whether we are allowed to dedupe the destination file */
> -static bool may_dedupe_file(struct file *file)
> +static int may_dedupe_file(struct file *file)
> {
> struct mnt_idmap *idmap = file_mnt_idmap(file);
> struct inode *inode = file_inode(file);
> + int ret;
>
> if (capable(CAP_SYS_ADMIN))
> - return true;
> + return 0;
> if (file->f_mode & FMODE_WRITE)
> - return true;
> - if (vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode), current_fsuid()))
> - return true;
> + return 0;
> + ret = vfs_inode_is_owned_by_me(idmap, inode);
> + if (ret <= 0)
> + return ret;
> if (!inode_permission(idmap, inode, MAY_WRITE))
> - return true;
> - return false;
> + return 0;
> + return -EPERM;
> }
>
> loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
> @@ -459,8 +461,8 @@ loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
> if (ret)
> return ret;
>
> - ret = -EPERM;
> - if (!may_dedupe_file(dst_file))
> + ret = may_dedupe_file(dst_file);
> + if (ret < 0)
> goto out_drop_write;
>
> ret = -EXDEV;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c895146c1444..f59a7456852f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2104,8 +2104,7 @@ static inline bool sb_start_intwrite_trylock(struct super_block *sb)
> return __sb_start_write_trylock(sb, SB_FREEZE_FS);
> }
>
> -bool inode_owner_or_capable(struct mnt_idmap *idmap,
> - const struct inode *inode);
> +bool inode_owner_or_capable(struct mnt_idmap *idmap, struct inode *inode);
>
> /*
> * VFS helper functions..
> @@ -2376,6 +2375,9 @@ struct inode_operations {
> struct dentry *dentry, struct file_kattr *fa);
> int (*fileattr_get)(struct dentry *dentry, struct file_kattr *fa);
> struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> + int (*is_owned_by_me)(struct mnt_idmap *idmap, struct inode *inode);
> + int (*have_same_owner)(struct mnt_idmap *idmap, struct inode *inode1,
> + struct inode *inode2);
> } ____cacheline_aligned;
>
> /* Did the driver provide valid mmap hook configuration? */
>
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-20 13:57 UTC (permalink / raw)
To: Roberto Sassu, Coiby Xu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <1987088bb29971883d2b5c06a31c8114c729422c.camel@huaweicloud.com>
On Mon, 2025-10-20 at 14:45 +0200, Roberto Sassu wrote:
> On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
> > On Sat, 2025-10-18 at 07:19 +0800, Coiby Xu wrote:
> > > > > > 2. Instead of defining an additional process_measurement() argument to identify
> > > > > > compressed kernel modules, to simplify the code it might be possible to define a
> > > > > > new "func" named COMPRESSED_MODULE_CHECK.
> > > > > >
> > > > > > + [READING_COMPRESSED_MODULE] = MODULE_CHECK, -> COMPRESSED_MODULE_CHECK
> > > > >
> > > > > I also thought about this approach. But IMA rule maps kernel module
> > > > > loading to MODULE_CHECK. If we define a new rule and ask users to use
> > > > > this new rule, ima_policy=secure_boot still won't work.
> > > >
> > > > I don't have a problem with extending the "secure-boot" policy to support
> > > > uncompressed kernel modules appended signatures, based on whether
> > > > CONFIG_MODULE_SIG is enabled. The new rule would be in addition to the existing
> > > > MODULE_CHECK rule.
> > >
> > > I assume once the new rule get added, we can't remove it for userspace
> > > backward compatibility, right? And with CPIO xattr supported, it seems
> > > there is no need to keep this rule. So if this concern is valid, do you
> > > think we shall switch to another approach i.e. to make IMA support
> > > verifying decompressed module and then make "secure-boot" to allow
> > > appended module signature?
> >
> > Yes, once the rule is added, it wouldn't be removed. As for "to make IMA
> > support verifying decompressed module", yes that might be a better solution,
> > than relying on "sig_enforce" being enabled. IMA already supports verifying the
> > appended signatures. A new IMA specific or LSM hook would need to be defined
> > after module_decompress().
> >
> > Remember based on policy, IMA supports:
> > 1. verifying the signature stored in security.ima xattr
> > 2. verifying the appended signature (not for compressed kernel modules)
> > 3. verifying both the xattr and appended signatures
> > 4. none
> >
> > To prevent 3 - verifying both types of signatures, the IMA arch specific policy
> > rule only adds the "appraise func=MODULE_CHECK ..." rule if CONFIG_MODULE_SIG is
> > NOT enabled. Calling set_module_sig_enforced() from ima_appraise_measurement()
> > to set sig_enforce could inadvertently result in requiring both the xattr and
> > the appended signature kernel module verification. To prevent this from
> > happening, "sig_enforce" should not be set, only verified in
> > ima_appraise_measurement().
> >
> > >
> > > Another thought is to make CPIO support xattr. Today I realize that
> > > ima_policy=secure_boot can also cause failure of loading kdump kernel.
> > > So the issue this patch tries to resolves has much less impact than I
> > > thought. Maybe we can wait until CPIO xattr support is ready? I'll help
> > > review and test Roberto's patches if this is the best way forward.
> >
> > I'm not sure of the status of the CPIO patch set. Roberto?
>
> I haven't had time to look at it recently. I can take the openEuler
> version, address the remaining comments and repost.
Thank you!
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Roberto Sassu @ 2025-10-20 12:45 UTC (permalink / raw)
To: Mimi Zohar, Coiby Xu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <559f6ebf4a19da321fffc2a3ca180dc3d6216a22.camel@linux.ibm.com>
On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
> On Sat, 2025-10-18 at 07:19 +0800, Coiby Xu wrote:
> > > > > 2. Instead of defining an additional process_measurement() argument to identify
> > > > > compressed kernel modules, to simplify the code it might be possible to define a
> > > > > new "func" named COMPRESSED_MODULE_CHECK.
> > > > >
> > > > > + [READING_COMPRESSED_MODULE] = MODULE_CHECK, -> COMPRESSED_MODULE_CHECK
> > > >
> > > > I also thought about this approach. But IMA rule maps kernel module
> > > > loading to MODULE_CHECK. If we define a new rule and ask users to use
> > > > this new rule, ima_policy=secure_boot still won't work.
> > >
> > > I don't have a problem with extending the "secure-boot" policy to support
> > > uncompressed kernel modules appended signatures, based on whether
> > > CONFIG_MODULE_SIG is enabled. The new rule would be in addition to the existing
> > > MODULE_CHECK rule.
> >
> > I assume once the new rule get added, we can't remove it for userspace
> > backward compatibility, right? And with CPIO xattr supported, it seems
> > there is no need to keep this rule. So if this concern is valid, do you
> > think we shall switch to another approach i.e. to make IMA support
> > verifying decompressed module and then make "secure-boot" to allow
> > appended module signature?
>
> Yes, once the rule is added, it wouldn't be removed. As for "to make IMA
> support verifying decompressed module", yes that might be a better solution,
> than relying on "sig_enforce" being enabled. IMA already supports verifying the
> appended signatures. A new IMA specific or LSM hook would need to be defined
> after module_decompress().
>
> Remember based on policy, IMA supports:
> 1. verifying the signature stored in security.ima xattr
> 2. verifying the appended signature (not for compressed kernel modules)
> 3. verifying both the xattr and appended signatures
> 4. none
>
> To prevent 3 - verifying both types of signatures, the IMA arch specific policy
> rule only adds the "appraise func=MODULE_CHECK ..." rule if CONFIG_MODULE_SIG is
> NOT enabled. Calling set_module_sig_enforced() from ima_appraise_measurement()
> to set sig_enforce could inadvertently result in requiring both the xattr and
> the appended signature kernel module verification. To prevent this from
> happening, "sig_enforce" should not be set, only verified in
> ima_appraise_measurement().
>
> >
> > Another thought is to make CPIO support xattr. Today I realize that
> > ima_policy=secure_boot can also cause failure of loading kdump kernel.
> > So the issue this patch tries to resolves has much less impact than I
> > thought. Maybe we can wait until CPIO xattr support is ready? I'll help
> > review and test Roberto's patches if this is the best way forward.
>
> I'm not sure of the status of the CPIO patch set. Roberto?
I haven't had time to look at it recently. I can take the openEuler
version, address the remaining comments and repost.
Roberto
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-20 12:21 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, Dmitry Torokhov, Karel Srot, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <z6f4getlayaxaxvlxfxn2yvn5dvhrct64wke4uu2s3dfll3bqq@754bklrku55n>
On Sat, 2025-10-18 at 07:19 +0800, Coiby Xu wrote:
> > > > 2. Instead of defining an additional process_measurement() argument to identify
> > > > compressed kernel modules, to simplify the code it might be possible to define a
> > > > new "func" named COMPRESSED_MODULE_CHECK.
> > > >
> > > > + [READING_COMPRESSED_MODULE] = MODULE_CHECK, -> COMPRESSED_MODULE_CHECK
> > >
> > > I also thought about this approach. But IMA rule maps kernel module
> > > loading to MODULE_CHECK. If we define a new rule and ask users to use
> > > this new rule, ima_policy=secure_boot still won't work.
> >
> > I don't have a problem with extending the "secure-boot" policy to support
> > uncompressed kernel modules appended signatures, based on whether
> > CONFIG_MODULE_SIG is enabled. The new rule would be in addition to the existing
> > MODULE_CHECK rule.
>
> I assume once the new rule get added, we can't remove it for userspace
> backward compatibility, right? And with CPIO xattr supported, it seems
> there is no need to keep this rule. So if this concern is valid, do you
> think we shall switch to another approach i.e. to make IMA support
> verifying decompressed module and then make "secure-boot" to allow
> appended module signature?
Yes, once the rule is added, it wouldn't be removed. As for "to make IMA
support verifying decompressed module", yes that might be a better solution,
than relying on "sig_enforce" being enabled. IMA already supports verifying the
appended signatures. A new IMA specific or LSM hook would need to be defined
after module_decompress().
Remember based on policy, IMA supports:
1. verifying the signature stored in security.ima xattr
2. verifying the appended signature (not for compressed kernel modules)
3. verifying both the xattr and appended signatures
4. none
To prevent 3 - verifying both types of signatures, the IMA arch specific policy
rule only adds the "appraise func=MODULE_CHECK ..." rule if CONFIG_MODULE_SIG is
NOT enabled. Calling set_module_sig_enforced() from ima_appraise_measurement()
to set sig_enforce could inadvertently result in requiring both the xattr and
the appended signature kernel module verification. To prevent this from
happening, "sig_enforce" should not be set, only verified in
ima_appraise_measurement().
>
> Another thought is to make CPIO support xattr. Today I realize that
> ima_policy=secure_boot can also cause failure of loading kdump kernel.
> So the issue this patch tries to resolves has much less impact than I
> thought. Maybe we can wait until CPIO xattr support is ready? I'll help
> review and test Roberto's patches if this is the best way forward.
I'm not sure of the status of the CPIO patch set. Roberto?
Mimi
^ permalink raw reply
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: Roberto Sassu @ 2025-10-20 11:53 UTC (permalink / raw)
To: Jonathan McDowell, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe
Cc: Linus Torvalds, James Bottomley, linux-integrity, linux-kernel,
zohar
In-Reply-To: <61049f236fe1eaf72402895cea6892b52ce7e279.1760958898.git.noodles@meta.com>
On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
>
> There are situations where userspace might reasonably desire exclusive
> access to the TPM, or the kernel's internal context saving + flushing
> may cause issues, for example when performing firmware upgrades. Extend
> the locking already used for avoiding concurrent userspace access to
> prevent internal users of the TPM when /dev/tpm<n> is in use.
>
> The few internal users who already hold the open_lock are changed to use
> tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
> functions changing to obtain read access to the open_lock. We return
> -EBUSY when another user has exclusive access, rather than adding waits.
>
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
> v2: Switch to _locked instead of _internal_ for function names.
> v3: Move to end of patch series.
>
> drivers/char/tpm/tpm-chip.c | 53 +++++++++++++++++++++++++------
> drivers/char/tpm/tpm-dev-common.c | 8 ++---
> drivers/char/tpm/tpm.h | 2 ++
> drivers/char/tpm/tpm2-space.c | 5 ++-
> 4 files changed, 52 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ba906966721a..687f6d8cd601 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
> EXPORT_SYMBOL_GPL(tpm_chip_stop);
>
> /**
> - * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
> * @chip: Chip to ref
> *
> * The caller must already have some kind of locking to ensure that chip is
> @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
> *
> * Returns -ERRNO if the chip could not be got.
> */
> -int tpm_try_get_ops(struct tpm_chip *chip)
> +int tpm_try_get_ops_locked(struct tpm_chip *chip)
> {
> int rc = -EIO;
>
> @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
> put_device(&chip->dev);
> return rc;
> }
> -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>
> /**
> - * tpm_put_ops() - Release a ref to the tpm_chip
> + * tpm_put_ops_locked() - Release a ref to the tpm_chip
> * @chip: Chip to put
> *
> - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
> - * be kfree'd.
> + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
> + * chip may be kfree'd.
> */
> -void tpm_put_ops(struct tpm_chip *chip)
> +void tpm_put_ops_locked(struct tpm_chip *chip)
> {
> tpm_chip_stop(chip);
> mutex_unlock(&chip->tpm_mutex);
> up_read(&chip->ops_sem);
> put_device(&chip->dev);
> }
> +
> +/**
> + * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * @chip: Chip to ref
> + *
> + * The caller must already have some kind of locking to ensure that chip is
> + * valid. This function will attempt to get the open_lock for the chip,
> + * ensuring no other user is expecting exclusive access, before locking the
> + * chip so that the ops member can be accessed safely. The locking prevents
> + * tpm_chip_unregister from completing, so it should not be held for long
> + * periods.
> + *
> + * Returns -ERRNO if the chip could not be got.
> + */
> +int tpm_try_get_ops(struct tpm_chip *chip)
> +{
> + if (!down_read_trylock(&chip->open_lock))
> + return -EBUSY;
Hi Jonathan
do I understand it correctly, that a process might open the TPM with
O_EXCL, and this will prevent IMA from extending a PCR until that
process closes the file descriptor?
If yes, this might be a concern, and I think an additional API to
prevent such behavior would be needed (for example when IMA is active,
i.e. there is a measurement policy loaded).
Thanks
Roberto
> +
> + return tpm_try_get_ops_locked(chip);
> +}
> +EXPORT_SYMBOL_GPL(tpm_try_get_ops);
> +
> +/**
> + * tpm_put_ops() - Release a ref to the tpm_chip
> + * @chip: Chip to put
> + *
> + * This is the opposite pair to tpm_try_get_ops(). After this returns
> + * chip may be kfree'd.
> + */
> +void tpm_put_ops(struct tpm_chip *chip)
> +{
> + tpm_put_ops_locked(chip);
> +
> + up_read(&chip->open_lock);
> +}
> EXPORT_SYMBOL_GPL(tpm_put_ops);
>
> /**
> @@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
> #ifdef CONFIG_TCG_TPM2_HMAC
> int rc;
>
> - rc = tpm_try_get_ops(chip);
> + rc = tpm_try_get_ops_locked(chip);
> if (!rc) {
> tpm2_end_auth_session(chip);
> - tpm_put_ops(chip);
> + tpm_put_ops_locked(chip);
> }
> #endif
>
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index f2a5e09257dd..0f5bc63411aa 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>
> mutex_lock(&priv->buffer_mutex);
> priv->command_enqueued = false;
> - ret = tpm_try_get_ops(priv->chip);
> + ret = tpm_try_get_ops_locked(priv->chip);
> if (ret) {
> priv->response_length = ret;
> goto out;
> @@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>
> ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
> sizeof(priv->data_buffer));
> - tpm_put_ops(priv->chip);
> + tpm_put_ops_locked(priv->chip);
>
> /*
> * If ret is > 0 then tpm_dev_transmit returned the size of the
> @@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
> * lock during this period so that the tpm can be unregistered even if
> * the char dev is held open.
> */
> - if (tpm_try_get_ops(priv->chip)) {
> + if (tpm_try_get_ops_locked(priv->chip)) {
> ret = -EPIPE;
> goto out;
> }
>
> ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
> sizeof(priv->data_buffer));
> - tpm_put_ops(priv->chip);
> + tpm_put_ops_locked(priv->chip);
>
> if (ret > 0) {
> priv->response_length = ret;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 02c07fef41ba..57ef8589f5f5 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
> const struct tpm_class_ops *ops);
> struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
> const struct tpm_class_ops *ops);
> +int tpm_try_get_ops_locked(struct tpm_chip *chip);
> +void tpm_put_ops_locked(struct tpm_chip *chip);
> int tpm_chip_register(struct tpm_chip *chip);
> void tpm_chip_unregister(struct tpm_chip *chip);
>
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 60354cd53b5c..0ad5e18355e0 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>
> void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
> {
> -
> - if (tpm_try_get_ops(chip) == 0) {
> + if (tpm_try_get_ops_locked(chip) == 0) {
> tpm2_flush_sessions(chip, space);
> - tpm_put_ops(chip);
> + tpm_put_ops_locked(chip);
> }
>
> kfree(space->context_buf);
^ permalink raw reply
* Re: [PATCH v6 01/10] tpm: Cap the number of PCR banks
From: Jonathan McDowell @ 2025-10-20 11:45 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, keyring, dpsmith, ross.philipson, Roberto Sassu,
Stefano Garzarella, Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe,
linux-kernel
In-Reply-To: <20251018111725.3116386-2-jarkko@kernel.org>
On Sat, Oct 18, 2025 at 02:17:16PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>tpm2_get_pcr_allocation() does not cap any upper limit for the number of
>banks. Cap the limit to four banks so that out of bounds values coming
>from external I/O cause on only limited harm.
Comment no longer matches code - we cap at 8. With the comment fixed:
Reviewed-By: Jonathan McDowell <noodles@meta.com>
>Cc: Roberto Sassu <roberto.sassu@huawei.com>
>Fixes: bcfff8384f6c ("tpm: dynamically allocate the allocated_banks array")
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
>v6
>- No changes.
>v5:
>- No changes.
>v4:
>- Revert spurious changes from include/linux/tpm.h.
>- Increase TPM2_MAX_BANKS to 8.
>- Rename TPM2_MAX_BANKS as TPM2_MAX_PCR_BANKS for the sake of clarity.
>v3:
>- Wrote a more clear commit message.
>- Fixed pr_err() message.
>v2:
>- A new patch.
>---
> drivers/char/tpm/tpm-chip.c | 13 +++++++++----
> drivers/char/tpm/tpm.h | 1 -
> drivers/char/tpm/tpm1-cmd.c | 25 -------------------------
> drivers/char/tpm/tpm2-cmd.c | 8 +++-----
> include/linux/tpm.h | 8 +++++---
> 5 files changed, 17 insertions(+), 38 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>index e25daf2396d3..6cb25862688f 100644
>--- a/drivers/char/tpm/tpm-chip.c
>+++ b/drivers/char/tpm/tpm-chip.c
>@@ -559,14 +559,19 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
>
> static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> {
>- int rc;
>+ int rc = 0;
>
> if (tpm_is_firmware_upgrade(chip))
> return 0;
>
>- rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
>- tpm2_get_pcr_allocation(chip) :
>- tpm1_get_pcr_allocation(chip);
>+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
>+ chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
>+ chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
>+ chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
>+ chip->nr_allocated_banks = 1;
>+ } else {
>+ rc = tpm2_get_pcr_allocation(chip);
>+ }
>
> if (rc > 0)
> return -ENODEV;
>diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>index 2726bd38e5ac..a37712c02e44 100644
>--- a/drivers/char/tpm/tpm.h
>+++ b/drivers/char/tpm/tpm.h
>@@ -252,7 +252,6 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
> ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
> const char *desc, size_t min_cap_length);
> int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
>-int tpm1_get_pcr_allocation(struct tpm_chip *chip);
> unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> int tpm_pm_suspend(struct device *dev);
> int tpm_pm_resume(struct device *dev);
>diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>index cf64c7385105..5c49bdff33de 100644
>--- a/drivers/char/tpm/tpm1-cmd.c
>+++ b/drivers/char/tpm/tpm1-cmd.c
>@@ -786,28 +786,3 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>
> return rc;
> }
>-
>-/**
>- * tpm1_get_pcr_allocation() - initialize the allocated bank
>- * @chip: TPM chip to use.
>- *
>- * The function initializes the SHA1 allocated bank to extend PCR
>- *
>- * Return:
>- * * 0 on success,
>- * * < 0 on error.
>- */
>-int tpm1_get_pcr_allocation(struct tpm_chip *chip)
>-{
>- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
>- GFP_KERNEL);
>- if (!chip->allocated_banks)
>- return -ENOMEM;
>-
>- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
>- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
>- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
>- chip->nr_allocated_banks = 1;
>-
>- return 0;
>-}
>diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>index 7d77f6fbc152..97501c567c34 100644
>--- a/drivers/char/tpm/tpm2-cmd.c
>+++ b/drivers/char/tpm/tpm2-cmd.c
>@@ -538,11 +538,9 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>
> nr_possible_banks = be32_to_cpup(
> (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>-
>- chip->allocated_banks = kcalloc(nr_possible_banks,
>- sizeof(*chip->allocated_banks),
>- GFP_KERNEL);
>- if (!chip->allocated_banks) {
>+ if (nr_possible_banks > TPM2_MAX_PCR_BANKS) {
>+ pr_err("tpm: unexpected number of banks: %u > %u",
>+ nr_possible_banks, TPM2_MAX_PCR_BANKS);
> rc = -ENOMEM;
> goto out;
> }
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index dc0338a783f3..eb0ff071bcae 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -26,7 +26,9 @@
> #include <crypto/aes.h>
>
> #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
>-#define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
>+
>+#define TPM2_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
>+#define TPM2_MAX_PCR_BANKS 8
>
> struct tpm_chip;
> struct trusted_key_payload;
>@@ -68,7 +70,7 @@ enum tpm2_curves {
>
> struct tpm_digest {
> u16 alg_id;
>- u8 digest[TPM_MAX_DIGEST_SIZE];
>+ u8 digest[TPM2_MAX_DIGEST_SIZE];
> } __packed;
>
> struct tpm_bank_info {
>@@ -189,7 +191,7 @@ struct tpm_chip {
> unsigned int groups_cnt;
>
> u32 nr_allocated_banks;
>- struct tpm_bank_info *allocated_banks;
>+ struct tpm_bank_info allocated_banks[TPM2_MAX_PCR_BANKS];
> #ifdef CONFIG_ACPI
> acpi_handle acpi_dev_handle;
> char ppi_version[TPM_PPI_VERSION_LEN + 1];
>--
>2.39.5
J.
--
Is it real, or is it Mimozine?
This .sig brought to you by the letter N and the number 9
Product of the Republic of HuggieTag
^ permalink raw reply
* [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>
From: Jonathan McDowell @ 2025-10-20 11:30 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe
Cc: Linus Torvalds, James Bottomley, linux-integrity, linux-kernel
In-Reply-To: <cover.1758646791.git.noodles@meta.com>
I hit a problem where ~ 1% of TPM firmware upgrades were failing.
Investigating revealed the issue was that although the upgrade tool uses
/dev/tpm0 this does not actually prevent access via /dev/tpmrm0, nor
internal kernel users. It *does* prevent access to others via /dev/tpm0
So the upgrade process started, the HW RNG came in to get some
randomness in the middle, did the HMAC context dance, and confused
everything to the point the TPM was no longer visible to the OS even
after a reboot.
Thankfully I've been able to recover those devices, but really what I'd
like is the ability for a userspace tool to exclusively access the TPM
without something coming in behind it. Given the lightweight attempt at
locking that already exists I think this was the original intention.
I've reworked this series based on feedback received.
Firstly, it's been reordered TPM sharing functionality doesn't break
during bisection.
Secondly, the O_EXCL check has been tightend up to ensure the caller is
also opening the device O_RDWR. Callers shouldn't really be opening the
TPM except for reading + writing, but this should help guard against
unexpected flags usage a bit more.
Finally, this revision keeps the prohibition on more than one user of
/dev/tpm#, to avoid unexpected breakages for clients that expect this to
guard against multiple invocations. A client only then needs to use
O_EXCL if it wants to prevent *all* other access, even with
ContextSaves, such as the firmware upgrade case.
(I've sent a separate standalone patch that allows the TPM HW RNG to be
disabled at run time, and it's now in -next, but even with that I think
something like this is a good idea as well.)
Jonathan McDowell (4):
tpm: Remove tpm_find_get_ops
tpm: Add O_EXCL for exclusive /dev/tpm access
tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM
access
tpm: Allow for exclusive TPM access when using /dev/tpm<n>
drivers/char/tpm/tpm-chip.c | 90 +++++++++++++++----------------
drivers/char/tpm/tpm-dev-common.c | 8 +--
drivers/char/tpm/tpm-dev.c | 35 ++++++++++--
drivers/char/tpm/tpm-dev.h | 1 +
drivers/char/tpm/tpm-interface.c | 20 +++++--
drivers/char/tpm/tpm.h | 3 +-
drivers/char/tpm/tpm2-space.c | 5 +-
drivers/char/tpm/tpm_tis_core.c | 3 +-
drivers/char/tpm/tpmrm-dev.c | 20 ++++++-
include/linux/tpm.h | 4 +-
10 files changed, 124 insertions(+), 65 deletions(-)
--
2.51.0
^ permalink raw reply
* [PATCH v3 1/4] tpm: Remove tpm_find_get_ops
From: Jonathan McDowell @ 2025-10-20 11:30 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe
Cc: Linus Torvalds, James Bottomley, linux-integrity, linux-kernel
In-Reply-To: <cover.1760958898.git.noodles@meta.com>
From: Jonathan McDowell <noodles@meta.com>
tpm_find_get_ops() looks for the first valid TPM if the caller passes in
NULL. All internal users have been converted to either associate
themselves with a TPM directly, or call tpm_default_chip() as part of
their setup. Remove the no longer necessary tpm_find_get_ops().
Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: No changes.
v3: Move to start of patch series
drivers/char/tpm/tpm-chip.c | 36 --------------------------------
drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++----
drivers/char/tpm/tpm.h | 1 -
drivers/char/tpm/tpm_tis_core.c | 3 +--
4 files changed, 17 insertions(+), 43 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e25daf2396d3..30d00219f9f3 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -230,42 +230,6 @@ struct tpm_chip *tpm_default_chip(void)
}
EXPORT_SYMBOL_GPL(tpm_default_chip);
-/**
- * tpm_find_get_ops() - find and reserve a TPM chip
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- *
- * Finds a TPM chip and reserves its class device and operations. The chip must
- * be released with tpm_put_ops() after use.
- * This function is for internal use only. It supports existing TPM callers
- * by accepting NULL, but those callers should be converted to pass in a chip
- * directly.
- *
- * Return:
- * A reserved &struct tpm_chip instance.
- * %NULL if a chip is not found.
- * %NULL if the chip is not available.
- */
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
-{
- int rc;
-
- if (chip) {
- if (!tpm_try_get_ops(chip))
- return chip;
- return NULL;
- }
-
- chip = tpm_default_chip();
- if (!chip)
- return NULL;
- rc = tpm_try_get_ops(chip);
- /* release additional reference we got from tpm_default_chip() */
- put_device(&chip->dev);
- if (rc)
- return NULL;
- return chip;
-}
-
/**
* tpm_dev_release() - free chip memory and the device number
* @dev: the character device for the TPM chip
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c9f173001d0e..f745a098908b 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -313,10 +313,13 @@ int tpm_is_tpm2(struct tpm_chip *chip)
{
int rc;
- chip = tpm_find_get_ops(chip);
if (!chip)
return -ENODEV;
+ rc = tpm_try_get_ops(chip);
+ if (rc)
+ return rc;
+
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
tpm_put_ops(chip);
@@ -338,10 +341,13 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
{
int rc;
- chip = tpm_find_get_ops(chip);
if (!chip)
return -ENODEV;
+ rc = tpm_try_get_ops(chip);
+ if (rc)
+ return rc;
+
if (chip->flags & TPM_CHIP_FLAG_TPM2)
rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
else
@@ -369,10 +375,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
int rc;
int i;
- chip = tpm_find_get_ops(chip);
if (!chip)
return -ENODEV;
+ rc = tpm_try_get_ops(chip);
+ if (rc)
+ return rc;
+
for (i = 0; i < chip->nr_allocated_banks; i++) {
if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
rc = -EINVAL;
@@ -492,10 +501,13 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
if (!out || max > TPM_MAX_RNG_DATA)
return -EINVAL;
- chip = tpm_find_get_ops(chip);
if (!chip)
return -ENODEV;
+ rc = tpm_try_get_ops(chip);
+ if (rc)
+ return rc;
+
if (chip->flags & TPM_CHIP_FLAG_TPM2)
rc = tpm2_get_random(chip, out, max);
else
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2726bd38e5ac..02c07fef41ba 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -267,7 +267,6 @@ static inline void tpm_msleep(unsigned int delay_msec)
int tpm_chip_bootstrap(struct tpm_chip *chip);
int tpm_chip_start(struct tpm_chip *chip);
void tpm_chip_stop(struct tpm_chip *chip);
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
struct tpm_chip *tpm_chip_alloc(struct device *dev,
const struct tpm_class_ops *ops);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8954a8660ffc..e2a1769081b1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -265,8 +265,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
/*
* Dump stack for forensics, as invalid TPM_STS.x could be
- * potentially triggered by impaired tpm_try_get_ops() or
- * tpm_find_get_ops().
+ * potentially triggered by impaired tpm_try_get_ops().
*/
dump_stack();
}
--
2.51.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox