[PATCH v2] lsm, ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
Paul Moore
paul at paul-moore.com
Wed Nov 5 02:47:48 UTC 2025
On Tue, Nov 4, 2025 at 7:19 PM Coiby Xu <coxu at 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 at 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 at 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 at redhat.com>
> >> > > Suggested-by: Mimi Zohar <zohar at linux.ibm.com>
> >> > > Signed-off-by: Coiby Xu <coxu at 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
More information about the Linux-security-module-archive
mailing list