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 <[email protected]> wrote: > > > On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote: > > > > On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <[email protected]> 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 <[email protected]> > > > > > Suggested-by: Mimi Zohar <[email protected]> > > > > > Signed-off-by: Coiby Xu <[email protected]> > > > > > --- > > > > > v1: > > > > > https://lore.kernel.org/linux-integrity/[email protected]/ > > > > > > > > > > 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
