Message-ID: <CAHC9VhRGwXvhU64Nk5jdmtPfrt9bbkzpLVqS0LRbtN3Q3HhnCw@mail.gmail.com>
Date: Tue, 4 Nov 2025 21:47:48 -0500
From: Paul Moore <paul@...l-moore.com>
To: Coiby Xu <coxu@...hat.com>
Cc: Mimi Zohar <zohar@...ux.ibm.com>, linux-integrity@...r.kernel.org, 
	linux-security-module@...r.kernel.org, Karel Srot <ksrot@...hat.com>, 
	James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, 
	Luis Chamberlain <mcgrof@...nel.org>, Petr Pavlu <petr.pavlu@...e.com>, 
	Daniel Gomez <da.gomez@...nel.org>, Sami Tolvanen <samitolvanen@...gle.com>, 
	Roberto Sassu <roberto.sassu@...wei.com>, Dmitry Kasatkin <dmitry.kasatkin@...il.com>, 
	Eric Snowberg <eric.snowberg@...cle.com>, open list <linux-kernel@...r.kernel.org>, 
	"open list:MODULE SUPPORT" <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file
 to access decompressed kernel module

On Tue, Nov 4, 2025 at 7:19 PM Coiby Xu <coxu@...hat.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@...ux.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@...hat.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@...hat.com>
> >> > > Suggested-by: Mimi Zohar <zohar@...ux.ibm.com>
> >> > > Signed-off-by: Coiby Xu <coxu@...hat.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

Powered by blists - more mailing lists