* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-05 20:25 UTC (permalink / raw)
To: Paul Moore
Cc: Coiby Xu, linux-integrity, linux-security-module, Karel Srot,
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: <CAHC9VhS6xWvu5TjjS4MRGFEWxdAhg-Xsf6L+=K0k8U+fgiAtTQ@mail.gmail.com>
On Wed, 2025-11-05 at 10:42 -0500, Paul Moore wrote:
> On Wed, Nov 5, 2025 at 9:07 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Tue, 2025-11-04 at 21:47 -0500, Paul Moore wrote:
> > > Assuming I'm understanding the problem correctly, I think you're
> > > making this harder than it needs to be. I believe something like this
> > > should solve the problem without having to add more conditionals
> > > around the hooks in kernel_read_file(), and limiting the multiple
> > > security_kernel_post_read_file() calls to just the compressed case ...
> > > and honestly in each of the _post_read_file() calls in the compressed
> > > case, the buffer contents have changed so it somewhat makes sense.
> >
> > > Given the code below, IMA could simply ignore the
> > > READING_MODULE_COMPRESSED case (or whatever it is the IMA needs to do
> > > in that case) and focus on the READING_MODULE case as it does today.
> > > I expect the associated IMA patch would be both trivial and small.
> > >
> > > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > > index c66b26184936..b435c498ec01 100644
> > > --- a/kernel/module/main.c
> > > +++ b/kernel/module/main.c
> > > @@ -3675,17 +3675,19 @@ static int idempotent_wait_for_completion(struct idempot
> > > ent *u)
> > >
> > > static int init_module_from_file(struct file *f, const char __user * uargs, int
> > > flags)
> > > {
> > > + bool compressed = !!(flags & MODULE_INIT_COMPRESSED_FILE);
> > > struct load_info info = { };
> > > void *buf = NULL;
> > > int len;
> > >
> > > - len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
> > > + len = kernel_read_file(f, 0, &buf, INT_MAX, NULL,
> > > + compressed ? READING_MODULE_COMPRESSED : READING_
> > > MODULE);
> > > if (len < 0) {
> > > mod_stat_inc(&failed_kreads);
> > > return len;
> > > }
> > >
> > > - if (flags & MODULE_INIT_COMPRESSED_FILE) {
> > > + if (compressed) {
> > > int err = module_decompress(&info, buf, len);
> > > vfree(buf); /* compressed data is no longer needed */
> > > if (err) {
> > > @@ -3693,6 +3695,14 @@ static int init_module_from_file(struct file *f, const ch
> > > ar __user * uargs, int
> > > mod_stat_add_long(len, &invalid_decompress_bytes);
> > > return err;
> > > }
> > > +
> > > + err = security_kernel_post_read_file(f,
> > > + (char *)info.hdr, info.len,
> > > + READING_MODULE);
> >
> > Without changing the enumeration here, IMA would not be able to differentiate
> > the first call to security_kernel_post_read_file() and this one. The first call
> > would result in unnecessary error messages.
>
> Given the patch snippet above, in the case where an uncompressed
> module is passed into init_module_from_file() there would be the
> following checks, in this order:
>
> * kernel_read_file()
> -> security_kernel_read_file(READING_MODULE)
> -> security_kernel_post_read_file(READING_MODULE)
> * init_module_from_file()
> -> NONE
>
> ... this should be the same as the current behavior.
>
> In the case where a compressed module is passed into
> init_module_from_file() there would be the following checks, in this
> order:
>
> * kernel_read_file()
> -> security_kernel_read_file(READING_MODULE_COMPRESSED)
> -> security_kernel_post_read_file(READING_MODULE_COMPRESSED)
> * init_module_from_file()
> -> security_kernel_post_read_file(READING_MODULE)
>
> ... the two differences being that the hook calls in
> kernel_read_file() use the READING_MODULE_COMPRESSED id, which seems
> appropriate as the data passed to the hook is the compressed
> representation, and the additional _post_read_file() hook call in
> init_module_from_file() using the READING_MODULE id, as the data
> passed to the hook is now uncompressed. Not only should IMA be able
> to easily differentiate between the two _post_read_file() calls, but
> it should have access to both the compressed and uncompressed data.
Thanks, Paul. Yes, a single additional enumeration is enough.
Mimi
^ permalink raw reply
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: James Bottomley @ 2025-11-05 16:33 UTC (permalink / raw)
To: Bagas Sanjaya, Linux Kernel Mailing List, Linux Documentation,
Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <aQtLerZYehQRWdqe@archie.me>
[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]
On Wed, 2025-11-05 at 20:04 +0700, Bagas Sanjaya wrote:
> On Tue, Nov 04, 2025 at 10:32:50PM -0500, James Bottomley wrote:
> > On Wed, 2025-11-05 at 08:57 +0700, Bagas Sanjaya wrote:
> > > On Tue, Nov 04, 2025 at 09:55:08AM -0500, James Bottomley wrote:
> > > > On Tue, 2025-11-04 at 20:13 +0700, Bagas Sanjaya wrote:
> > > > > The last section heading in TPM security docs is formatted as
> > > > > title heading instead. As such, it shows up as TPM toctree
> > > > > entry. Demote it to section heading as appropriate.
> > > >
> > > > It's supposed to be a separate heading. It's explaining how to
> > > > certify your booted kernel rather than describing TPM security
> > > > within the kernel.
> > >
> > > Should I keep the whole section as-is or should I move it to
> > > separate docs?
> >
> > Why might it need moving?
>
> Just to tidy up toctree then...
I'd really rather have the files in the doc tree grouped for ease (and
reminder of) maintenance. The two headings belong together in one file
because if someone updates the doc for one, there's at least a chance
they'll notice the other might need an update as well; whereas if we do
separate files for every heading the tree becomes very fragmented and
the chance of something being missed increases.
Regards,
James
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Paul Moore @ 2025-11-05 15:42 UTC (permalink / raw)
To: Mimi Zohar
Cc: Coiby Xu, linux-integrity, linux-security-module, Karel Srot,
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: <0c7e94a436a3742003e5e1155a48480d8307a9c7.camel@linux.ibm.com>
On Wed, Nov 5, 2025 at 9:07 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Tue, 2025-11-04 at 21:47 -0500, Paul Moore wrote:
> > Assuming I'm understanding the problem correctly, I think you're
> > making this harder than it needs to be. I believe something like this
> > should solve the problem without having to add more conditionals
> > around the hooks in kernel_read_file(), and limiting the multiple
> > security_kernel_post_read_file() calls to just the compressed case ...
> > and honestly in each of the _post_read_file() calls in the compressed
> > case, the buffer contents have changed so it somewhat makes sense.
>
> > Given the code below, IMA could simply ignore the
> > READING_MODULE_COMPRESSED case (or whatever it is the IMA needs to do
> > in that case) and focus on the READING_MODULE case as it does today.
> > I expect the associated IMA patch would be both trivial and small.
> >
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index c66b26184936..b435c498ec01 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -3675,17 +3675,19 @@ static int idempotent_wait_for_completion(struct idempot
> > ent *u)
> >
> > static int init_module_from_file(struct file *f, const char __user * uargs, int
> > flags)
> > {
> > + bool compressed = !!(flags & MODULE_INIT_COMPRESSED_FILE);
> > struct load_info info = { };
> > void *buf = NULL;
> > int len;
> >
> > - len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
> > + len = kernel_read_file(f, 0, &buf, INT_MAX, NULL,
> > + compressed ? READING_MODULE_COMPRESSED : READING_
> > MODULE);
> > if (len < 0) {
> > mod_stat_inc(&failed_kreads);
> > return len;
> > }
> >
> > - if (flags & MODULE_INIT_COMPRESSED_FILE) {
> > + if (compressed) {
> > int err = module_decompress(&info, buf, len);
> > vfree(buf); /* compressed data is no longer needed */
> > if (err) {
> > @@ -3693,6 +3695,14 @@ static int init_module_from_file(struct file *f, const ch
> > ar __user * uargs, int
> > mod_stat_add_long(len, &invalid_decompress_bytes);
> > return err;
> > }
> > +
> > + err = security_kernel_post_read_file(f,
> > + (char *)info.hdr, info.len,
> > + READING_MODULE);
>
> Without changing the enumeration here, IMA would not be able to differentiate
> the first call to security_kernel_post_read_file() and this one. The first call
> would result in unnecessary error messages.
Given the patch snippet above, in the case where an uncompressed
module is passed into init_module_from_file() there would be the
following checks, in this order:
* kernel_read_file()
-> security_kernel_read_file(READING_MODULE)
-> security_kernel_post_read_file(READING_MODULE)
* init_module_from_file()
-> NONE
... this should be the same as the current behavior.
In the case where a compressed module is passed into
init_module_from_file() there would be the following checks, in this
order:
* kernel_read_file()
-> security_kernel_read_file(READING_MODULE_COMPRESSED)
-> security_kernel_post_read_file(READING_MODULE_COMPRESSED)
* init_module_from_file()
-> security_kernel_post_read_file(READING_MODULE)
... the two differences being that the hook calls in
kernel_read_file() use the READING_MODULE_COMPRESSED id, which seems
appropriate as the data passed to the hook is the compressed
representation, and the additional _post_read_file() hook call in
init_module_from_file() using the READING_MODULE id, as the data
passed to the hook is now uncompressed. Not only should IMA be able
to easily differentiate between the two _post_read_file() calls, but
it should have access to both the compressed and uncompressed data.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-05 14:07 UTC (permalink / raw)
To: Paul Moore, Coiby Xu
Cc: linux-integrity, linux-security-module, Karel Srot, 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: <CAHC9VhRGwXvhU64Nk5jdmtPfrt9bbkzpLVqS0LRbtN3Q3HhnCw@mail.gmail.com>
On Tue, 2025-11-04 at 21:47 -0500, Paul Moore wrote:
> Assuming I'm understanding the problem correctly, I think you're
> making this harder than it needs to be. I believe something like this
> should solve the problem without having to add more conditionals
> around the hooks in kernel_read_file(), and limiting the multiple
> security_kernel_post_read_file() calls to just the compressed case ...
> and honestly in each of the _post_read_file() calls in the compressed
> case, the buffer contents have changed so it somewhat makes sense.
> Given the code below, IMA could simply ignore the
> READING_MODULE_COMPRESSED case (or whatever it is the IMA needs to do
> in that case) and focus on the READING_MODULE case as it does today.
> I expect the associated IMA patch would be both trivial and small.
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..b435c498ec01 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3675,17 +3675,19 @@ static int idempotent_wait_for_completion(struct idempot
> ent *u)
>
> static int init_module_from_file(struct file *f, const char __user * uargs, int
> flags)
> {
> + bool compressed = !!(flags & MODULE_INIT_COMPRESSED_FILE);
> struct load_info info = { };
> void *buf = NULL;
> int len;
>
> - len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
> + len = kernel_read_file(f, 0, &buf, INT_MAX, NULL,
> + compressed ? READING_MODULE_COMPRESSED : READING_
> MODULE);
> if (len < 0) {
> mod_stat_inc(&failed_kreads);
> return len;
> }
>
> - if (flags & MODULE_INIT_COMPRESSED_FILE) {
> + if (compressed) {
> int err = module_decompress(&info, buf, len);
> vfree(buf); /* compressed data is no longer needed */
> if (err) {
> @@ -3693,6 +3695,14 @@ static int init_module_from_file(struct file *f, const ch
> ar __user * uargs, int
> mod_stat_add_long(len, &invalid_decompress_bytes);
> return err;
> }
> +
> + err = security_kernel_post_read_file(f,
> + (char *)info.hdr, info.len,
> + READING_MODULE);
Without changing the enumeration here, IMA would not be able to differentiate
the first call to security_kernel_post_read_file() and this one. The first call
would result in unnecessary error messages.
Adding an additional call to security_kernel_post_read_file() here, would
require defining 2 additional enumerations: READING_MODULE_COMPRESSED,
READING_MODULE_DECOMPRESSED.
> + if (err) {
> + mod_stat_inc(&failed_kreads);
> + return err;
> + }
> } else {
> info.hdr = buf;
> info.len = len;
Deferring the security_kernel_post_read_file() call to here, eliminates the need
for defining additional enumerations. (Coiby's first link.)
Adding an additional call to security_kernel_post_read_file() here, requires 1
additional enumeration. (Coiby's 2nd link.)
Mimi
^ permalink raw reply
* [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-05 13:09 UTC (permalink / raw)
To: kexec, linux-integrity
Cc: Pingfan Liu, Andrew Morton, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <20251105130922.13321-1-piliu@redhat.com>
When I tested kexec with the latest kernel, I ran into the following warning:
[ 40.712410] ------------[ cut here ]------------
[ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
[...]
[ 40.816047] Call trace:
[ 40.818498] kimage_map_segment+0x144/0x198 (P)
[ 40.823221] ima_kexec_post_load+0x58/0xc0
[ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
[...]
[ 40.855423] ---[ end trace 0000000000000000 ]---
This is caused by the fact that kexec allocates the destination directly
in the CMA area. In that case, the CMA kernel address should be exported
directly to the IMA component, instead of using the vmalloc'd address.
Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Alexander Graf <graf@amazon.com>
Cc: Steven Chen <chenste@linux.microsoft.com>
Cc: linux-integrity@vger.kernel.org
To: kexec@lists.infradead.org
---
kernel/kexec_core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 9a1966207041..abe40286a02c 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
kimage_entry_t *ptr, entry;
struct page **src_pages;
unsigned int npages;
+ struct page *cma;
void *vaddr = NULL;
int i;
@@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
size = image->segment[idx].memsz;
eaddr = addr + size;
+ cma = image->segment_cma[idx];
+ if (cma)
+ return cma;
/*
* Collect the source pages and map them in a contiguous VA range.
*/
@@ -1014,7 +1018,8 @@ void *kimage_map_segment(struct kimage *image, int idx)
void kimage_unmap_segment(void *segment_buffer)
{
- vunmap(segment_buffer);
+ if (is_vmalloc_addr(segment_buffer))
+ vunmap(segment_buffer);
}
struct kexec_load_limit {
--
2.49.0
^ permalink raw reply related
* [PATCH 1/2] kernel/kexec: Change the prototype of kimage_map_segment()
From: Pingfan Liu @ 2025-11-05 13:09 UTC (permalink / raw)
To: kexec, linux-integrity
Cc: Pingfan Liu, Andrew Morton, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
The kexec segment index will be required to extract the corresponding
information for that segment in kimage_map_segment(). Additionally,
kexec_segment already holds the kexec relocation destination address and
size. Therefore, the prototype of kimage_map_segment() can be changed.
Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Alexander Graf <graf@amazon.com>
Cc: Steven Chen <chenste@linux.microsoft.com>
To: kexec@lists.infradead.org
To: linux-integrity@vger.kernel.org
---
include/linux/kexec.h | 4 ++--
kernel/kexec_core.c | 9 ++++++---
security/integrity/ima/ima_kexec.c | 4 +---
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index ff7e231b0485..8a22bc9b8c6c 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -530,7 +530,7 @@ extern bool kexec_file_dbg_print;
#define kexec_dprintk(fmt, arg...) \
do { if (kexec_file_dbg_print) pr_info(fmt, ##arg); } while (0)
-extern void *kimage_map_segment(struct kimage *image, unsigned long addr, unsigned long size);
+extern void *kimage_map_segment(struct kimage *image, int idx);
extern void kimage_unmap_segment(void *buffer);
#else /* !CONFIG_KEXEC_CORE */
struct pt_regs;
@@ -540,7 +540,7 @@ static inline void __crash_kexec(struct pt_regs *regs) { }
static inline void crash_kexec(struct pt_regs *regs) { }
static inline int kexec_should_crash(struct task_struct *p) { return 0; }
static inline int kexec_crash_loaded(void) { return 0; }
-static inline void *kimage_map_segment(struct kimage *image, unsigned long addr, unsigned long size)
+static inline void *kimage_map_segment(struct kimage *image, int idx)
{ return NULL; }
static inline void kimage_unmap_segment(void *buffer) { }
#define kexec_in_progress false
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index fa00b239c5d9..9a1966207041 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -960,17 +960,20 @@ int kimage_load_segment(struct kimage *image, int idx)
return result;
}
-void *kimage_map_segment(struct kimage *image,
- unsigned long addr, unsigned long size)
+void *kimage_map_segment(struct kimage *image, int idx)
{
+ unsigned long addr, size, eaddr;
unsigned long src_page_addr, dest_page_addr = 0;
- unsigned long eaddr = addr + size;
kimage_entry_t *ptr, entry;
struct page **src_pages;
unsigned int npages;
void *vaddr = NULL;
int i;
+ addr = image->segment[idx].mem;
+ size = image->segment[idx].memsz;
+ eaddr = addr + size;
+
/*
* Collect the source pages and map them in a contiguous VA range.
*/
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 7362f68f2d8b..5beb69edd12f 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -250,9 +250,7 @@ void ima_kexec_post_load(struct kimage *image)
if (!image->ima_buffer_addr)
return;
- ima_kexec_buffer = kimage_map_segment(image,
- image->ima_buffer_addr,
- image->ima_buffer_size);
+ ima_kexec_buffer = kimage_map_segment(image, image->ima_segment_index);
if (!ima_kexec_buffer) {
pr_err("Could not map measurements buffer.\n");
return;
--
2.49.0
^ permalink raw reply related
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: Bagas Sanjaya @ 2025-11-05 13:04 UTC (permalink / raw)
To: James Bottomley, Linux Kernel Mailing List, Linux Documentation,
Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <a8a5b95e06e2d5d1c04aab8933f25cd07903a3e8.camel@HansenPartnership.com>
[-- Attachment #1: Type: text/plain, Size: 856 bytes --]
On Tue, Nov 04, 2025 at 10:32:50PM -0500, James Bottomley wrote:
> On Wed, 2025-11-05 at 08:57 +0700, Bagas Sanjaya wrote:
> > On Tue, Nov 04, 2025 at 09:55:08AM -0500, James Bottomley wrote:
> > > On Tue, 2025-11-04 at 20:13 +0700, Bagas Sanjaya wrote:
> > > > The last section heading in TPM security docs is formatted as
> > > > title heading instead. As such, it shows up as TPM toctree entry.
> > > > Demote it to section heading as appropriate.
> > >
> > > It's supposed to be a separate heading. It's explaining how to
> > > certify your booted kernel rather than describing TPM security
> > > within the kernel.
> >
> > Should I keep the whole section as-is or should I move it to separate
> > docs?
>
> Why might it need moving?
Just to tidy up toctree then...
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: James Bottomley @ 2025-11-05 3:32 UTC (permalink / raw)
To: Bagas Sanjaya, Linux Kernel Mailing List, Linux Documentation,
Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <aQqvEsdoj0El2Dq4@archie.me>
[-- Attachment #1: Type: text/plain, Size: 684 bytes --]
On Wed, 2025-11-05 at 08:57 +0700, Bagas Sanjaya wrote:
> On Tue, Nov 04, 2025 at 09:55:08AM -0500, James Bottomley wrote:
> > On Tue, 2025-11-04 at 20:13 +0700, Bagas Sanjaya wrote:
> > > The last section heading in TPM security docs is formatted as
> > > title heading instead. As such, it shows up as TPM toctree entry.
> > > Demote it to section heading as appropriate.
> >
> > It's supposed to be a separate heading. It's explaining how to
> > certify your booted kernel rather than describing TPM security
> > within the kernel.
>
> Should I keep the whole section as-is or should I move it to separate
> docs?
Why might it need moving?
Regards,
James
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Paul Moore @ 2025-11-05 2:47 UTC (permalink / raw)
To: Coiby Xu
Cc: Mimi Zohar, linux-integrity, linux-security-module, Karel Srot,
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: <fftfj4o3kqxmfu3hb655xczqcddoeqjv55llsnwkrdu5isdm4z@6sqe3k24a6kk>
On Tue, Nov 4, 2025 at 7:19 PM Coiby Xu <coxu@redhat.com> wrote:
> On Sun, Nov 02, 2025 at 10:43:04AM -0500, Paul Moore wrote:
> >On Sun, Nov 2, 2025 at 10:06 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >> On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote:
> >> > On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
> >> > >
> >> > > 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(-)
> >> >
> >> > We don't really need a new LSM hook for this do we? Can't we just
> >> > define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
> >> > another call to security_kernel_post_read_file() after the module is
> >> > unpacked? Something like the snippet below ...
> >>
> >> Yes, this is similar to my suggestion based on defining multiple enumerations:
> >> READING_MODULE, READING_COMPRESSED_MODULE, and READING_DECOMPRESSED_MODULE.
> >> With this solution, IMA would need to make an exception in the post kernel
> >> module read for the READING_COMPRESSED_MODULE case, since the kernel module has
> >> not yet been decompressed.
> >>
> >> Coiby suggested further simplification by moving the call later. At which point
> >> either there is or isn't an appended signature for non-compressed and
> >> decompressed kernel modules.
> >>
> >> As long as you don't have a problem calling the security_kernel_post_read_file()
> >> hook again, could we move the call later and pass READING_MODULE_UNCOMPRESSED?
> >
> >It isn't clear from these comments if you are talking about moving
> >only the second security_kernel_post_read_file() call that was
> >proposed for init_module_from_file() to later in the function, leaving
> >the call in kernel_read_file() intact, or something else?
>
> Hi Paul and Mimi,
>
> Thanks for sharing your feedback! Yes, you are right, there is no need
> for a new LSM hook. Actually by not introducing a new LSM hook, we can
> have a much simpler solution!
>
> >
> >I think we want to leave the hook calls in kernel_read_file() intact,
> >in which case I'm not certain what advantage there is in moving the
> >security_kernel_post_read_file() call to a location where it is called
> >in init_module_from_file() regardless of if the module is compressed
> >or not. In the uncompressed case you are calling the hook twice for
> >no real benefit? It may be helpful to submit a patch with your
> >proposal as a patch can be worth a thousand words ;)
> >
> >
> >> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> >> > index c66b26184936..f127000d2e0a 100644
> >> > --- a/kernel/module/main.c
> >> > +++ b/kernel/module/main.c
> >> > @@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
> >> > ar __user * uargs, int
> >> > mod_stat_add_long(len, &invalid_decompress_bytes);
> >> > return err;
> >> > }
> >> > +
> >> > + err = security_kernel_post_read_file(f,
> >> > + (char *)info.hdr, info.len,
> >> > + READING_MODULE_DECOMPRESS);
> >> > + if (err) {
> >> > + mod_stat_inc(&failed_kreads);
> >> > + return err;
> >> > + }
> >> > } else {
> >> > info.hdr = buf;
> >> > info.len = len;
> >>
> >> == defer security_kernel_post_read_file() call to here ==
>
> By moving security_kernel_post_read_file, I think what Mimi means is to
> move security_kernel_post_read_file in init_module_from_file() to later
> in the function,
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362a..66725e53fef0c1 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,14 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
> info.len = len;
> }
>
> + err = security_kernel_post_read_file(f, (char *)info.hdr, info.len,
> + READING_MODULE);
> + if (err) {
> + mod_stat_inc(&failed_kreads);
> + free_copy(&info, flags);
> + return err;
> + }
> +
> return load_module(&info, uargs, flags);
> }
>
> If we only call security_kernel_post_read_file the 2nd time for a
> decompressed kernel module, IMA won't be sure what to do when
> security_kernel_post_read_file is called for the 1st time because it
> can't distinguish between a compressed module with appended signature or
> a uncompressed module without appended signature. If it permits 1st
> calling security_kernel_post_read_file, a uncompressed module without
> appended signature can be loaded. If it doesn't permit 1st calling
> security_kernel_post_read_file, there is no change to call
> security_kernel_post_read_file again for decompressed module.
>
> And you are right, there is no need to call
> security_kernel_post_read_file twice. And from the perspective of IMA,
> it simplifies reasoning if it is guaranteed that IMA will always access
> uncompressed kernel module regardless regardless of its original
> compression state.
>
> So I think a better solution is to stop calling
> security_kernel_post_read_file in kernel_read_file for READING_MODULE.
> This can also avoiding introducing an unnecessary
> READING_MODULE_UNCOMPRESSED/READING_COMPRESSED_MODULE enumeration and
> can make the solution even simpler,
>
> diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
> index de32c95d823dbd..7c78e84def6ec7 100644
> --- a/fs/kernel_read_file.c
> +++ b/fs/kernel_read_file.c
> @@ -107,7 +107,12 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf,
> goto out_free;
> }
>
> - ret = security_kernel_post_read_file(file, *buf, i_size, id);
> + /*
> + * security_kernel_post_read_file will be called later after
> + * a read kernel module is truly decompressed
> + */
> + if (id != READING_MODULE)
> + ret = security_kernel_post_read_file(file, *buf, i_size, id);
> }
Assuming I'm understanding the problem correctly, I think you're
making this harder than it needs to be. I believe something like this
should solve the problem without having to add more conditionals
around the hooks in kernel_read_file(), and limiting the multiple
security_kernel_post_read_file() calls to just the compressed case ...
and honestly in each of the _post_read_file() calls in the compressed
case, the buffer contents have changed so it somewhat makes sense.
Given the code below, IMA could simply ignore the
READING_MODULE_COMPRESSED case (or whatever it is the IMA needs to do
in that case) and focus on the READING_MODULE case as it does today.
I expect the associated IMA patch would be both trivial and small.
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..b435c498ec01 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3675,17 +3675,19 @@ static int idempotent_wait_for_completion(struct idempot
ent *u)
static int init_module_from_file(struct file *f, const char __user * uargs, int
flags)
{
+ bool compressed = !!(flags & MODULE_INIT_COMPRESSED_FILE);
struct load_info info = { };
void *buf = NULL;
int len;
- len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
+ len = kernel_read_file(f, 0, &buf, INT_MAX, NULL,
+ compressed ? READING_MODULE_COMPRESSED : READING_
MODULE);
if (len < 0) {
mod_stat_inc(&failed_kreads);
return len;
}
- if (flags & MODULE_INIT_COMPRESSED_FILE) {
+ if (compressed) {
int err = module_decompress(&info, buf, len);
vfree(buf); /* compressed data is no longer needed */
if (err) {
@@ -3693,6 +3695,14 @@ static int init_module_from_file(struct file *f, const ch
ar __user * uargs, int
mod_stat_add_long(len, &invalid_decompress_bytes);
return err;
}
+
+ err = security_kernel_post_read_file(f,
+ (char *)info.hdr, info.len,
+ READING_MODULE);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ return err;
+ }
} else {
info.hdr = buf;
info.len = len;
--
paul-moore.com
^ permalink raw reply related
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: Bagas Sanjaya @ 2025-11-05 1:57 UTC (permalink / raw)
To: James Bottomley, Linux Kernel Mailing List, Linux Documentation,
Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <50acd6bfbc8b9006bef5d7d0376b7ce4ab35f94c.camel@HansenPartnership.com>
[-- Attachment #1: Type: text/plain, Size: 625 bytes --]
On Tue, Nov 04, 2025 at 09:55:08AM -0500, James Bottomley wrote:
> On Tue, 2025-11-04 at 20:13 +0700, Bagas Sanjaya wrote:
> > The last section heading in TPM security docs is formatted as title
> > heading instead. As such, it shows up as TPM toctree entry. Demote it
> > to section heading as appropriate.
>
> It's supposed to be a separate heading. It's explaining how to certify
> your booted kernel rather than describing TPM security within the
> kernel.
Should I keep the whole section as-is or should I move it to separate docs?
Thanks.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Coiby Xu @ 2025-11-05 0:18 UTC (permalink / raw)
To: Paul Moore, Mimi Zohar
Cc: linux-integrity, linux-security-module, Karel Srot, 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: <CAHC9VhToe-VNqbh6TY2iYnRvqTHRfQjnHYSRWYgt8K7NcLKMdg@mail.gmail.com>
On Sun, Nov 02, 2025 at 10:43:04AM -0500, Paul Moore wrote:
>On Sun, Nov 2, 2025 at 10:06 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>> On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote:
>> > On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
>> > >
>> > > 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(-)
>> >
>> > We don't really need a new LSM hook for this do we? Can't we just
>> > define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
>> > another call to security_kernel_post_read_file() after the module is
>> > unpacked? Something like the snippet below ...
>>
>> Yes, this is similar to my suggestion based on defining multiple enumerations:
>> READING_MODULE, READING_COMPRESSED_MODULE, and READING_DECOMPRESSED_MODULE.
>> With this solution, IMA would need to make an exception in the post kernel
>> module read for the READING_COMPRESSED_MODULE case, since the kernel module has
>> not yet been decompressed.
>>
>> Coiby suggested further simplification by moving the call later. At which point
>> either there is or isn't an appended signature for non-compressed and
>> decompressed kernel modules.
>>
>> As long as you don't have a problem calling the security_kernel_post_read_file()
>> hook again, could we move the call later and pass READING_MODULE_UNCOMPRESSED?
>
>It isn't clear from these comments if you are talking about moving
>only the second security_kernel_post_read_file() call that was
>proposed for init_module_from_file() to later in the function, leaving
>the call in kernel_read_file() intact, or something else?
Hi Paul and Mimi,
Thanks for sharing your feedback! Yes, you are right, there is no need
for a new LSM hook. Actually by not introducing a new LSM hook, we can
have a much simpler solution!
>
>I think we want to leave the hook calls in kernel_read_file() intact,
>in which case I'm not certain what advantage there is in moving the
>security_kernel_post_read_file() call to a location where it is called
>in init_module_from_file() regardless of if the module is compressed
>or not. In the uncompressed case you are calling the hook twice for
>no real benefit? It may be helpful to submit a patch with your
>proposal as a patch can be worth a thousand words ;)
>
>
>> > diff --git a/kernel/module/main.c b/kernel/module/main.c
>> > index c66b26184936..f127000d2e0a 100644
>> > --- a/kernel/module/main.c
>> > +++ b/kernel/module/main.c
>> > @@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
>> > ar __user * uargs, int
>> > mod_stat_add_long(len, &invalid_decompress_bytes);
>> > return err;
>> > }
>> > +
>> > + err = security_kernel_post_read_file(f,
>> > + (char *)info.hdr, info.len,
>> > + READING_MODULE_DECOMPRESS);
>> > + if (err) {
>> > + mod_stat_inc(&failed_kreads);
>> > + return err;
>> > + }
>> > } else {
>> > info.hdr = buf;
>> > info.len = len;
>>
>> == defer security_kernel_post_read_file() call to here ==
By moving security_kernel_post_read_file, I think what Mimi means is to
move security_kernel_post_read_file in init_module_from_file() to later
in the function,
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b261849362a..66725e53fef0c1 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,14 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
info.len = len;
}
+ err = security_kernel_post_read_file(f, (char *)info.hdr, info.len,
+ READING_MODULE);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ free_copy(&info, flags);
+ return err;
+ }
+
return load_module(&info, uargs, flags);
}
If we only call security_kernel_post_read_file the 2nd time for a
decompressed kernel module, IMA won't be sure what to do when
security_kernel_post_read_file is called for the 1st time because it
can't distinguish between a compressed module with appended signature or
a uncompressed module without appended signature. If it permits 1st
calling security_kernel_post_read_file, a uncompressed module without
appended signature can be loaded. If it doesn't permit 1st calling
security_kernel_post_read_file, there is no change to call
security_kernel_post_read_file again for decompressed module.
And you are right, there is no need to call
security_kernel_post_read_file twice. And from the perspective of IMA,
it simplifies reasoning if it is guaranteed that IMA will always access
uncompressed kernel module regardless regardless of its original
compression state.
So I think a better solution is to stop calling
security_kernel_post_read_file in kernel_read_file for READING_MODULE.
This can also avoiding introducing an unnecessary
READING_MODULE_UNCOMPRESSED/READING_COMPRESSED_MODULE enumeration and
can make the solution even simpler,
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index de32c95d823dbd..7c78e84def6ec7 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -107,7 +107,12 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf,
goto out_free;
}
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ /*
+ * security_kernel_post_read_file will be called later after
+ * a read kernel module is truly decompressed
+ */
+ if (id != READING_MODULE)
+ ret = security_kernel_post_read_file(file, *buf, i_size, id);
}
Btw, I notice IMA is the only user of security_kernel_post_read_file so
this change won't affect other LSMs. For a full patch, please visit
https://github.com/coiby/linux/commit/558d85779ab5d794874749ecfae0e48b890bf3e0.patch
If there are concerns that I'm unaware of and a new
READING_MODULE_UNCOMPRESSED/READING_COMPRESSED_MODULE enumeration is
necessary, here's another patch
https://github.com/coiby/linux/commit/cdd40317b6070f48ec871c6a89428084f38ca083.patch
>
>--
>paul-moore.com
>
--
Best regards,
Coiby
^ permalink raw reply related
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: James Bottomley @ 2025-11-04 14:55 UTC (permalink / raw)
To: Bagas Sanjaya, Linux Kernel Mailing List, Linux Documentation,
Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <20251104131312.23791-1-bagasdotme@gmail.com>
On Tue, 2025-11-04 at 20:13 +0700, Bagas Sanjaya wrote:
> The last section heading in TPM security docs is formatted as title
> heading instead. As such, it shows up as TPM toctree entry. Demote it
> to section heading as appropriate.
It's supposed to be a separate heading. It's explaining how to certify
your booted kernel rather than describing TPM security within the
kernel.
Regards,
James
^ permalink raw reply
* [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: Bagas Sanjaya @ 2025-11-04 13:13 UTC (permalink / raw)
To: Linux Kernel Mailing List, Linux Documentation, Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet,
Bagas Sanjaya
The last section heading in TPM security docs is formatted as title
heading instead. As such, it shows up as TPM toctree entry. Demote it
to section heading as appropriate.
Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
Documentation/security/tpm/tpm-security.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/security/tpm/tpm-security.rst b/Documentation/security/tpm/tpm-security.rst
index 4f633f2510336b..bf73bbe66db2fa 100644
--- a/Documentation/security/tpm/tpm-security.rst
+++ b/Documentation/security/tpm/tpm-security.rst
@@ -153,7 +153,7 @@ protect key sealing and parameter decryption to protect key unsealing
and random number generation.
Null Primary Key Certification in Userspace
-===========================================
+-------------------------------------------
Every TPM comes shipped with a couple of X.509 certificates for the
primary endorsement key. This document assumes that the Elliptic
base-commit: 27600b51fbc8b9a4eba18c8d88d7edb146605f3f
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply related
* Re: [PATCH] module: Only declare set_module_sig_enforced when CONFIG_MODULE_SIG=y
From: Coiby Xu @ 2025-11-04 3:05 UTC (permalink / raw)
To: Aaron Tomlin
Cc: linux-modules, linux-integrity, kernel test robot,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
open list:MODULE SUPPORT
In-Reply-To: <w4vqvjighjl33a32gvwnu7xlzmp6yztx42gbjixrhic3wt34j6@5flsq5omspn7>
On Fri, Oct 31, 2025 at 04:09:27PM -0400, Aaron Tomlin wrote:
>On Fri, Oct 31, 2025 at 04:09:48PM +0800, Coiby Xu wrote:
>> 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
>>
>>
>
>Looks good to me.
>
>Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
Hi Aaron,
Thanks for reviewing the patch!
>
>--
>Aaron Tomlin
>
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] module: Only declare set_module_sig_enforced when CONFIG_MODULE_SIG=y
From: Coiby Xu @ 2025-11-04 3:04 UTC (permalink / raw)
To: Daniel Gomez
Cc: linux-modules, linux-integrity, kernel test robot,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
open list:MODULE SUPPORT
In-Reply-To: <3bf85718-8cea-4982-944d-b4c7a4faaf8f@kernel.org>
On Sat, Nov 01, 2025 at 11:10:51PM +0100, Daniel Gomez wrote:
>On 31/10/2025 09.09, Coiby Xu wrote:
>> 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'
>
>It's a bit unclear whether you're referring to a current upstream issue (which I
>couldn't find as of -rc3), or if this is just a hypothetical scenario.
Hi Daniel,
Yes, this issue is hypothetical and currently doesn't cause any real
trouble. lkp found this issue in one of my proposed patches
https://lore.kernel.org/lkml/20250928030358.3873311-1-coxu@redhat.com/
But I'll use a different solution so the above patch will be abandoned
and will not be applied.
>
>>
>> So only declare set_module_sig_enforced when CONFIG_MODULE_SIG is
>> enabled.
>
>I only see cases where code has a safeguard like in
>security/integrity/ima/ima_efi.c:71
>
> if (IS_ENABLED(CONFIG_MODULE_SIG))
> set_module_sig_enforced();
>
>>
>> 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.
>
>I think the correct term we should use here is runtime safeguard. The code does
>not actually depend on that config, nor is there any dep in Kconfig.
Thanks for correcting me! Sorry I didn't realize the constant folding
compiler optimization and made a false claim while forgetting the fact
security/integrity/ima/ima_efi.c also explicitly use
"#if !IS_ENABLED(CONFIG_MODULE_SIG)".
>
>>
>> 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>
>
>
>Just minor nits regarding the commit message structure. This change should allow
>us to remove the safeguard from users of set_module_sig_enforced().
Thanks for the suggestion! Does the following commit address address you
concern?
module: Only declare set_module_sig_enforced when CONFIG_MODULE_SIG=y
Currently if set_module_sig_enforced is called with CONFIG_MODULE_SIG=n
e.g. [1], it can lead to a linking error,
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'
This happens because the actual implementation of
set_module_sig_enforced comes from CONFIG_MODULE_SIG but both the
function declaration and the empty stub definition are tied to
CONFIG_MODULES.
So bind set_module_sig_enforced to CONFIG_MODULE_SIG instead. This
allows (future) users to call set_module_sig_enforced directly without
the "if IS_ENABLED(CONFIG_MODULE_SIG)" safeguard.
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
use "if IS_ENABLED(CONFIG_MODULE_SIG)" safeguard.
[1] https://lore.kernel.org/lkml/20250928030358.3873311-1-coxu@redhat.com/
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202510030029.VRKgik99-lkp@intel.com/
>
>
>Other than that, LGTM,
>
>Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
Thanks for reviewing the patch!
>
--
Best regards,
Coiby
^ permalink raw reply
* tpm2h 0.15.0
From: Jarkko Sakkinen @ 2025-11-04 1:46 UTC (permalink / raw)
To: tpm2; +Cc: linux-integrity, keyrings
tpm2sh 0.15.0 is much more stabilized than previous iterations, and
enhances key management with a proper support for compiling command
lists from policy expressions embedded to the TPM 2.0 ASN.1 key
files:
~ main ≡
❯ tpm2sh create tpm:81000001 --data deadbeef --policy '(pcr(sha256:16) or pcr(sha256:7)) and secret(tpm:81000001)' keyedhash:sha256 | tpm2sh load
vtpm:80000000
~ main ≡
❯ tpm2sh cache
HANDLE TYPE DETAILS
80000000 transient keyedhash:sha256
~ main ≡
❯ tpm2sh unseal vtpm:80000000
deadbeef
RustCrypto crates have been erased and all software crypto is based
on openssl crate and libssl in order to have a patchable crypto:
❯ ldd target/release/tpm2sh
linux-vdso.so.1 (0x00007f4eddc64000)
libssl.so.3 => /lib/x86_64-linux-gnu/libssl.so.3 (0x00007f4edd757000)
libcrypto.so.3 => /lib/x86_64-linux-gnu/libcrypto.so.3 (0x00007f4edd2d0000)
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f4eddc0a000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f4edd1f0000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4edd00e000)
/lib64/ld-linux-x86-64.so.2 (0x00007f4eddc66000)
Total size of dependency graph is 129, which is not whole a lot in the
usual Rust metrics and tpm2sh is fine-tuned to compile nicely with rustc
1.7x toolchains. This allows to compile it fluently to e.g., BuildRoot
and Yocto images. E.g., to keep some control and narrow down
dependencies I wrote my own custom PKCS#1, PCKS#8, SEC1 and X.509
parsers using rasn [1].
Converting external keys to TPM keys is super trivial:
~ main ≡
❯ tpm2sh create-primary ecc-nist-p256:sha256
vtpm:80000000
~ main ≡
❯ tpm2sh cache
HANDLE TYPE DETAILS
80000000 transient ecc-nist-p256:sha256
~ main ≡
❯ tpm2sh convert vtpm:80000000 -I private.pem | tpm2sh load
vtpm:80000001
~ main ≡
❯ tpm2sh cache
HANDLE TYPE DETAILS
80000000 transient ecc-nist-p256:sha256
80000001 transient rsa-2048:sha256
`tpm2-tpmkey` crate reads and writes otherwise the format following the
standard, except it adds an optional `parentPubkey` attribute, which
enable parent auto-discovery from persistent handles and vtpm cache
for the tpm2sh load subcommand.
The custom (and stripped off) X.509 parser allows to trivially download
EC certificates:
~ main ≡
❯ tpm2sh memory
HANDLE TYPE DETAILS
01c00002 certificate rsa-2048:sha256
01c0000a certificate ecc-nist-p256:sha256
81000001 persistent rsa-2048:sha256
81000002 persistent ecc-nist-p256:sha256
~ main ≡
❯ tpm2sh memory tpm:01c0000a | openssl x509 -text -noout | head -15
Certificate:
Data:
Version: 3 (0x2)
Serial Number: 1298017026 (0x4d5e2b02)
Signature Algorithm: ecdsa-with-SHA256
Issuer: C = DE, O = Infineon Technologies AG, OU = OPTIGA(TM), CN = Infineon OPTIGA(TM) TPM 2.0 ECC CA 042
Validity
Not Before: Sep 16 22:18:38 2020 GMT
Not After : Sep 16 22:18:38 2035 GMT
Subject:
Subject Public Key Info:
Public Key Algorithm: id-ecPublicKey
Public-Key: (256 bit)
pub:
04:a5:09:12:cd:a6:0d:79:49:2f:b0:fa:39:bf:cf:
The stack overall has grown into a micro-ecosystem of re-usable components:
1. https://crates.io/crates/tpm2sh
3. https://crates.io/crates/tpm2-tpmkey
2. https://crates.io/crates/tpm2-policy-language
4. https://crates.io/crates/tpm2-crypto
5. https://crates.io/crates/tpm2-protocol
[1] https://github.com/librasn/compiler
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: Jonathan McDowell @ 2025-11-03 18:38 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Roberto Sassu, Peter Huewe, Jason Gunthorpe, Linus Torvalds,
James Bottomley, linux-integrity, linux-kernel, zohar
In-Reply-To: <aP_KT0GiQSzt1ClO@kernel.org>
On Mon, Oct 27, 2025 at 09:38:55PM +0200, Jarkko Sakkinen wrote:
>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.
Kernel access is exactly what caused the issue for me, in particular the
HW RNG access during a firmware upgrade. My patch to be able to disable
the HW RNG at runtime has landed in -next, which helps a lot, but it
really would be nice to be able to say "Hands off, I'm busy with this",
which is what led to this patch set.
To James' query about the fact the upgrade process should be properly
handled, I think the issue is probably that the HMAC context saving
around HW RNG access hit errors that were not gracefully handled, and we
marked the TPM as disabled in tpm2_load_null, causing failure
mid-upgrade.
J.
--
What have you got in your pocket?
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Paul Moore @ 2025-11-02 15:43 UTC (permalink / raw)
To: Mimi Zohar
Cc: Coiby Xu, linux-integrity, linux-security-module, Karel Srot,
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: <baa39fcd1b6b485f14b8f06dcd96b81359e6e491.camel@linux.ibm.com>
On Sun, Nov 2, 2025 at 10:06 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote:
> > On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
> > >
> > > 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(-)
> >
> > We don't really need a new LSM hook for this do we? Can't we just
> > define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
> > another call to security_kernel_post_read_file() after the module is
> > unpacked? Something like the snippet below ...
>
> Yes, this is similar to my suggestion based on defining multiple enumerations:
> READING_MODULE, READING_COMPRESSED_MODULE, and READING_DECOMPRESSED_MODULE.
> With this solution, IMA would need to make an exception in the post kernel
> module read for the READING_COMPRESSED_MODULE case, since the kernel module has
> not yet been decompressed.
>
> Coiby suggested further simplification by moving the call later. At which point
> either there is or isn't an appended signature for non-compressed and
> decompressed kernel modules.
>
> As long as you don't have a problem calling the security_kernel_post_read_file()
> hook again, could we move the call later and pass READING_MODULE_UNCOMPRESSED?
It isn't clear from these comments if you are talking about moving
only the second security_kernel_post_read_file() call that was
proposed for init_module_from_file() to later in the function, leaving
the call in kernel_read_file() intact, or something else?
I think we want to leave the hook calls in kernel_read_file() intact,
in which case I'm not certain what advantage there is in moving the
security_kernel_post_read_file() call to a location where it is called
in init_module_from_file() regardless of if the module is compressed
or not. In the uncompressed case you are calling the hook twice for
no real benefit? It may be helpful to submit a patch with your
proposal as a patch can be worth a thousand words ;)
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index c66b26184936..f127000d2e0a 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
> > ar __user * uargs, int
> > mod_stat_add_long(len, &invalid_decompress_bytes);
> > return err;
> > }
> > +
> > + err = security_kernel_post_read_file(f,
> > + (char *)info.hdr, info.len,
> > + READING_MODULE_DECOMPRESS);
> > + if (err) {
> > + mod_stat_inc(&failed_kreads);
> > + return err;
> > + }
> > } else {
> > info.hdr = buf;
> > info.len = len;
>
> == defer security_kernel_post_read_file() call to here ==
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-02 15:05 UTC (permalink / raw)
To: Paul Moore, Coiby Xu
Cc: linux-integrity, linux-security-module, Karel Srot, 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: <CAHC9VhRBXkW+XuqhxJvEOYR_VMxFh4TRWUtXzZky=AG_nyBYEQ@mail.gmail.com>
On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote:
> On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
> >
> > 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(-)
>
> We don't really need a new LSM hook for this do we? Can't we just
> define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
> another call to security_kernel_post_read_file() after the module is
> unpacked? Something like the snippet below ...
Yes, this is similar to my suggestion based on defining multiple enumerations:
READING_MODULE, READING_COMPRESSED_MODULE, and READING_DECOMPRESSED_MODULE.
With this solution, IMA would need to make an exception in the post kernel
module read for the READING_COMPRESSED_MODULE case, since the kernel module has
not yet been decompressed.
Coiby suggested further simplification by moving the call later. At which point
either there is or isn't an appended signature for non-compressed and
decompressed kernel modules.
As long as you don't have a problem calling the security_kernel_post_read_file()
hook again, could we move the call later and pass READING_MODULE_UNCOMPRESSED?
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..f127000d2e0a 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
> ar __user * uargs, int
> mod_stat_add_long(len, &invalid_decompress_bytes);
> return err;
> }
> +
> + err = security_kernel_post_read_file(f,
> + (char *)info.hdr, info.len,
> + READING_MODULE_DECOMPRESS);
> + if (err) {
> + mod_stat_inc(&failed_kreads);
> + return err;
> + }
> } else {
> info.hdr = buf;
> info.len = len;
== defer security_kernel_post_read_file() call to here ==
Mimi
^ permalink raw reply
* Re: [PATCH] module: Only declare set_module_sig_enforced when CONFIG_MODULE_SIG=y
From: Daniel Gomez @ 2025-11-01 22:10 UTC (permalink / raw)
To: Coiby Xu, linux-modules
Cc: linux-integrity, kernel test robot, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, open list:MODULE SUPPORT
In-Reply-To: <20251031080949.2001716-1-coxu@redhat.com>
On 31/10/2025 09.09, Coiby Xu wrote:
> 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'
It's a bit unclear whether you're referring to a current upstream issue (which I
couldn't find as of -rc3), or if this is just a hypothetical scenario.
>
> So only declare set_module_sig_enforced when CONFIG_MODULE_SIG is
> enabled.
I only see cases where code has a safeguard like in
security/integrity/ima/ima_efi.c:71
if (IS_ENABLED(CONFIG_MODULE_SIG))
set_module_sig_enforced();
>
> 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.
I think the correct term we should use here is runtime safeguard. The code does
not actually depend on that config, nor is there any dep in Kconfig.
>
> 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>
Just minor nits regarding the commit message structure. This change should allow
us to remove the safeguard from users of set_module_sig_enforced().
Other than that, LGTM,
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Paul Moore @ 2025-11-01 16:50 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, linux-security-module, Mimi Zohar, Karel Srot,
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: <20251031074016.1975356-1-coxu@redhat.com>
On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
>
> 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(-)
We don't really need a new LSM hook for this do we? Can't we just
define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
another call to security_kernel_post_read_file() after the module is
unpacked? Something like the snippet below ...
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..f127000d2e0a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
ar __user * uargs, int
mod_stat_add_long(len, &invalid_decompress_bytes);
return err;
}
+
+ err = security_kernel_post_read_file(f,
+ (char *)info.hdr, info.len,
+ READING_MODULE_DECOMPRESS);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ return err;
+ }
} else {
info.hdr = buf;
info.len = len;
--
paul-moore.com
^ permalink raw reply related
* Re: [PATCH] KEYS: Remove the ad-hoc compilation flag CAAM_DEBUG
From: Serge E. Hallyn @ 2025-10-31 20:41 UTC (permalink / raw)
To: Ye Bin
Cc: a.fatoum, kernel, James.Bottomley, jarkko, zohar, dhowells, paul,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
yebin10
In-Reply-To: <20251028132254.841715-1-yebin@huaweicloud.com>
On Tue, Oct 28, 2025 at 09:22:54PM +0800, Ye Bin wrote:
> 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>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
(I guess this is only in linux-next right now?)
> ---
> 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
* Re: [PATCH] KEYS: encrypted: Return early on allocation failure and drop goto
From: Serge E. Hallyn @ 2025-10-31 20:32 UTC (permalink / raw)
To: 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: <20251029163157.119000-1-thorsten.blum@linux.dev>
On Wed, Oct 29, 2025 at 05:31:56PM +0100, Thorsten Blum wrote:
> 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>
It seems like purely personal preference, but I don't see any
error in it, so in that sense
Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
> 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
* Re: [PATCH] module: Only declare set_module_sig_enforced when CONFIG_MODULE_SIG=y
From: Aaron Tomlin @ 2025-10-31 20:09 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-modules, linux-integrity, kernel test robot,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
open list:MODULE SUPPORT
In-Reply-To: <20251031080949.2001716-1-coxu@redhat.com>
On Fri, Oct 31, 2025 at 04:09:48PM +0800, Coiby Xu wrote:
> 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
>
>
Looks good to me.
Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
--
Aaron Tomlin
^ 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