* 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
* Re: [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>
From: Mimi Zohar @ 2025-10-27 11:50 UTC (permalink / raw)
To: Jarkko Sakkinen, Jonathan McDowell
Cc: Peter Huewe, Jason Gunthorpe, Linus Torvalds, James Bottomley,
linux-integrity, linux-kernel
In-Reply-To: <aPvLtt7a7X0Y1LBI@kernel.org>
On Fri, 2025-10-24 at 21:55 +0300, Jarkko Sakkinen wrote:
> 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 :-)
Roberto has already commented on this patch set, saying it would affect IMA[1].
I still need to look at the patch set, but please don't break IMA.
[1]https://lore.kernel.org/linux-integrity/cec499d5130f37a7887d39b44efd8538dd361fe3.camel@huaweicloud.com/
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: Jarkko Sakkinen @ 2025-10-27 19:38 UTC (permalink / raw)
To: Roberto Sassu
Cc: Jonathan McDowell, Peter Huewe, 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).
Also this would be a problem with hwrng.
This probably needs to be refined somehow. I don't have a solution at
hand but "invariant" is that in-kernel caller should override user space
exclusion, even when O_EXCL is used.
>
> Thanks
>
> Roberto
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>
From: Jarkko Sakkinen @ 2025-10-27 19:41 UTC (permalink / raw)
To: Mimi Zohar
Cc: Jonathan McDowell, Peter Huewe, Jason Gunthorpe, Linus Torvalds,
James Bottomley, linux-integrity, linux-kernel
In-Reply-To: <6fe4fb1126f2d45b77637c34bf274bef4205a427.camel@linux.ibm.com>
On Mon, Oct 27, 2025 at 07:50:46AM -0400, Mimi Zohar wrote:
> On Fri, 2025-10-24 at 21:55 +0300, Jarkko Sakkinen wrote:
> > 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 :-)
>
> Roberto has already commented on this patch set, saying it would affect IMA[1].
> I still need to look at the patch set, but please don't break IMA.
See my response in that thread.
>
> [1]https://lore.kernel.org/linux-integrity/cec499d5130f37a7887d39b44efd8538dd361fe3.camel@huaweicloud.com/
>
> --
> thanks,
>
> Mimi
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] hwrng: tpm: Do not enable by default
From: Jarkko Sakkinen @ 2025-10-27 19:51 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Huewe, linux-integrity, 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, Oct 21, 2025 at 02:46:15PM +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.
>
> 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>
Looking at DRBG_* from [1] I don't see anything you describe. If OPTEE
writes NVRAM, then the implementation is broken.
Also AFAIK, it is pre-seeded per power cycle. There's nothing that even
distantly relates on using NVRAM.
[1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-4-Supporting-Routines-Code.pdf
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: Jarkko Sakkinen @ 2025-10-27 20:04 UTC (permalink / raw)
To: Ye Bin
Cc: a.fatoum, kernel, James.Bottomley, zohar, dhowells, paul, jmorris,
serge, linux-integrity, keyrings, linux-security-module, yebin10
In-Reply-To: <20251024061153.61470-1-yebin@huaweicloud.com>
On Fri, Oct 24, 2025 at 02:11:53PM +0800, 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
Please fix the broken design while at it:
1. Remove the ad-hoc compilation flag (i.e., CAAM_DEBUG).
2. Substitute pr_info calls with pr_debug calls.
Then you can turn then on and off either dynamically, or alternatively
from the kernel command-line [1].
[1] https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v2] tpm_crb: Add idle support for the Arm FF-A start method
From: Jarkko Sakkinen @ 2025-10-27 20:06 UTC (permalink / raw)
To: Stuart Yoder
Cc: linux-integrity, peterhuewe, jgg, sudeep.holla, Prachotan.Bathi,
linux-kernel
In-Reply-To: <20251024174200.1974090-1-stuart.yoder@arm.com>
On Fri, Oct 24, 2025 at 12:42:00PM -0500, Stuart Yoder wrote:
> 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>
A version of this went already to rc2.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: James Bottomley @ 2025-10-27 20:09 UTC (permalink / raw)
To: Jarkko Sakkinen, Roberto Sassu
Cc: Jonathan McDowell, Peter Huewe, Jason Gunthorpe, Linus Torvalds,
linux-integrity, linux-kernel, zohar
In-Reply-To: <aP_KT0GiQSzt1ClO@kernel.org>
On Mon, 2025-10-27 at 21:38 +0200, Jarkko Sakkinen wrote:
> On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
[...]
> > 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).
>
> Also this would be a problem with hwrng.
>
> This probably needs to be refined somehow. I don't have a solution at
> hand but "invariant" is that in-kernel caller should override user
> space exclusion, even when O_EXCL is used.
Also, are we sure we need O_EXCL in the first place? A well
functioning TPM is supposed to be able to cope with field upgrade while
it receives other commands. When it's in this state, it's supposed to
return TPM_RC_UPGRADE to inappropriate commands, so if we made sure we
can correctly handle that in the kernel, that might be enough to get
all this to work correctly without needing an exclusive open.
Of course, Field Upgrade is likely to be the least well tested of any
TPM capability, so there's a good chance at least one TPM out there
isn't going to behave as the standard says it should.
Regards,
James
^ permalink raw reply
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: Jarkko Sakkinen @ 2025-10-27 20:18 UTC (permalink / raw)
To: James Bottomley
Cc: Roberto Sassu, Jonathan McDowell, Peter Huewe, Jason Gunthorpe,
Linus Torvalds, linux-integrity, linux-kernel, zohar
In-Reply-To: <feb863c3e73f0b73b53d7d6f9889c79d37476855.camel@HansenPartnership.com>
On Mon, Oct 27, 2025 at 04:09:35PM -0400, James Bottomley wrote:
> On Mon, 2025-10-27 at 21:38 +0200, Jarkko Sakkinen wrote:
> > On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
> [...]
> > > 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).
> >
> > Also this would be a problem with hwrng.
> >
> > This probably needs to be refined somehow. I don't have a solution at
> > hand but "invariant" is that in-kernel caller should override user
> > space exclusion, even when O_EXCL is used.
>
> Also, are we sure we need O_EXCL in the first place? A well
> functioning TPM is supposed to be able to cope with field upgrade while
> it receives other commands. When it's in this state, it's supposed to
> return TPM_RC_UPGRADE to inappropriate commands, so if we made sure we
> can correctly handle that in the kernel, that might be enough to get
> all this to work correctly without needing an exclusive open.
>
> Of course, Field Upgrade is likely to be the least well tested of any
> TPM capability, so there's a good chance at least one TPM out there
> isn't going to behave as the standard says it should.
>
> Regards,
>
> James
I get that depending on configuration someone really would want to
have guaranteed exclusive access to the device. Since it is opt-in
via O_EXCL, I don't have anything in principle againts adding it.
The patch set needs rework but feature itself is totally fine.
BR, Jarkko
^ permalink raw reply
* [PATCH] tpm_crb: add missing loc parameter to kerneldoc
From: Stuart Yoder @ 2025-10-28 2:09 UTC (permalink / raw)
To: linux-integrity, jarkko, peterhuewe, jgg, sudeep.holla
Cc: Prachotan.Bathi, linux-kernel
Update the kerneldoc parameter definitions for __crb_go_idle
and __crb_cmd_ready to include the loc parameter.
Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
drivers/char/tpm/tpm_crb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index c75a531cfb98..0eb48429f73d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -179,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.
@@ -233,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.
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: yebin @ 2025-10-28 2:39 UTC (permalink / raw)
To: Ahmad Fatoum, kernel, James.Bottomley, jarkko, zohar, dhowells,
paul, jmorris, serge, linux-integrity, keyrings,
linux-security-module, yebin10
In-Reply-To: <85aca6f9-f279-4977-8888-416af127edac@pengutronix.de>
On 2025/10/24 16:10, Ahmad Fatoum wrote:
> 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.
>
This patch was submitted by Herbert Xu and is on linux-next.
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
commit 275a9a3f9b6a2158bfb7826074b72d5bdfb2ac35
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue Oct 21 12:07:56 2025 +0800
KEYS: trusted: Pass argument by pointer in dump_options
Instead of passing pkey_info into dump_options by value, using a
pointer instead.
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 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));
>>>
>>
>>
>
^ permalink raw reply
* Re: [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: yebin @ 2025-10-28 2:41 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: a.fatoum, kernel, James.Bottomley, zohar, dhowells, paul, jmorris,
serge, linux-integrity, keyrings, linux-security-module, yebin10
In-Reply-To: <aP_QWHPHpGr-nUZD@kernel.org>
On 2025/10/28 4:04, Jarkko Sakkinen wrote:
> On Fri, Oct 24, 2025 at 02:11:53PM +0800, 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
>
> Please fix the broken design while at it:
>
> 1. Remove the ad-hoc compilation flag (i.e., CAAM_DEBUG).
> 2. Substitute pr_info calls with pr_debug calls.
>
Alright, I'll fix the two points you mentioned.
> Then you can turn then on and off either dynamically, or alternatively
> from the kernel command-line [1].
>
> [1] https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html
>
> BR, Jarkko
>
^ permalink raw reply
* Re: [PATCH] hwrng: tpm: Do not enable by default
From: Jan Kiszka @ 2025-10-28 5:46 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-integrity, Linux Kernel Mailing List,
Ilias Apalodimas, Jens Wiklander, OP-TEE TrustedFirmware,
linux-crypto
In-Reply-To: <aP_NN3HwO4Hp0-9T@kernel.org>
On 27.10.25 20:51, Jarkko Sakkinen wrote:
> On Tue, Oct 21, 2025 at 02:46:15PM +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.
>>
>> 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>
>
> Looking at DRBG_* from [1] I don't see anything you describe. If OPTEE
> writes NVRAM, then the implementation is broken.
It's not OP-TEE, but it might be indeed an artifact of the reference
implementation that the fTPM is using because it is also targeting
simulation:
https://github.com/microsoft/ms-tpm-20-ref/blob/ee21db0a941decd3cac67925ea3310873af60ab3/TPMCmd/tpm/src/main/ExecCommand.c#L99
(Page 942 in [1])
-> ... ->
https://github.com/microsoft/ms-tpm-20-ref/blob/main/TPMCmd/tpm/src/subsystem/Time.c#L68
(Page 1075 in [1])
>
> Also AFAIK, it is pre-seeded per power cycle. There's nothing that even
> distantly relates on using NVRAM.
>
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-4-Supporting-Routines-Code.pdf
>
> BR, Jarkko
Given how detailed [1] is, we likely need to address that directly there
to avoid spreading this issue into fTPMs. Fact is, that there firmware
implementations out there which exactly do what [1] suggests: writing to
NV every 4 seconds on every command.
Jan
--
Siemens AG, Foundational Technologies
Linux Expert Center
^ permalink raw reply
* Re: [PATCH] KEYS: fix compilation warnings in the dump_options() function
From: Ahmad Fatoum @ 2025-10-28 7:03 UTC (permalink / raw)
To: yebin, kernel, James.Bottomley, jarkko, zohar, dhowells, paul,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
yebin10, Horia Geantă, Pankaj Gupta, Gaurav Jain,
linux-crypto, Herbert Xu, Meenakshi Aggarwal
In-Reply-To: <69002CFA.2040701@huaweicloud.com>
Hi,
On 10/28/25 3:39 AM, yebin wrote:
>
>
> On 2025/10/24 16:10, Ahmad Fatoum wrote:
>> 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.
>>
> This patch was submitted by Herbert Xu and is on linux-next.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> commit 275a9a3f9b6a2158bfb7826074b72d5bdfb2ac35
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Tue Oct 21 12:07:56 2025 +0800
>
> KEYS: trusted: Pass argument by pointer in dump_options
>
> Instead of passing pkey_info into dump_options by value, using a
> pointer instead.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks for the clarification. The maintainers for trusted keys were
unfortunately not Cc'd on that submission, so it flew at least under my
radar. :/
Cheers,
Ahmad
>
>
>> 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
* [PATCH] KEYS: Remove the ad-hoc compilation flag CAAM_DEBUG
From: Ye Bin @ 2025-10-28 13:22 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>
Fix the broken design based on Jarkko Sakkinen's suggestions as follows:
1. Remove the ad-hoc compilation flag (i.e., CAAM_DEBUG).
2. Substitute pr_info calls with pr_debug calls.
Closes: https://patchwork.kernel.org/project/linux-integrity/patch/20251024061153.61470-1-yebin@huaweicloud.com/
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
security/keys/trusted-keys/trusted_caam.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/keys/trusted-keys/trusted_caam.c
index 601943ce0d60..c903ee7328ca 100644
--- a/security/keys/trusted-keys/trusted_caam.c
+++ b/security/keys/trusted-keys/trusted_caam.c
@@ -28,16 +28,10 @@ static const match_table_t key_tokens = {
{opt_err, NULL}
};
-#ifdef CAAM_DEBUG
static inline void dump_options(const struct caam_pkey_info *pkey_info)
{
- pr_info("key encryption algo %d\n", pkey_info->key_enc_algo);
+ pr_debug("key encryption algo %d\n", pkey_info->key_enc_algo);
}
-#else
-static inline void dump_options(const struct caam_pkey_info *pkey_info)
-{
-}
-#endif
static int get_pkey_options(char *c,
struct caam_pkey_info *pkey_info)
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 1/2] vfs: Allow filesystems with foreign owner IDs to override UID checks
From: Christian Brauner @ 2025-10-29 12:39 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: <1523597.1761052821@warthog.procyon.org.uk>
On Tue, Oct 21, 2025 at 02:20:21PM +0100, David Howells wrote:
> 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)) {
I think I mentioned this off-list. It looks like we can but I don't know
if there was any history behind not doing it that way. But please try.
^ permalink raw reply
* [PATCH] KEYS: encrypted: Return early on allocation failure and drop goto
From: Thorsten Blum @ 2025-10-29 16:31 UTC (permalink / raw)
To: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-integrity, keyrings, linux-security-module,
linux-kernel
Return ERR_PTR(-ENOMEM) immediately if memory allocation fails, instead
of using goto and returning a NULL pointer, and remove the now-unused
'out' label.
At the call site, check 'ascii_buf' with IS_ERR() and propagate the
error code returned by datablob_format().
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/keys/encrypted-keys/encrypted.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index be1f2118447c..25df00b7dbe9 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -276,7 +276,7 @@ static char *datablob_format(struct encrypted_key_payload *epayload,
ascii_buf = kmalloc(asciiblob_len + 1, GFP_KERNEL);
if (!ascii_buf)
- goto out;
+ return ERR_PTR(-ENOMEM);
ascii_buf[asciiblob_len] = '\0';
@@ -288,7 +288,6 @@ static char *datablob_format(struct encrypted_key_payload *epayload,
bufp = &ascii_buf[len];
for (i = 0; i < (asciiblob_len - len) / 2; i++)
bufp = hex_byte_pack(bufp, iv[i]);
-out:
return ascii_buf;
}
@@ -932,8 +931,8 @@ static long encrypted_read(const struct key *key, char *buffer,
goto out;
ascii_buf = datablob_format(epayload, asciiblob_len);
- if (!ascii_buf) {
- ret = -ENOMEM;
+ if (IS_ERR(ascii_buf)) {
+ ret = PTR_ERR(ascii_buf);
goto out;
}
--
2.51.0
^ permalink raw reply related
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-30 0:31 UTC (permalink / raw)
To: Mimi Zohar
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: <02d18fe0a0ca1223eec9af5c8e01739aa164bf32.camel@linux.ibm.com>
On Fri, Oct 24, 2025 at 11:16:37AM -0400, Mimi Zohar 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().
>
>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);
Thanks for proposing a better solution! Yeah, decompressing the kernel
module in IMA is unnecessary. Do you think we can further extend your
idea to call one IMA hook only after kernel module decompression is
done? If we call two IMA hooks in finit_module, we'll need coordination
between two IMA hooks i.e. the 1st IMA hook shouldn't fail in order for
the 2nd IMA hook to be executed. The new IMA hook will always have
access to the decompressed kernel module buffer so there is no need to
differentiate between compressed and decompressed module.
Another question is whether we should allow loading a kernel module with
appended signature but misses IMA signature. Both IMA arch specific policy
and init_module syscall only require appended signature verification. On
the other hand, we only allow "appraise_type=imasig|modsig" but not
appraise_type=modsig. How about we allow loading a kernel module with
valid appended signature regardless of its IMA signature? We won't call
set_module_sig_enforced but as long as we know is_module_sig_enforced()
is true, we allow the module in IMA.
>
>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.
I haven't figured out why to include "template=ima-modsig" for testing
purpose considering we can simply check if the kernel module has been
loaded successfully. It it related to the design that "The d-modsig and
modsig fields are only populated if both the measure and appraise rules
trigger"? If so, can you also help me understand there is such a design?
[1] https://ima-doc.readthedocs.io/en/latest/event-log-format.html#ima-modsig
>--
>Mimi
>
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-30 0:33 UTC (permalink / raw)
To: Mimi Zohar, Roberto Sassu
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: <497d1608572eec7d3d498967f0bae7d45023635b.camel@linux.ibm.com>
On Mon, Oct 20, 2025 at 09:57:19AM -0400, Mimi Zohar wrote:
>On Mon, 2025-10-20 at 14:45 +0200, Roberto Sassu wrote:
>> On Mon, 2025-10-20 at 08:21 -0400, Mimi Zohar wrote:
[...]
>> >
>> > >
>> > > 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!
+1, I'm looking forward to the reposted patch set!
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-30 3:01 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: <xldwpvz4gzp74kualadf2n2wdlpbo3xorrgvkibhdocjtroipd@dpukmalev4yu>
On Thu, 2025-10-30 at 08:31 +0800, Coiby Xu wrote:
> On Fri, Oct 24, 2025 at 11:16:37AM -0400, Mimi Zohar 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().
> >
> > 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);
>
> Thanks for proposing a better solution! Yeah, decompressing the kernel
> module in IMA is unnecessary. Do you think we can further extend your
> idea to call one IMA hook only after kernel module decompression is
> done? If we call two IMA hooks in finit_module, we'll need coordination
> between two IMA hooks i.e. the 1st IMA hook shouldn't fail in order for
> the 2nd IMA hook to be executed. The new IMA hook will always have
> access to the decompressed kernel module buffer so there is no need to
> differentiate between compressed and decompressed module.
Agreed instead of verifying the kernel module signature on the LSM
kernel_post_read_file() hook, define and move it to a new IMA/LSM hook after it
is decompressed, would be much simpler than coordinating two LSM hooks.
>
> Another question is whether we should allow loading a kernel module with
> appended signature but misses IMA signature. Both IMA arch specific policy
> and init_module syscall only require appended signature verification. On
> the other hand, we only allow "appraise_type=imasig|modsig" but not
> appraise_type=modsig. How about we allow loading a kernel module with
> valid appended signature regardless of its IMA signature? We won't call
> set_module_sig_enforced but as long as we know is_module_sig_enforced()
> is true, we allow the module in IMA.
Based on the policy, IMA enforces signature verification. Only if
CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
IMA kernel module appraise rule. However, custom policies could still require
both types of signatures, not necessarily signed by the same entity.
The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
appended signature.
>
> >
> > 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.
>
> I haven't figured out why to include "template=ima-modsig" for testing
> purpose considering we can simply check if the kernel module has been
> loaded successfully.
That's fine too.
> It it related to the design that "The d-modsig and
> modsig fields are only populated if both the measure and appraise rules
> trigger"? If so, can you also help me understand there is such a design?
>
> [1] https://ima-doc.readthedocs.io/en/latest/event-log-format.html#ima-modsig
The "ima-sig" template contains the file data hash and file signature, allowing
the attestation server to verify the signature based on the file data hash
contained in the measurement list.
In addition to the file data hash and the file signature, the "ima-modsig"
template contains the file data hash without the appended signature, allowing
the attestation server to verify the appended signature against it.
Mimi
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-30 13:42 UTC (permalink / raw)
To: Mimi Zohar
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: <26cb0926bd707edea6f19ca1bf8f5d5d3d10ff96.camel@linux.ibm.com>
On Wed, Oct 29, 2025 at 11:01:27PM -0400, Mimi Zohar wrote:
>On Thu, 2025-10-30 at 08:31 +0800, Coiby Xu wrote:
>> On Fri, Oct 24, 2025 at 11:16:37AM -0400, Mimi Zohar 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().
>> >
>> > 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);
>>
>> Thanks for proposing a better solution! Yeah, decompressing the kernel
>> module in IMA is unnecessary. Do you think we can further extend your
>> idea to call one IMA hook only after kernel module decompression is
>> done? If we call two IMA hooks in finit_module, we'll need coordination
>> between two IMA hooks i.e. the 1st IMA hook shouldn't fail in order for
>> the 2nd IMA hook to be executed. The new IMA hook will always have
>> access to the decompressed kernel module buffer so there is no need to
>> differentiate between compressed and decompressed module.
>
>Agreed instead of verifying the kernel module signature on the LSM
>kernel_post_read_file() hook, define and move it to a new IMA/LSM hook after it
>is decompressed, would be much simpler than coordinating two LSM hooks.
Thanks for confirming it! I'll send a new version once the testing is
finished.
>
>>
>> Another question is whether we should allow loading a kernel module with
>> appended signature but misses IMA signature. Both IMA arch specific policy
>> and init_module syscall only require appended signature verification. On
>> the other hand, we only allow "appraise_type=imasig|modsig" but not
>> appraise_type=modsig. How about we allow loading a kernel module with
>> valid appended signature regardless of its IMA signature? We won't call
>> set_module_sig_enforced but as long as we know is_module_sig_enforced()
>> is true, we allow the module in IMA.
>
>Based on the policy, IMA enforces signature verification. Only if
>CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
>IMA kernel module appraise rule. However, custom policies could still require
>both types of signatures, not necessarily signed by the same entity.
>
>The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
>appended signature.
Thanks for the clarification! If I understand you correctly, some users
may want to enforce IMA signature verification and we should provide
such flexibility. Then do you think it's a good idea to change the kernel
module rule in ima_policy=secure_boot to
"appraise func=MODULE_CHECK appraise_type=imasig|modsig" so
ima_policy=secure_boot can also work for in-kernel decompressing
modules?
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-10-30 16:50 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: <pbkvp4o4m3spjgvctapidfnsswakekxl2vyigqip6yyfzp73z4@rgbohq7h4nnt>
On Thu, 2025-10-30 at 21:42 +0800, Coiby Xu wrote:
> > >
> > > Another question is whether we should allow loading a kernel module with
> > > appended signature but misses IMA signature. Both IMA arch specific policy
> > > and init_module syscall only require appended signature verification. On
> > > the other hand, we only allow "appraise_type=imasig|modsig" but not
> > > appraise_type=modsig. How about we allow loading a kernel module with
> > > valid appended signature regardless of its IMA signature? We won't call
> > > set_module_sig_enforced but as long as we know is_module_sig_enforced()
> > > is true, we allow the module in IMA.
> >
> > Based on the policy, IMA enforces signature verification. Only if
> > CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
> > IMA kernel module appraise rule. However, custom policies could still require
> > both types of signatures, not necessarily signed by the same entity.
> >
> > The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
> > appended signature.
>
> Thanks for the clarification! If I understand you correctly, some users
> may want to enforce IMA signature verification and we should provide
> such flexibility. Then do you think it's a good idea to change the kernel
> module rule in ima_policy=secure_boot to
> "appraise func=MODULE_CHECK appraise_type=imasig|modsig" so
> ima_policy=secure_boot can also work for in-kernel decompressing
> modules?
Yes, that's fine. Unlike the arch specific policy rules and the Kconfig
appraise rules, which persist after loading a custom policy, the builtin secure
boot rules do not persist.
Mimi
^ permalink raw reply
* [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Coiby Xu @ 2025-10-31 7:40 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Mimi Zohar
Cc: Karel Srot, Paul Moore, James Morris, Serge E. Hallyn,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, open list,
open list:MODULE SUPPORT
In-Reply-To: <20250928030358.3873311-1-coxu@redhat.com>
Currently, when in-kernel module decompression (CONFIG_MODULE_DECOMPRESS)
is enabled, IMA has no way to verify the appended module signature as it
can't decompress the module.
Define a new LSM hook security_kernel_module_read_file which will be
called after kernel module decompression is done so IMA can access the
decompressed kernel module to verify the appended signature.
Since IMA can access both xattr and appended kernel module signature
with the new LSM hook, it no longer uses the security_kernel_post_read_file
LSM hook for kernel module loading.
Before enabling in-kernel module decompression, a kernel module in
initramfs can still be loaded with ima_policy=secure_boot. So adjust the
kernel module rule in secure_boot policy to allow either an IMA
signature OR an appended signature i.e. to use
"appraise func=MODULE_CHECK appraise_type=imasig|modsig".
Reported-by: Karel Srot <ksrot@redhat.com>
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
v1: https://lore.kernel.org/linux-integrity/20250928030358.3873311-1-coxu@redhat.com/
include/linux/lsm_hook_defs.h | 2 ++
include/linux/security.h | 7 +++++++
kernel/module/main.c | 10 +++++++++-
security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 2 +-
security/security.c | 17 +++++++++++++++++
6 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 8c42b4bde09c..ced42eb8b618 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -232,6 +232,8 @@ LSM_HOOK(int, 0, kernel_read_file, struct file *file,
enum kernel_read_file_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
loff_t size, enum kernel_read_file_id id)
+LSM_HOOK(int, 0, kernel_module_read_file, struct file *file, char *buf,
+ loff_t size)
LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
int flags)
LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old,
diff --git a/include/linux/security.h b/include/linux/security.h
index 92ac3f27b973..e47951292c73 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -508,6 +508,7 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
bool contents);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
+int security_kernel_module_read_file(struct file *file, char *buf, loff_t size);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
int security_task_fix_setgid(struct cred *new, const struct cred *old,
@@ -1295,6 +1296,12 @@ static inline int security_kernel_post_read_file(struct file *file,
return 0;
}
+static inline int security_kernel_module_read_file(struct file *file,
+ char *buf, loff_t size)
+{
+ return 0;
+}
+
static inline int security_task_fix_setuid(struct cred *new,
const struct cred *old,
int flags)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..40bc86fa7384 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3678,6 +3678,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
struct load_info info = { };
void *buf = NULL;
int len;
+ int err;
len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
if (len < 0) {
@@ -3686,7 +3687,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
}
if (flags & MODULE_INIT_COMPRESSED_FILE) {
- int err = module_decompress(&info, buf, len);
+ err = module_decompress(&info, buf, len);
vfree(buf); /* compressed data is no longer needed */
if (err) {
mod_stat_inc(&failed_decompress);
@@ -3698,6 +3699,13 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
info.len = len;
}
+ err = security_kernel_module_read_file(f, (char *)info.hdr, info.len);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ free_copy(&info, flags);
+ return err;
+ }
+
return load_module(&info, uargs, flags);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cdd225f65a62..53d2e90176ea 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -635,6 +635,27 @@ static int ima_file_check(struct file *file, int mask)
MAY_APPEND), FILE_CHECK);
}
+/**
+ * ima_read_kernel_module - collect/appraise/audit measurement
+ * @file: file pointer to the module.
+ * @buf: buffer containing module data (possibly decompressed).
+ * @size: size of the buffer.
+ *
+ * This IMA hook for kernel_module_read_file LSM hook is called after a kernel
+ * module has been read into memory and (if applicable) decompressed. It
+ * measures and/or appraises the module based on the IMA policy.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int ima_read_kernel_module(struct file *file, char *buf, loff_t size)
+{
+ struct lsm_prop prop;
+
+ security_current_getlsmprop_subj(&prop);
+ return process_measurement(file, current_cred(), &prop, buf, size,
+ MAY_READ, MODULE_CHECK);
+}
+
static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
size_t buf_size)
{
@@ -881,6 +902,10 @@ static int ima_post_read_file(struct file *file, char *buf, loff_t size,
enum ima_hooks func;
struct lsm_prop prop;
+ /* kernel module will be addressed in ima_read_kernel_module */
+ if (read_id == READING_MODULE)
+ return 0;
+
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -1250,6 +1275,7 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
LSM_HOOK_INIT(kernel_load_data, ima_load_data),
LSM_HOOK_INIT(kernel_post_load_data, ima_post_load_data),
LSM_HOOK_INIT(kernel_read_file, ima_read_file),
+ LSM_HOOK_INIT(kernel_module_read_file, ima_read_kernel_module),
LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
LSM_HOOK_INIT(path_post_mknod, ima_post_path_mknod),
#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 128fab897930..2c9bdc618ac9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -241,7 +241,7 @@ static struct ima_rule_entry build_appraise_rules[] __ro_after_init = {
static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
{.action = APPRAISE, .func = MODULE_CHECK,
- .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST},
{.action = APPRAISE, .func = FIRMWARE_CHECK,
.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
{.action = APPRAISE, .func = KEXEC_KERNEL_CHECK,
diff --git a/security/security.c b/security/security.c
index 4d3c03a4524c..311ba63a8889 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3442,6 +3442,23 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
}
EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
+/**
+ * security_kernel_module_read_file() - Read a kernel module loaded by finit_module
+ * @file: file
+ * @buf: contents of decompressed kernel module
+ * @size: size of decompressed kernel module
+ *
+ * Read a kernel module loaded by the finit_module syscall. Unlike
+ * security_kernel_post_read_file, it has access to the decompressed kernel module.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_kernel_module_read_file(struct file *file, char *buf, loff_t size)
+{
+ return call_int_hook(kernel_module_read_file, file, buf, size);
+}
+EXPORT_SYMBOL_GPL(security_kernel_module_read_file);
+
/**
* security_kernel_load_data() - Load data provided by userspace
* @id: data identifier
base-commit: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6
--
2.51.0
^ permalink raw reply related
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-10-31 7:58 UTC (permalink / raw)
To: Mimi Zohar
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: <1fb245a0e72a360df3a768726351e7fa76301471.camel@linux.ibm.com>
On Thu, Oct 30, 2025 at 12:50:48PM -0400, Mimi Zohar wrote:
>On Thu, 2025-10-30 at 21:42 +0800, Coiby Xu wrote:
>> > >
>> > > Another question is whether we should allow loading a kernel module with
>> > > appended signature but misses IMA signature. Both IMA arch specific policy
>> > > and init_module syscall only require appended signature verification. On
>> > > the other hand, we only allow "appraise_type=imasig|modsig" but not
>> > > appraise_type=modsig. How about we allow loading a kernel module with
>> > > valid appended signature regardless of its IMA signature? We won't call
>> > > set_module_sig_enforced but as long as we know is_module_sig_enforced()
>> > > is true, we allow the module in IMA.
>> >
>> > Based on the policy, IMA enforces signature verification. Only if
>> > CONFIG_MODULE_SIG is configured, the IMA arch specific policy does not define an
>> > IMA kernel module appraise rule. However, custom policies could still require
>> > both types of signatures, not necessarily signed by the same entity.
>> >
>> > The option "appraise_type=imasig|modsig" allows either an IMA signature OR an
>> > appended signature.
>>
>> Thanks for the clarification! If I understand you correctly, some users
>> may want to enforce IMA signature verification and we should provide
>> such flexibility. Then do you think it's a good idea to change the kernel
>> module rule in ima_policy=secure_boot to
>> "appraise func=MODULE_CHECK appraise_type=imasig|modsig" so
>> ima_policy=secure_boot can also work for in-kernel decompressing
>> modules?
>
>Yes, that's fine. Unlike the arch specific policy rules and the Kconfig
>appraise rules, which persist after loading a custom policy, the builtin secure
>boot rules do not persist.
Thanks for the confirmation! v2 has been posted.
>
>Mimi
>
>
--
Best regards,
Coiby
^ permalink raw reply
* [PATCH] module: Only declare set_module_sig_enforced when CONFIG_MODULE_SIG=y
From: Coiby Xu @ 2025-10-31 8:09 UTC (permalink / raw)
To: linux-modules
Cc: linux-integrity, kernel test robot, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, open list:MODULE SUPPORT
Currently, set_module_sig_enforced is declared as long as CONFIG_MODULES
is enabled. This can lead to a linking error if
set_module_sig_enforced is called with CONFIG_MODULE_SIG=n,
ld: security/integrity/ima/ima_appraise.o: in function `ima_appraise_measurement':
security/integrity/ima/ima_appraise.c:587:(.text+0xbbb): undefined reference to `set_module_sig_enforced'
So only declare set_module_sig_enforced when CONFIG_MODULE_SIG is
enabled.
Note this issue hasn't caused a real problem because all current callers
of set_module_sig_enforced e.g. security/integrity/ima/ima_efi.c
depend on CONFIG_MODULE_SIG=y.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202510030029.VRKgik99-lkp@intel.com/
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
include/linux/module.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index e135cc79acee..fa251958b138 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -769,8 +769,6 @@ static inline bool is_livepatch_module(struct module *mod)
#endif
}
-void set_module_sig_enforced(void);
-
void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data);
#else /* !CONFIG_MODULES... */
@@ -865,10 +863,6 @@ static inline bool module_requested_async_probing(struct module *module)
}
-static inline void set_module_sig_enforced(void)
-{
-}
-
/* Dereference module function descriptor */
static inline
void *dereference_module_function_descriptor(struct module *mod, void *ptr)
@@ -924,6 +918,8 @@ static inline bool retpoline_module_ok(bool has_retpoline)
#ifdef CONFIG_MODULE_SIG
bool is_module_sig_enforced(void);
+void set_module_sig_enforced(void);
+
static inline bool module_sig_ok(struct module *module)
{
return module->sig_ok;
@@ -934,6 +930,10 @@ static inline bool is_module_sig_enforced(void)
return false;
}
+static inline void set_module_sig_enforced(void)
+{
+}
+
static inline bool module_sig_ok(struct module *module)
{
return true;
base-commit: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6
--
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