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 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