| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <84a0e1785c7f0ff816b3246be49012092ae12126.camel@linux.ibm.com>
Date: Wed, 05 Nov 2025 15:47:25 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Coiby Xu <coxu@...hat.com>, Paul Moore <paul@...l-moore.com>
Cc: 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 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@...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);
> }
>
> 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
Powered by blists - more mailing lists