* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: kernel test robot @ 2025-11-07 6:54 UTC (permalink / raw)
To: Tahera Fahimi, zohar, roberto.sassu, dmitry.kasatkin,
eric.snowberg, paul, jmorris, serge, linux-integrity,
linux-security-module, linux-kernel, code
Cc: oe-kbuild-all, Tahera Fahimi
In-Reply-To: <20251106181404.3429710-1-taherafahimi@linux.microsoft.com>
Hi Tahera,
kernel test robot noticed the following build warnings:
[auto build test WARNING on zohar-integrity/next-integrity]
[also build test WARNING on linus/master v6.18-rc4 next-20251107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tahera-Fahimi/ima-avoid-duplicate-policy-rules-insertions/20251107-021615
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20251106181404.3429710-1-taherafahimi%40linux.microsoft.com
patch subject: [Patch V1] ima: avoid duplicate policy rules insertions
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20251107/202511071406.hU1UdCKh-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511071406.hU1UdCKh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511071406.hU1UdCKh-lkp@intel.com/
All warnings (new ones prefixed by >>):
security/integrity/ima/ima_policy.c: In function 'template_has_field':
>> security/integrity/ima/ima_policy.c:1958:13: warning: unused variable 'j' [-Wunused-variable]
1958 | int j;
| ^
--
>> Warning: security/integrity/ima/ima_policy.c:2078 function parameter 'new_rule' not described in 'ima_rule_exists'
vim +/j +1958 security/integrity/ima/ima_policy.c
1955
1956 static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
1957 {
> 1958 int j;
1959
1960 for (int j = 0; j < template2->num_fields; j++)
1961 if (strcmp(field_id, template2->fields[j]->field_id) == 0)
1962 return true;
1963
1964 return false;
1965 }
1966
1967 static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
1968 {
1969 int j;
1970
1971 for (j = 0; j < keyrings->count; j++) {
1972 if (strcmp(item, keyrings->items[j]) == 0)
1973 return true;
1974 }
1975 return false;
1976 }
1977
1978 static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
1979 {
1980 int j;
1981
1982 for (j = 0; j < labels->count; j++) {
1983 if (strcmp(item, labels->items[j]) == 0)
1984 return true;
1985 }
1986 return false;
1987 }
1988
1989 static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
1990 {
1991 int i;
1992
1993 if (rule1->flags != rule2->flags)
1994 return false;
1995
1996 if (rule1->action != rule2->action)
1997 return false;
1998
1999 if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
2000 ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
2001 ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
2002 ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
2003 ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
2004 ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
2005 ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
2006 ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
2007 ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
2008 ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
2009 ((rule1->flags & IMA_VALIDATE_ALGOS) &&
2010 rule1->allowed_algos != rule2->allowed_algos) ||
2011 ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
2012 ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
2013 return false;
2014
2015 if (!rule1->template && !rule2->template) {
2016 ;
2017 } else if (!rule1->template || !rule2->template) {
2018 return false;
2019 } else if (rule1->template->num_fields != rule2->template->num_fields) {
2020 return false;
2021 } else if (rule1->template->num_fields != 0) {
2022 for (i = 0; i < rule1->template->num_fields; i++) {
2023 if (!template_has_field(rule1->template->fields[i]->field_id,
2024 rule2->template))
2025 return false;
2026 }
2027 }
2028
2029 if (rule1->flags & IMA_KEYRINGS) {
2030 if (!rule1->keyrings && !rule2->keyrings) {
2031 ;
2032 } else if (!rule1->keyrings || !rule2->keyrings) {
2033 return false;
2034 } else if (rule1->keyrings->count != rule2->keyrings->count) {
2035 return false;
2036 } else if (rule1->keyrings->count != 0) {
2037 for (i = 0; i < rule1->keyrings->count; i++) {
2038 if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
2039 return false;
2040 }
2041 }
2042 }
2043
2044 if (rule1->flags & IMA_LABEL) {
2045 if (!rule1->label && !rule2->label) {
2046 ;
2047 } else if (!rule1->label || !rule2->label) {
2048 return false;
2049 } else if (rule1->label->count != rule2->label->count) {
2050 return false;
2051 } else if (rule1->label->count != 0) {
2052 for (i = 0; i < rule1->label->count; i++) {
2053 if (!labels_has_item(rule1->label->items[i], rule2->label))
2054 return false;
2055 }
2056 }
2057 }
2058
2059 for (i = 0; i < MAX_LSM_RULES; i++) {
2060 if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
2061 continue;
2062
2063 if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
2064 return false;
2065
2066 if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
2067 return false;
2068 }
2069
2070 return true;
2071 }
2072
2073 /**
2074 * ima_rule_exists - check if a rule already exists in the policy
2075 *
2076 * Checking both the active policy and the temporary rules list.
2077 */
> 2078 static bool ima_rule_exists(struct ima_rule_entry *new_rule)
2079 {
2080 struct ima_rule_entry *entry;
2081 struct list_head *ima_rules_tmp;
2082
2083 if (!list_empty(&ima_temp_rules)) {
2084 list_for_each_entry(entry, &ima_temp_rules, list) {
2085 if (ima_rules_equal(entry, new_rule))
2086 return true;
2087 }
2088 }
2089
2090 rcu_read_lock();
2091 ima_rules_tmp = rcu_dereference(ima_rules);
2092 list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
2093 if (ima_rules_equal(entry, new_rule)) {
2094 rcu_read_unlock();
2095 return true;
2096 }
2097 }
2098 rcu_read_unlock();
2099
2100 return false;
2101 }
2102
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-07 5:25 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <CAF+s44TOt+EwGi9VDES9PC+VaGZoDCw6rbyRv_mnb0xbaLScbg@mail.gmail.com>
On 11/07/25 at 01:13pm, Pingfan Liu wrote:
> On Fri, Nov 7, 2025 at 9:51 AM Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/06/25 at 06:01pm, Pingfan Liu wrote:
> > > On Thu, Nov 6, 2025 at 4:01 PM Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > On 11/06/25 at 02:59pm, Pingfan Liu wrote:
> > > > > When I tested kexec with the latest kernel, I ran into the following warning:
> > > > >
> > > > > [ 40.712410] ------------[ cut here ]------------
> > > > > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > > > > [...]
> > > > > [ 40.816047] Call trace:
> > > > > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > > > > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > > > > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > > > > [...]
> > > > > [ 40.855423] ---[ end trace 0000000000000000 ]---
> > > > >
> > > > > This is caused by the fact that kexec allocates the destination directly
> > > > > in the CMA area. In that case, the CMA kernel address should be exported
> > > > > directly to the IMA component, instead of using the vmalloc'd address.
> > > >
> > > > Well, you didn't update the log accordingly.
> > > >
> > >
> > > I am not sure what you mean. Do you mean the earlier content which I
> > > replied to you?
> >
> > No. In v1, you return cma directly. But in v2, you return its direct
> > mapping address, isnt' it?
> >
>
> Yes. But I think it is a fault in the code, which does not convey the
> expression in the commit log. Do you think I should rephrase the words
> "the CMA kernel address" as "the CMA kernel direct mapping address"?
That's fine to me.
>
> > >
> > > > Do you know why cma area can't be mapped into vmalloc?
> > > >
> > > Should not the kernel direct mapping be used?
> >
> > When image->segment_cma[i] has value, image->ima_buffer_addr also
> > contains the physical address of the cma area, why cma physical address
> > can't be mapped into vmalloc and cause the failure and call trace?
> >
>
> It could be done using the vmalloc approach, but it's unnecessary.
> IIUC, kimage_map_segment() was introduced to provide a contiguous
> virtual address for IMA access, since the IND_SRC pages are scattered
> throughout the kernel. However, in the CMA case, there is already a
> contiguous virtual address in the kernel direct mapping range.
> Normally, when we have a physical address, we simply use
> phys_to_virt() to get its corresponding kernel virtual address.
OK, I understand cma area is contiguous, and no need to map into
vmalloc. I am wondering why in the old code mapping cma addrss into
vmalloc cause the warning which you said is a IMA problem.
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-07 5:13 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <aQ1QiLGXWRsZbiYo@MiWiFi-R3L-srv>
On Fri, Nov 7, 2025 at 9:51 AM Baoquan He <bhe@redhat.com> wrote:
>
> On 11/06/25 at 06:01pm, Pingfan Liu wrote:
> > On Thu, Nov 6, 2025 at 4:01 PM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > On 11/06/25 at 02:59pm, Pingfan Liu wrote:
> > > > When I tested kexec with the latest kernel, I ran into the following warning:
> > > >
> > > > [ 40.712410] ------------[ cut here ]------------
> > > > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > > > [...]
> > > > [ 40.816047] Call trace:
> > > > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > > > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > > > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > > > [...]
> > > > [ 40.855423] ---[ end trace 0000000000000000 ]---
> > > >
> > > > This is caused by the fact that kexec allocates the destination directly
> > > > in the CMA area. In that case, the CMA kernel address should be exported
> > > > directly to the IMA component, instead of using the vmalloc'd address.
> > >
> > > Well, you didn't update the log accordingly.
> > >
> >
> > I am not sure what you mean. Do you mean the earlier content which I
> > replied to you?
>
> No. In v1, you return cma directly. But in v2, you return its direct
> mapping address, isnt' it?
>
Yes. But I think it is a fault in the code, which does not convey the
expression in the commit log. Do you think I should rephrase the words
"the CMA kernel address" as "the CMA kernel direct mapping address"?
> >
> > > Do you know why cma area can't be mapped into vmalloc?
> > >
> > Should not the kernel direct mapping be used?
>
> When image->segment_cma[i] has value, image->ima_buffer_addr also
> contains the physical address of the cma area, why cma physical address
> can't be mapped into vmalloc and cause the failure and call trace?
>
It could be done using the vmalloc approach, but it's unnecessary.
IIUC, kimage_map_segment() was introduced to provide a contiguous
virtual address for IMA access, since the IND_SRC pages are scattered
throughout the kernel. However, in the CMA case, there is already a
contiguous virtual address in the kernel direct mapping range.
Normally, when we have a physical address, we simply use
phys_to_virt() to get its corresponding kernel virtual address.
Thanks,
Pingfan
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-07 1:51 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <CAF+s44TyM7sBVmGn7kn5Cw+Ygm02F93hchiSBN0Q_qR=oA+DLg@mail.gmail.com>
On 11/06/25 at 06:01pm, Pingfan Liu wrote:
> On Thu, Nov 6, 2025 at 4:01 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/06/25 at 02:59pm, Pingfan Liu wrote:
> > > When I tested kexec with the latest kernel, I ran into the following warning:
> > >
> > > [ 40.712410] ------------[ cut here ]------------
> > > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > > [...]
> > > [ 40.816047] Call trace:
> > > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > > [...]
> > > [ 40.855423] ---[ end trace 0000000000000000 ]---
> > >
> > > This is caused by the fact that kexec allocates the destination directly
> > > in the CMA area. In that case, the CMA kernel address should be exported
> > > directly to the IMA component, instead of using the vmalloc'd address.
> >
> > Well, you didn't update the log accordingly.
> >
>
> I am not sure what you mean. Do you mean the earlier content which I
> replied to you?
No. In v1, you return cma directly. But in v2, you return its direct
mapping address, isnt' it?
>
> > Do you know why cma area can't be mapped into vmalloc?
> >
> Should not the kernel direct mapping be used?
When image->segment_cma[i] has value, image->ima_buffer_addr also
contains the physical address of the cma area, why cma physical address
can't be mapped into vmalloc and cause the failure and call trace?
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Andrew Morton @ 2025-11-07 0:44 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <CAF+s44Qoai54qUDqmVQoqWspneuuYF=SLYezoew_EHb_0By77w@mail.gmail.com>
On Thu, 6 Nov 2025 10:57:33 +0800 Pingfan Liu <piliu@redhat.com> wrote:
> > > This is caused by the fact that kexec allocates the destination directly
> > > in the CMA area. In that case, the CMA kernel address should be exported
> > > directly to the IMA component, instead of using the vmalloc'd address.
> >
> > This is something we should backport into tearlier kernels.
> >
> > > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Baoquan He <bhe@redhat.com>
> > > Cc: Alexander Graf <graf@amazon.com>
> > > Cc: Steven Chen <chenste@linux.microsoft.com>
> > > Cc: linux-integrity@vger.kernel.org
> > > To: kexec@lists.infradead.org
> >
> > So I'm thinking we should add
> >
> > Fixes: 0091d9241ea2 ("kexec: define functions to map and unmap segments")
> Should be:
> Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
>
> Because 07d24902977e came after 0091d9241ea2 and introduced this issue.
Thanks, I updated the mm.git copy of this patch.
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-06 22:15 UTC (permalink / raw)
To: Coiby Xu
Cc: Paul Moore, linux-integrity, linux-security-module, Karel Srot,
James Morris, Serge E. Hallyn, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, open list, open list:MODULE SUPPORT
In-Reply-To: <d24wnmefebnheerigmh6ts5yskkutz726l6a2f6g5s3s5fhhrv@osaactobwb5g>
On Thu, 2025-11-06 at 21:29 +0800, Coiby Xu wrote:
> On Wed, Nov 05, 2025 at 03:47:25PM -0500, Mimi Zohar wrote:
> > On Wed, 2025-11-05 at 08:18 +0800, Coiby Xu wrote:
> [...]
> >
> > 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.)
>
> Hi Mimi,
>
> Thanks for summarizing your conversation with Paul! I can confirm Paul's
> approach works
> https://github.com/coiby/linux/tree/in_kernel_decompression_ima_no_lsm_hook_paul
>
> While testing the patch today, I realized there is another
> issue/challenge introduced by in-kernel module decompression. IMA
> appraisal is to verify the digest of compressed kernel module but
> currently the passed buffer is uncompressed module. When IMA uses
> uncompressed module data to calculate the digest, xattr signature
> verification will fail. If we always make IMA read the original kernel
> module data again to calculate the digest, does it look like a
> quick-and-dirty fix? If we can assume people won't load kernel module so
> often, the performance impact is negligible. Otherwise we may have to
> introduce a new LSM hook so IMA can access uncompressed and original
> module data one time.
ima_collect_measurement() stores the file hash info in the iint and uses that
information to verify the signature as stored in the security xattr.
Decompressing the kernel module shouldn't affect the xattr signature
verification.
The patch with a few minor changes looks good:
- READDING_MODULE_CHECK -> READING_MODULE_CHECK
- Fix the enumeration name in ima_main.c
- scripts/checkpatch.pl code/comment line length has been relaxed to 100 chars,
but the section "Breaking long lines and strings" in
Documentation/process/coding-style.rst still recommends 80 characters.
There are cases where it is necessary to go over the 80 char line limit for
readability, but in general both Roberto and I prefer, as much as possible, to
limit the line length to 80 char. To detect where/when the line limit is
greater than 80 chars, use the scripts/checkpatch.pl "--max-line-length=80"
option.
After fixing the patch, please post it to linux-integrity mailing list.
--
thanks,
Mimi
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Anirudh Venkataramanan @ 2025-11-06 20:32 UTC (permalink / raw)
To: Tahera Fahimi, zohar, roberto.sassu, dmitry.kasatkin,
eric.snowberg, paul, jmorris, serge, linux-integrity,
linux-security-module, linux-kernel, code
In-Reply-To: <20251106181404.3429710-1-taherafahimi@linux.microsoft.com>
On 11/6/2025 10:14 AM, Tahera Fahimi wrote:
> Prevent redundant IMA policy rules by checking for duplicates before insertion. This ensures that
> rules are not re-added when userspace is restarted (using systemd-soft-reboot) without a full system
> reboot. ima_rule_exists() detects duplicates in both temporary and active rule lists.
I have run into this too. Thanks for proposing a patch!
FWIW - I am fairly new to the IMA subsystem, so feedback below is mostly
structural, with some IMA specific comments.
>
> Signed-off-by: Tahera Fahimi <taherafahimi@linux.microsoft.com>
> ---
> security/integrity/ima/ima_policy.c | 157 +++++++++++++++++++++++++++-
> 1 file changed, 156 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 164d62832f8ec..3dd902101dbda 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1953,6 +1953,153 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> return result;
> }
>
> +static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
> +{
> + int j;
j is declared in the loop header below too, which is more correct
because it keeps the scope of j to be within the loop. So I'd say get
rid of the above declaration.
> +
> + for (int j = 0; j < template2->num_fields; j++)
> + if (strcmp(field_id, template2->fields[j]->field_id) == 0)
> + return true;
I believe the preferred kernel style is to use if (!strcmp(...)).
> +
> + return false;
> +}
> +
> +static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
> +{
> + int j;
> +
> + for (j = 0; j < keyrings->count; j++) {
> + if (strcmp(item, keyrings->items[j]) == 0)
> + return true;
> + }
> + return false;
> +}
> +
> +static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
> +{
> + int j;
> +
> + for (j = 0; j < labels->count; j++) {
> + if (strcmp(item, labels->items[j]) == 0)
> + return true;
> + }
> + return false;
> +}
> +
> +static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
> +{
> + int i;
i is used further down in this function, and even in all those cases,
the scope of i can be limited to the loop body where it's used.
If you didn't know this already - you can use cppcheck to identify and
reduce the scope of variables.
> +
> + if (rule1->flags != rule2->flags)
> + return false;
> +
> + if (rule1->action != rule2->action)
> + return false;
> +
> + if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
> + ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
> + ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
> + ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
> + ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
> + ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
> + ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
> + ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
> + ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
> + ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
> + ((rule1->flags & IMA_VALIDATE_ALGOS) &&
> + rule1->allowed_algos != rule2->allowed_algos) ||
> + ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
> + ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
> + return false;
So the goal is to prevent the exact same policy rule from being added,
not to update an existing rule, correct? IOW, you could end up with two
very similar rules, because the new rule has one thing that's different
compared to the existing rule?
I feel that a little bit of commentary around what makes two rules the
same would be useful.
> +
> + if (!rule1->template && !rule2->template) {
> + ;
You're trying to do nothing and continue on. A goto statement would
communicate intent better. There are other places below with the same
noop structure.
To be fair, I also don't completely understand what you're trying to
achieve here, Regardless, this "do nothing inside a conditional" looks
weird and I feel like there should be a way to structure your logic
without resorting to this.
> + } else if (!rule1->template || !rule2->template) {
> + return false;
> + } else if (rule1->template->num_fields != rule2->template->num_fields) {
> + return false;
> + } else if (rule1->template->num_fields != 0) {
> + for (i = 0; i < rule1->template->num_fields; i++) {
> + if (!template_has_field(rule1->template->fields[i]->field_id,
> + rule2->template))
> + return false;
> + }
> + }
if + return will achieve the same end goals as else if + return, with
lesser clutter. I have seen some static analyzers flag this pattern, but
I can't remember which one at the moment.
So something like this:
if (!rule1->template && !rule2->template)
goto some_target;
if (!rule1->template || !rule2->template)
return false;
if (rule1->template->num_fields != rule2->template->num_fields)
return false;
if (rule1->template->num_fields != 0) {
for (i = 0; i < rule1->template->num_fields; i++) {
if (!template_has_field(rule1->template->fields[i]->field_id,
rule2->template))
return false;
}
}
some_target:
...
...
> +
> + if (rule1->flags & IMA_KEYRINGS) {
> + if (!rule1->keyrings && !rule2->keyrings) {
> + ;
Another if block no-op
> + } else if (!rule1->keyrings || !rule2->keyrings) {
> + return false;
> + } else if (rule1->keyrings->count != rule2->keyrings->count) {
> + return false;
> + } else if (rule1->keyrings->count != 0) {
if (rule1->keyrings->count)
> + for (i = 0; i < rule1->keyrings->count; i++) {
for (int i,
> + if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
> + return false;
> + }
> + }
> + }
> +
> + if (rule1->flags & IMA_LABEL) {
> + if (!rule1->label && !rule2->label) {
> + ;
Another if block no-op
> + } else if (!rule1->label || !rule2->label) {
> + return false;
> + } else if (rule1->label->count != rule2->label->count) {
> + return false;
> + } else if (rule1->label->count != 0) {
> + for (i = 0; i < rule1->label->count; i++) {
> + if (!labels_has_item(rule1->label->items[i], rule2->label))
> + return false;
> + }
> + }
> + }
> +
> + for (i = 0; i < MAX_LSM_RULES; i++) {
for (int i,
> + if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
> + continue;
> +
> + if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
> + return false;
> +
> + if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * ima_rule_exists - check if a rule already exists in the policy
> + *
> + * Checking both the active policy and the temporary rules list.
> + */
> +static bool ima_rule_exists(struct ima_rule_entry *new_rule)
> +{
> + struct ima_rule_entry *entry;
> + struct list_head *ima_rules_tmp;
> +
> + if (!list_empty(&ima_temp_rules)) {
> + list_for_each_entry(entry, &ima_temp_rules, list) {
> + if (ima_rules_equal(entry, new_rule))
> + return true;
> + }
> + }
> +
> + rcu_read_lock();
> + ima_rules_tmp = rcu_dereference(ima_rules);
> + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
> + if (ima_rules_equal(entry, new_rule)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> /**
> * ima_parse_add_rule - add a rule to ima_policy_rules
> * @rule: ima measurement policy rule
> @@ -1993,7 +2140,15 @@ ssize_t ima_parse_add_rule(char *rule)
> return result;
> }
>
> - list_add_tail(&entry->list, &ima_temp_rules);
> + if (!ima_rule_exists(entry)) {
> + list_add_tail(&entry->list, &ima_temp_rules);
> + } else {
> + result = -EEXIST;
Is it necessary to set result? Or can you just pass -EEXIST to the audit
call below?
> + ima_free_rule(entry);
> + integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> + NULL, op, "duplicate-policy", result,
> + audit_info);
> + }
>
> return len;
> }
^ permalink raw reply
* [Patch V1] ima: avoid duplicate policy rules insertions
From: Tahera Fahimi @ 2025-11-06 18:14 UTC (permalink / raw)
To: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, paul,
jmorris, serge, linux-integrity, linux-security-module,
linux-kernel, code
Cc: Tahera Fahimi
Prevent redundant IMA policy rules by checking for duplicates before insertion. This ensures that
rules are not re-added when userspace is restarted (using systemd-soft-reboot) without a full system
reboot. ima_rule_exists() detects duplicates in both temporary and active rule lists.
Signed-off-by: Tahera Fahimi <taherafahimi@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 157 +++++++++++++++++++++++++++-
1 file changed, 156 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 164d62832f8ec..3dd902101dbda 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1953,6 +1953,153 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
return result;
}
+static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
+{
+ int j;
+
+ for (int j = 0; j < template2->num_fields; j++)
+ if (strcmp(field_id, template2->fields[j]->field_id) == 0)
+ return true;
+
+ return false;
+}
+
+static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
+{
+ int j;
+
+ for (j = 0; j < keyrings->count; j++) {
+ if (strcmp(item, keyrings->items[j]) == 0)
+ return true;
+ }
+ return false;
+}
+
+static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
+{
+ int j;
+
+ for (j = 0; j < labels->count; j++) {
+ if (strcmp(item, labels->items[j]) == 0)
+ return true;
+ }
+ return false;
+}
+
+static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
+{
+ int i;
+
+ if (rule1->flags != rule2->flags)
+ return false;
+
+ if (rule1->action != rule2->action)
+ return false;
+
+ if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
+ ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
+ ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
+ ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
+ ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
+ ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
+ ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
+ ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
+ ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
+ ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
+ ((rule1->flags & IMA_VALIDATE_ALGOS) &&
+ rule1->allowed_algos != rule2->allowed_algos) ||
+ ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
+ ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
+ return false;
+
+ if (!rule1->template && !rule2->template) {
+ ;
+ } else if (!rule1->template || !rule2->template) {
+ return false;
+ } else if (rule1->template->num_fields != rule2->template->num_fields) {
+ return false;
+ } else if (rule1->template->num_fields != 0) {
+ for (i = 0; i < rule1->template->num_fields; i++) {
+ if (!template_has_field(rule1->template->fields[i]->field_id,
+ rule2->template))
+ return false;
+ }
+ }
+
+ if (rule1->flags & IMA_KEYRINGS) {
+ if (!rule1->keyrings && !rule2->keyrings) {
+ ;
+ } else if (!rule1->keyrings || !rule2->keyrings) {
+ return false;
+ } else if (rule1->keyrings->count != rule2->keyrings->count) {
+ return false;
+ } else if (rule1->keyrings->count != 0) {
+ for (i = 0; i < rule1->keyrings->count; i++) {
+ if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
+ return false;
+ }
+ }
+ }
+
+ if (rule1->flags & IMA_LABEL) {
+ if (!rule1->label && !rule2->label) {
+ ;
+ } else if (!rule1->label || !rule2->label) {
+ return false;
+ } else if (rule1->label->count != rule2->label->count) {
+ return false;
+ } else if (rule1->label->count != 0) {
+ for (i = 0; i < rule1->label->count; i++) {
+ if (!labels_has_item(rule1->label->items[i], rule2->label))
+ return false;
+ }
+ }
+ }
+
+ for (i = 0; i < MAX_LSM_RULES; i++) {
+ if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
+ continue;
+
+ if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
+ return false;
+
+ if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
+ return false;
+ }
+
+ return true;
+}
+
+/**
+ * ima_rule_exists - check if a rule already exists in the policy
+ *
+ * Checking both the active policy and the temporary rules list.
+ */
+static bool ima_rule_exists(struct ima_rule_entry *new_rule)
+{
+ struct ima_rule_entry *entry;
+ struct list_head *ima_rules_tmp;
+
+ if (!list_empty(&ima_temp_rules)) {
+ list_for_each_entry(entry, &ima_temp_rules, list) {
+ if (ima_rules_equal(entry, new_rule))
+ return true;
+ }
+ }
+
+ rcu_read_lock();
+ ima_rules_tmp = rcu_dereference(ima_rules);
+ list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
+ if (ima_rules_equal(entry, new_rule)) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
/**
* ima_parse_add_rule - add a rule to ima_policy_rules
* @rule: ima measurement policy rule
@@ -1993,7 +2140,15 @@ ssize_t ima_parse_add_rule(char *rule)
return result;
}
- list_add_tail(&entry->list, &ima_temp_rules);
+ if (!ima_rule_exists(entry)) {
+ list_add_tail(&entry->list, &ima_temp_rules);
+ } else {
+ result = -EEXIST;
+ ima_free_rule(entry);
+ integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
+ NULL, op, "duplicate-policy", result,
+ audit_info);
+ }
return len;
}
--
2.43.0
^ permalink raw reply related
* [PATCH] tpm: add WQ_PERCPU to alloc_workqueue users
From: Marco Crivellari @ 2025-11-06 16:28 UTC (permalink / raw)
To: linux-kernel, linux-integrity
Cc: Tejun Heo, Lai Jiangshan, Frederic Weisbecker,
Sebastian Andrzej Siewior, Marco Crivellari, Michal Hocko,
Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe
Currently if a user enqueues a work item using schedule_delayed_work() the
used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
schedule_work() that is using system_wq and queue_work(), that makes use
again of WORK_CPU_UNBOUND.
This lack of consistency cannot be addressed without refactoring the API.
alloc_workqueue() treats all queues as per-CPU by default, while unbound
workqueues must opt-in via WQ_UNBOUND.
This default is suboptimal: most workloads benefit from unbound queues,
allowing the scheduler to place worker threads where they’re needed and
reducing noise when CPUs are isolated.
This continues the effort to refactor workqueue APIs, which began with
the introduction of new workqueues and a new alloc_workqueue flag in:
commit 128ea9f6ccfb ("workqueue: Add system_percpu_wq and system_dfl_wq")
commit 930c2ea566af ("workqueue: Add new WQ_PERCPU flag")
This change adds a new WQ_PERCPU flag to explicitly request
alloc_workqueue() to be per-cpu when WQ_UNBOUND has not been specified.
With the introduction of the WQ_PERCPU flag (equivalent to !WQ_UNBOUND),
any alloc_workqueue() caller that doesn’t explicitly specify WQ_UNBOUND
must now use WQ_PERCPU.
Once migration is complete, WQ_UNBOUND can be removed and unbound will
become the implicit default.
Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
drivers/char/tpm/tpm-dev-common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f2a5e09257dd..f942c0c8e402 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -275,7 +275,8 @@ void tpm_common_release(struct file *file, struct file_priv *priv)
int __init tpm_dev_common_init(void)
{
- tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+ tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM | WQ_PERCPU,
+ 0);
return !tpm_dev_wq ? -ENOMEM : 0;
}
--
2.51.1
^ permalink raw reply related
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Coiby Xu @ 2025-11-06 13:35 UTC (permalink / raw)
To: Paul Moore, Mimi Zohar
Cc: linux-integrity, linux-security-module, Karel Srot, James Morris,
Serge E. Hallyn, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
open list, open list:MODULE SUPPORT
In-Reply-To: <8646ad03f2f14f45c0ade7c7d7cc148f56d964b1.camel@linux.ibm.com>
On Wed, Nov 05, 2025 at 03:25:05PM -0500, Mimi Zohar wrote:
>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.
Yeah, thank Paul for elaborating on the solution!
>
>Mimi
>
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Coiby Xu @ 2025-11-06 13:29 UTC (permalink / raw)
To: Mimi Zohar
Cc: Paul Moore, linux-integrity, linux-security-module, Karel Srot,
James Morris, Serge E. Hallyn, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, open list, open list:MODULE SUPPORT
In-Reply-To: <84a0e1785c7f0ff816b3246be49012092ae12126.camel@linux.ibm.com>
On Wed, Nov 05, 2025 at 03:47:25PM -0500, Mimi Zohar wrote:
>On Wed, 2025-11-05 at 08:18 +0800, Coiby Xu wrote:
[...]
>
>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.)
Hi Mimi,
Thanks for summarizing your conversation with Paul! I can confirm Paul's
approach works
https://github.com/coiby/linux/tree/in_kernel_decompression_ima_no_lsm_hook_paul
While testing the patch today, I realized there is another
issue/challenge introduced by in-kernel module decompression. IMA
appraisal is to verify the digest of compressed kernel module but
currently the passed buffer is uncompressed module. When IMA uses
uncompressed module data to calculate the digest, xattr signature
verification will fail. If we always make IMA read the original kernel
module data again to calculate the digest, does it look like a
quick-and-dirty fix? If we can assume people won't load kernel module so
often, the performance impact is negligible. Otherwise we may have to
introduce a new LSM hook so IMA can access uncompressed and original
module data one time.
>
>thanks,
>
>Mimi
>
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-06 10:01 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <aQxV2ULFzG/xrl7/@MiWiFi-R3L-srv>
On Thu, Nov 6, 2025 at 4:01 PM Baoquan He <bhe@redhat.com> wrote:
>
> On 11/06/25 at 02:59pm, Pingfan Liu wrote:
> > When I tested kexec with the latest kernel, I ran into the following warning:
> >
> > [ 40.712410] ------------[ cut here ]------------
> > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > [...]
> > [ 40.816047] Call trace:
> > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > [...]
> > [ 40.855423] ---[ end trace 0000000000000000 ]---
> >
> > This is caused by the fact that kexec allocates the destination directly
> > in the CMA area. In that case, the CMA kernel address should be exported
> > directly to the IMA component, instead of using the vmalloc'd address.
>
> Well, you didn't update the log accordingly.
>
I am not sure what you mean. Do you mean the earlier content which I
replied to you?
> Do you know why cma area can't be mapped into vmalloc?
>
Should not the kernel direct mapping be used?
Thanks,
Pingfan
> >
> > Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
> > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Alexander Graf <graf@amazon.com>
> > Cc: Steven Chen <chenste@linux.microsoft.com>
> > Cc: linux-integrity@vger.kernel.org
> > Cc: <stable@vger.kernel.org>
> > To: kexec@lists.infradead.org
> > ---
> > v1 -> v2:
> > return page_address(page) instead of *page
> >
> > kernel/kexec_core.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 9a1966207041..332204204e53 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > kimage_entry_t *ptr, entry;
> > struct page **src_pages;
> > unsigned int npages;
> > + struct page *cma;
> > void *vaddr = NULL;
> > int i;
> >
> > @@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > size = image->segment[idx].memsz;
> > eaddr = addr + size;
> >
> > + cma = image->segment_cma[idx];
> > + if (cma)
> > + return page_address(cma);
> > /*
> > * Collect the source pages and map them in a contiguous VA range.
> > */
> > @@ -1014,7 +1018,8 @@ void *kimage_map_segment(struct kimage *image, int idx)
> >
> > void kimage_unmap_segment(void *segment_buffer)
> > {
> > - vunmap(segment_buffer);
> > + if (is_vmalloc_addr(segment_buffer))
> > + vunmap(segment_buffer);
> > }
> >
> > struct kexec_load_limit {
> > --
> > 2.49.0
> >
>
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-06 8:01 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <20251106065904.10772-2-piliu@redhat.com>
On 11/06/25 at 02:59pm, Pingfan Liu wrote:
> When I tested kexec with the latest kernel, I ran into the following warning:
>
> [ 40.712410] ------------[ cut here ]------------
> [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> [...]
> [ 40.816047] Call trace:
> [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> [ 40.823221] ima_kexec_post_load+0x58/0xc0
> [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> [...]
> [ 40.855423] ---[ end trace 0000000000000000 ]---
>
> This is caused by the fact that kexec allocates the destination directly
> in the CMA area. In that case, the CMA kernel address should be exported
> directly to the IMA component, instead of using the vmalloc'd address.
Well, you didn't update the log accordingly.
Do you know why cma area can't be mapped into vmalloc?
>
> Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
> Signed-off-by: Pingfan Liu <piliu@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Alexander Graf <graf@amazon.com>
> Cc: Steven Chen <chenste@linux.microsoft.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: <stable@vger.kernel.org>
> To: kexec@lists.infradead.org
> ---
> v1 -> v2:
> return page_address(page) instead of *page
>
> kernel/kexec_core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 9a1966207041..332204204e53 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
> kimage_entry_t *ptr, entry;
> struct page **src_pages;
> unsigned int npages;
> + struct page *cma;
> void *vaddr = NULL;
> int i;
>
> @@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
> size = image->segment[idx].memsz;
> eaddr = addr + size;
>
> + cma = image->segment_cma[idx];
> + if (cma)
> + return page_address(cma);
> /*
> * Collect the source pages and map them in a contiguous VA range.
> */
> @@ -1014,7 +1018,8 @@ void *kimage_map_segment(struct kimage *image, int idx)
>
> void kimage_unmap_segment(void *segment_buffer)
> {
> - vunmap(segment_buffer);
> + if (is_vmalloc_addr(segment_buffer))
> + vunmap(segment_buffer);
> }
>
> struct kexec_load_limit {
> --
> 2.49.0
>
^ permalink raw reply
* [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-06 6:59 UTC (permalink / raw)
To: kexec, linux-integrity
Cc: Pingfan Liu, Andrew Morton, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <20251106065904.10772-1-piliu@redhat.com>
When I tested kexec with the latest kernel, I ran into the following warning:
[ 40.712410] ------------[ cut here ]------------
[ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
[...]
[ 40.816047] Call trace:
[ 40.818498] kimage_map_segment+0x144/0x198 (P)
[ 40.823221] ima_kexec_post_load+0x58/0xc0
[ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
[...]
[ 40.855423] ---[ end trace 0000000000000000 ]---
This is caused by the fact that kexec allocates the destination directly
in the CMA area. In that case, the CMA kernel address should be exported
directly to the IMA component, instead of using the vmalloc'd address.
Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Alexander Graf <graf@amazon.com>
Cc: Steven Chen <chenste@linux.microsoft.com>
Cc: linux-integrity@vger.kernel.org
Cc: <stable@vger.kernel.org>
To: kexec@lists.infradead.org
---
v1 -> v2:
return page_address(page) instead of *page
kernel/kexec_core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 9a1966207041..332204204e53 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
kimage_entry_t *ptr, entry;
struct page **src_pages;
unsigned int npages;
+ struct page *cma;
void *vaddr = NULL;
int i;
@@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
size = image->segment[idx].memsz;
eaddr = addr + size;
+ cma = image->segment_cma[idx];
+ if (cma)
+ return page_address(cma);
/*
* Collect the source pages and map them in a contiguous VA range.
*/
@@ -1014,7 +1018,8 @@ void *kimage_map_segment(struct kimage *image, int idx)
void kimage_unmap_segment(void *segment_buffer)
{
- vunmap(segment_buffer);
+ if (is_vmalloc_addr(segment_buffer))
+ vunmap(segment_buffer);
}
struct kexec_load_limit {
--
2.49.0
^ permalink raw reply related
* [PATCHv2 1/2] kernel/kexec: Change the prototype of kimage_map_segment()
From: Pingfan Liu @ 2025-11-06 6:59 UTC (permalink / raw)
To: kexec, linux-integrity
Cc: Pingfan Liu, Andrew Morton, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
The kexec segment index will be required to extract the corresponding
information for that segment in kimage_map_segment(). Additionally,
kexec_segment already holds the kexec relocation destination address and
size. Therefore, the prototype of kimage_map_segment() can be changed.
Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Alexander Graf <graf@amazon.com>
Cc: Steven Chen <chenste@linux.microsoft.com>
Cc: <stable@vger.kernel.org>
To: kexec@lists.infradead.org
To: linux-integrity@vger.kernel.org
---
include/linux/kexec.h | 4 ++--
kernel/kexec_core.c | 9 ++++++---
security/integrity/ima/ima_kexec.c | 4 +---
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index ff7e231b0485..8a22bc9b8c6c 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -530,7 +530,7 @@ extern bool kexec_file_dbg_print;
#define kexec_dprintk(fmt, arg...) \
do { if (kexec_file_dbg_print) pr_info(fmt, ##arg); } while (0)
-extern void *kimage_map_segment(struct kimage *image, unsigned long addr, unsigned long size);
+extern void *kimage_map_segment(struct kimage *image, int idx);
extern void kimage_unmap_segment(void *buffer);
#else /* !CONFIG_KEXEC_CORE */
struct pt_regs;
@@ -540,7 +540,7 @@ static inline void __crash_kexec(struct pt_regs *regs) { }
static inline void crash_kexec(struct pt_regs *regs) { }
static inline int kexec_should_crash(struct task_struct *p) { return 0; }
static inline int kexec_crash_loaded(void) { return 0; }
-static inline void *kimage_map_segment(struct kimage *image, unsigned long addr, unsigned long size)
+static inline void *kimage_map_segment(struct kimage *image, int idx)
{ return NULL; }
static inline void kimage_unmap_segment(void *buffer) { }
#define kexec_in_progress false
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index fa00b239c5d9..9a1966207041 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -960,17 +960,20 @@ int kimage_load_segment(struct kimage *image, int idx)
return result;
}
-void *kimage_map_segment(struct kimage *image,
- unsigned long addr, unsigned long size)
+void *kimage_map_segment(struct kimage *image, int idx)
{
+ unsigned long addr, size, eaddr;
unsigned long src_page_addr, dest_page_addr = 0;
- unsigned long eaddr = addr + size;
kimage_entry_t *ptr, entry;
struct page **src_pages;
unsigned int npages;
void *vaddr = NULL;
int i;
+ addr = image->segment[idx].mem;
+ size = image->segment[idx].memsz;
+ eaddr = addr + size;
+
/*
* Collect the source pages and map them in a contiguous VA range.
*/
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 7362f68f2d8b..5beb69edd12f 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -250,9 +250,7 @@ void ima_kexec_post_load(struct kimage *image)
if (!image->ima_buffer_addr)
return;
- ima_kexec_buffer = kimage_map_segment(image,
- image->ima_buffer_addr,
- image->ima_buffer_size);
+ ima_kexec_buffer = kimage_map_segment(image, image->ima_segment_index);
if (!ima_kexec_buffer) {
pr_err("Could not map measurements buffer.\n");
return;
--
2.49.0
^ permalink raw reply related
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-06 6:56 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <aQwUUh3noWGXaite@MiWiFi-R3L-srv>
On Thu, Nov 6, 2025 at 11:22 AM Baoquan He <bhe@redhat.com> wrote:
>
> On 11/06/25 at 10:33am, Pingfan Liu wrote:
> > Hi Baoquan,
> >
> > Thanks for your review. Please see the comment below.
> >
> > On Thu, Nov 6, 2025 at 10:04 AM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > Hi Pingfan,
> > >
> > > On 11/05/25 at 09:09pm, Pingfan Liu wrote:
> > > > When I tested kexec with the latest kernel, I ran into the following warning:
> > > >
> > > > [ 40.712410] ------------[ cut here ]------------
> > > > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > > > [...]
> > > > [ 40.816047] Call trace:
> > > > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > > > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > > > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > > > [...]
> > > > [ 40.855423] ---[ end trace 0000000000000000 ]---
> > > >
> > > > This is caused by the fact that kexec allocates the destination directly
> > > > in the CMA area. In that case, the CMA kernel address should be exported
> > > > directly to the IMA component, instead of using the vmalloc'd address.
> > > >
> > > > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Baoquan He <bhe@redhat.com>
> > > > Cc: Alexander Graf <graf@amazon.com>
> > > > Cc: Steven Chen <chenste@linux.microsoft.com>
> > > > Cc: linux-integrity@vger.kernel.org
> > > > To: kexec@lists.infradead.org
> > > > ---
> > > > kernel/kexec_core.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > index 9a1966207041..abe40286a02c 100644
> > > > --- a/kernel/kexec_core.c
> > > > +++ b/kernel/kexec_core.c
> > > > @@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > > > kimage_entry_t *ptr, entry;
> > > > struct page **src_pages;
> > > > unsigned int npages;
> > > > + struct page *cma;
> > > > void *vaddr = NULL;
> > > > int i;
> > > >
> > > > @@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > > > size = image->segment[idx].memsz;
> > > > eaddr = addr + size;
> > > >
> > > > + cma = image->segment_cma[idx];
> > >
> > > Thanks for your fix. But I totally can't get what you are doing. The idx
> > > passed into kimage_map_segment() could index image->segment[], and can
> > > index image->segment_cma[], could you reconsider and make the code more
> > > reasonable?
> > >
> >
> > Since idx can index both image->segment[] and segment_cma[], the
> > behavior differs based on whether segment_cma[idx] is NULL:
> >
> > - If segment_cma[idx] is not NULL, it points directly to the final
> > target location, eliminating the need for data copying that
> > traditional kexec relocation requires.
> > - If segment_cma[idx] is NULL, the segment relies on the traditional
> > kexec relocation code to copy its data.
>
> I see, thanks. While image->segment_cma[idx] records the struct page of
> the relevant cma area, but not virtual address. Is it OK for IMA later
Oops. It requires page_address(page) to convert the address. I will
send out V2 to fix it.
Thanks,
Pingfan
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-06 3:21 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <CAF+s44StrV1USH4ZLQ6DRobXA=hXOs6EVkmeeeTsK-koWsV_KQ@mail.gmail.com>
On 11/06/25 at 10:33am, Pingfan Liu wrote:
> Hi Baoquan,
>
> Thanks for your review. Please see the comment below.
>
> On Thu, Nov 6, 2025 at 10:04 AM Baoquan He <bhe@redhat.com> wrote:
> >
> > Hi Pingfan,
> >
> > On 11/05/25 at 09:09pm, Pingfan Liu wrote:
> > > When I tested kexec with the latest kernel, I ran into the following warning:
> > >
> > > [ 40.712410] ------------[ cut here ]------------
> > > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > > [...]
> > > [ 40.816047] Call trace:
> > > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > > [...]
> > > [ 40.855423] ---[ end trace 0000000000000000 ]---
> > >
> > > This is caused by the fact that kexec allocates the destination directly
> > > in the CMA area. In that case, the CMA kernel address should be exported
> > > directly to the IMA component, instead of using the vmalloc'd address.
> > >
> > > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Baoquan He <bhe@redhat.com>
> > > Cc: Alexander Graf <graf@amazon.com>
> > > Cc: Steven Chen <chenste@linux.microsoft.com>
> > > Cc: linux-integrity@vger.kernel.org
> > > To: kexec@lists.infradead.org
> > > ---
> > > kernel/kexec_core.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > index 9a1966207041..abe40286a02c 100644
> > > --- a/kernel/kexec_core.c
> > > +++ b/kernel/kexec_core.c
> > > @@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > > kimage_entry_t *ptr, entry;
> > > struct page **src_pages;
> > > unsigned int npages;
> > > + struct page *cma;
> > > void *vaddr = NULL;
> > > int i;
> > >
> > > @@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > > size = image->segment[idx].memsz;
> > > eaddr = addr + size;
> > >
> > > + cma = image->segment_cma[idx];
> >
> > Thanks for your fix. But I totally can't get what you are doing. The idx
> > passed into kimage_map_segment() could index image->segment[], and can
> > index image->segment_cma[], could you reconsider and make the code more
> > reasonable?
> >
>
> Since idx can index both image->segment[] and segment_cma[], the
> behavior differs based on whether segment_cma[idx] is NULL:
>
> - If segment_cma[idx] is not NULL, it points directly to the final
> target location, eliminating the need for data copying that
> traditional kexec relocation requires.
> - If segment_cma[idx] is NULL, the segment relies on the traditional
> kexec relocation code to copy its data.
I see, thanks. While image->segment_cma[idx] records the struct page of
the relevant cma area, but not virtual address. Is it OK for IMA later
to update? ima_kexec_buffer is supposed to be a virtual address,
wondering how IMA behaved in this case.
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-06 2:57 UTC (permalink / raw)
To: Andrew Morton
Cc: kexec, linux-integrity, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <20251105161432.98eb69f87f30627a9067e78e@linux-foundation.org>
Hi Andrew,
Thanks for your help, but on second thought, I think the Fixes commit is wrong.
On Thu, Nov 6, 2025 at 8:14 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 5 Nov 2025 21:09:22 +0800 Pingfan Liu <piliu@redhat.com> wrote:
>
> > When I tested kexec with the latest kernel, I ran into the following warning:
> >
> > [ 40.712410] ------------[ cut here ]------------
> > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > [...]
> > [ 40.816047] Call trace:
> > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > [...]
> > [ 40.855423] ---[ end trace 0000000000000000 ]---
> >
> > This is caused by the fact that kexec allocates the destination directly
> > in the CMA area. In that case, the CMA kernel address should be exported
> > directly to the IMA component, instead of using the vmalloc'd address.
>
> This is something we should backport into tearlier kernels.
>
> > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Alexander Graf <graf@amazon.com>
> > Cc: Steven Chen <chenste@linux.microsoft.com>
> > Cc: linux-integrity@vger.kernel.org
> > To: kexec@lists.infradead.org
>
> So I'm thinking we should add
>
> Fixes: 0091d9241ea2 ("kexec: define functions to map and unmap segments")
Should be:
Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
Because 07d24902977e came after 0091d9241ea2 and introduced this issue.
Thanks,
Pingfan
> Cc: <stable@vger.kernel.org>
>
> yes?
>
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-06 2:33 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <aQwCDMhidg+a2Jzt@MiWiFi-R3L-srv>
Hi Baoquan,
Thanks for your review. Please see the comment below.
On Thu, Nov 6, 2025 at 10:04 AM Baoquan He <bhe@redhat.com> wrote:
>
> Hi Pingfan,
>
> On 11/05/25 at 09:09pm, Pingfan Liu wrote:
> > When I tested kexec with the latest kernel, I ran into the following warning:
> >
> > [ 40.712410] ------------[ cut here ]------------
> > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > [...]
> > [ 40.816047] Call trace:
> > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > [...]
> > [ 40.855423] ---[ end trace 0000000000000000 ]---
> >
> > This is caused by the fact that kexec allocates the destination directly
> > in the CMA area. In that case, the CMA kernel address should be exported
> > directly to the IMA component, instead of using the vmalloc'd address.
> >
> > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Alexander Graf <graf@amazon.com>
> > Cc: Steven Chen <chenste@linux.microsoft.com>
> > Cc: linux-integrity@vger.kernel.org
> > To: kexec@lists.infradead.org
> > ---
> > kernel/kexec_core.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 9a1966207041..abe40286a02c 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > kimage_entry_t *ptr, entry;
> > struct page **src_pages;
> > unsigned int npages;
> > + struct page *cma;
> > void *vaddr = NULL;
> > int i;
> >
> > @@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
> > size = image->segment[idx].memsz;
> > eaddr = addr + size;
> >
> > + cma = image->segment_cma[idx];
>
> Thanks for your fix. But I totally can't get what you are doing. The idx
> passed into kimage_map_segment() could index image->segment[], and can
> index image->segment_cma[], could you reconsider and make the code more
> reasonable?
>
Since idx can index both image->segment[] and segment_cma[], the
behavior differs based on whether segment_cma[idx] is NULL:
- If segment_cma[idx] is not NULL, it points directly to the final
target location, eliminating the need for data copying that
traditional kexec relocation requires.
- If segment_cma[idx] is NULL, the segment relies on the traditional
kexec relocation code to copy its data.
Thanks,
Pingfan
> > + if (cma)
> > + return cma;
> > /*
> > * Collect the source pages and map them in a contiguous VA range.
> > */
> > @@ -1014,7 +1018,8 @@ void *kimage_map_segment(struct kimage *image, int idx)
> >
> > void kimage_unmap_segment(void *segment_buffer)
> > {
> > - vunmap(segment_buffer);
> > + if (is_vmalloc_addr(segment_buffer))
> > + vunmap(segment_buffer);
> > }
> >
> > struct kexec_load_limit {
> > --
> > 2.49.0
> >
>
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-06 2:03 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <20251105130922.13321-2-piliu@redhat.com>
Hi Pingfan,
On 11/05/25 at 09:09pm, Pingfan Liu wrote:
> When I tested kexec with the latest kernel, I ran into the following warning:
>
> [ 40.712410] ------------[ cut here ]------------
> [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> [...]
> [ 40.816047] Call trace:
> [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> [ 40.823221] ima_kexec_post_load+0x58/0xc0
> [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> [...]
> [ 40.855423] ---[ end trace 0000000000000000 ]---
>
> This is caused by the fact that kexec allocates the destination directly
> in the CMA area. In that case, the CMA kernel address should be exported
> directly to the IMA component, instead of using the vmalloc'd address.
>
> Signed-off-by: Pingfan Liu <piliu@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Alexander Graf <graf@amazon.com>
> Cc: Steven Chen <chenste@linux.microsoft.com>
> Cc: linux-integrity@vger.kernel.org
> To: kexec@lists.infradead.org
> ---
> kernel/kexec_core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 9a1966207041..abe40286a02c 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -967,6 +967,7 @@ void *kimage_map_segment(struct kimage *image, int idx)
> kimage_entry_t *ptr, entry;
> struct page **src_pages;
> unsigned int npages;
> + struct page *cma;
> void *vaddr = NULL;
> int i;
>
> @@ -974,6 +975,9 @@ void *kimage_map_segment(struct kimage *image, int idx)
> size = image->segment[idx].memsz;
> eaddr = addr + size;
>
> + cma = image->segment_cma[idx];
Thanks for your fix. But I totally can't get what you are doing. The idx
passed into kimage_map_segment() could index image->segment[], and can
index image->segment_cma[], could you reconsider and make the code more
reasonable?
> + if (cma)
> + return cma;
> /*
> * Collect the source pages and map them in a contiguous VA range.
> */
> @@ -1014,7 +1018,8 @@ void *kimage_map_segment(struct kimage *image, int idx)
>
> void kimage_unmap_segment(void *segment_buffer)
> {
> - vunmap(segment_buffer);
> + if (is_vmalloc_addr(segment_buffer))
> + vunmap(segment_buffer);
> }
>
> struct kexec_load_limit {
> --
> 2.49.0
>
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Pingfan Liu @ 2025-11-06 1:15 UTC (permalink / raw)
To: Andrew Morton
Cc: kexec, linux-integrity, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <20251105161432.98eb69f87f30627a9067e78e@linux-foundation.org>
On Thu, Nov 6, 2025 at 8:14 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 5 Nov 2025 21:09:22 +0800 Pingfan Liu <piliu@redhat.com> wrote:
>
> > When I tested kexec with the latest kernel, I ran into the following warning:
> >
> > [ 40.712410] ------------[ cut here ]------------
> > [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> > [...]
> > [ 40.816047] Call trace:
> > [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> > [ 40.823221] ima_kexec_post_load+0x58/0xc0
> > [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> > [...]
> > [ 40.855423] ---[ end trace 0000000000000000 ]---
> >
> > This is caused by the fact that kexec allocates the destination directly
> > in the CMA area. In that case, the CMA kernel address should be exported
> > directly to the IMA component, instead of using the vmalloc'd address.
>
> This is something we should backport into tearlier kernels.
>
> > Signed-off-by: Pingfan Liu <piliu@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Alexander Graf <graf@amazon.com>
> > Cc: Steven Chen <chenste@linux.microsoft.com>
> > Cc: linux-integrity@vger.kernel.org
> > To: kexec@lists.infradead.org
>
> So I'm thinking we should add
>
> Fixes: 0091d9241ea2 ("kexec: define functions to map and unmap segments")
> Cc: <stable@vger.kernel.org>
>
> yes?
>
Yes, it should be. Thanks for your help!
Best Regards,
Pingfan
^ permalink raw reply
* Re: [PATCH 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Andrew Morton @ 2025-11-06 0:14 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Baoquan He, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen
In-Reply-To: <20251105130922.13321-2-piliu@redhat.com>
On Wed, 5 Nov 2025 21:09:22 +0800 Pingfan Liu <piliu@redhat.com> wrote:
> When I tested kexec with the latest kernel, I ran into the following warning:
>
> [ 40.712410] ------------[ cut here ]------------
> [ 40.712576] WARNING: CPU: 2 PID: 1562 at kernel/kexec_core.c:1001 kimage_map_segment+0x144/0x198
> [...]
> [ 40.816047] Call trace:
> [ 40.818498] kimage_map_segment+0x144/0x198 (P)
> [ 40.823221] ima_kexec_post_load+0x58/0xc0
> [ 40.827246] __do_sys_kexec_file_load+0x29c/0x368
> [...]
> [ 40.855423] ---[ end trace 0000000000000000 ]---
>
> This is caused by the fact that kexec allocates the destination directly
> in the CMA area. In that case, the CMA kernel address should be exported
> directly to the IMA component, instead of using the vmalloc'd address.
This is something we should backport into tearlier kernels.
> Signed-off-by: Pingfan Liu <piliu@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Alexander Graf <graf@amazon.com>
> Cc: Steven Chen <chenste@linux.microsoft.com>
> Cc: linux-integrity@vger.kernel.org
> To: kexec@lists.infradead.org
So I'm thinking we should add
Fixes: 0091d9241ea2 ("kexec: define functions to map and unmap segments")
Cc: <stable@vger.kernel.org>
yes?
^ permalink raw reply
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: Bagas Sanjaya @ 2025-11-05 23:23 UTC (permalink / raw)
To: James Bottomley, Linux Kernel Mailing List, Linux Documentation,
Linux Integrity
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <bbba0752a40859a114bac987d279a8b268e5e5eb.camel@HansenPartnership.com>
[-- Attachment #1: Type: text/plain, Size: 707 bytes --]
On Wed, Nov 05, 2025 at 11:33:39AM -0500, James Bottomley wrote:
> On Wed, 2025-11-05 at 20:04 +0700, Bagas Sanjaya wrote:
> > > Why might it need moving?
> >
> > Just to tidy up toctree then...
>
> I'd really rather have the files in the doc tree grouped for ease (and
> reminder of) maintenance. The two headings belong together in one file
> because if someone updates the doc for one, there's at least a chance
> they'll notice the other might need an update as well; whereas if we do
> separate files for every heading the tree becomes very fragmented and
> the chance of something being missed increases.
>
OK, thanks!
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-05 20:47 UTC (permalink / raw)
To: Coiby Xu, Paul Moore
Cc: linux-integrity, linux-security-module, Karel Srot, James Morris,
Serge E. Hallyn, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
open list, open list:MODULE SUPPORT
In-Reply-To: <fftfj4o3kqxmfu3hb655xczqcddoeqjv55llsnwkrdu5isdm4z@6sqe3k24a6kk>
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
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-05 20:25 UTC (permalink / raw)
To: Paul Moore
Cc: Coiby Xu, linux-integrity, linux-security-module, Karel Srot,
James Morris, Serge E. Hallyn, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, open list, open list:MODULE SUPPORT
In-Reply-To: <CAHC9VhS6xWvu5TjjS4MRGFEWxdAhg-Xsf6L+=K0k8U+fgiAtTQ@mail.gmail.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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox