On Wed, 2025-11-05 at 10:42 -0500, Paul Moore wrote: > On Wed, Nov 5, 2025 at 9:07 AM Mimi Zohar <[email protected]> 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
