Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module

[Mimi Zohar <zohar@linux.ibm.com>]

On Wed, 2025-11-05 at 08:18 +0800, Coiby Xu 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);
>   	}
> 
> 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

Hi Coiby,

Based on the conversation with Paul, there is no reason to remove the existing
security_kernel_post_read_file() call.

The changes are similar to the 2nd link, but a bit different.
- Define a single enumeration named READING_MODULE_COMPRESSED.

- In module/main.c add a new security_kernel_post_read_file() call immediately
after decompressing the kernel module.  Like a previous version of this patch,
call kernel_read_file() with either READING_MODULE or READING_MODULE_COMPRESSED
based on MODULE_INIT_COMPRESSED_FILE.

- In ima_post_read_file() defer verifying the signature when the enumeration is
READING_MODULE_COMPRESSED.  (No need for a new function ima_read_kernel_module.)

thanks,

Mimi