* 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: Pingfan Liu @ 2025-11-07 9:00 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <aQ2C1UyJoyyuC/ZK@MiWiFi-R3L-srv>
On Fri, Nov 07, 2025 at 01:25:41PM +0800, Baoquan He wrote:
> 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.
>
It doesn't go that far. The old code doesn't map CMA into vmalloc'd
area.
void *kimage_map_segment(struct kimage *image, int idx)
{
...
for_each_kimage_entry(image, ptr, entry) {
if (entry & IND_DESTINATION) {
dest_page_addr = entry & PAGE_MASK;
} else if (entry & IND_SOURCE) {
if (dest_page_addr >= addr && dest_page_addr < eaddr) {
src_page_addr = entry & PAGE_MASK;
src_pages[i++] =
virt_to_page(__va(src_page_addr));
if (i == npages)
break;
dest_page_addr += PAGE_SIZE;
}
}
}
/* Sanity check. */
WARN_ON(i < npages); //--> This is the warning thrown by kernel
vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
kfree(src_pages);
if (!vaddr)
pr_err("Could not map ima buffer.\n");
return vaddr;
}
When CMA is used, there is no IND_SOURCE, so we have i=0 < npages.
Now, I see how my words ("In that case, the CMA kernel address should be
exported directly to the IMA component, instead of using the vmalloc'd
address.") confused you. As for "instead of using the vmalloc'd
address", I meant to mention "vmap()" approach.
Best Regards,
Pingfan
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-07 9:31 UTC (permalink / raw)
To: Pingfan Liu
Cc: kexec, linux-integrity, Andrew Morton, Mimi Zohar, Roberto Sassu,
Alexander Graf, Steven Chen, stable
In-Reply-To: <aQ21GVR1pEjzWvw1@fedora>
On 11/07/25 at 05:00pm, Pingfan Liu wrote:
> On Fri, Nov 07, 2025 at 01:25:41PM +0800, Baoquan He wrote:
> > 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.
> >
>
> It doesn't go that far. The old code doesn't map CMA into vmalloc'd
> area.
>
> void *kimage_map_segment(struct kimage *image, int idx)
> {
> ...
> for_each_kimage_entry(image, ptr, entry) {
> if (entry & IND_DESTINATION) {
> dest_page_addr = entry & PAGE_MASK;
> } else if (entry & IND_SOURCE) {
> if (dest_page_addr >= addr && dest_page_addr < eaddr) {
> src_page_addr = entry & PAGE_MASK;
> src_pages[i++] =
> virt_to_page(__va(src_page_addr));
> if (i == npages)
> break;
> dest_page_addr += PAGE_SIZE;
> }
> }
> }
>
> /* Sanity check. */
> WARN_ON(i < npages); //--> This is the warning thrown by kernel
>
> vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
> kfree(src_pages);
>
> if (!vaddr)
> pr_err("Could not map ima buffer.\n");
>
> return vaddr;
> }
>
> When CMA is used, there is no IND_SOURCE, so we have i=0 < npages.
> Now, I see how my words ("In that case, the CMA kernel address should be
> exported directly to the IMA component, instead of using the vmalloc'd
> address.") confused you. As for "instead of using the vmalloc'd
> address", I meant to mention "vmap()" approach.
Ok, I got it. It's truly a bug because if image->segment_cma[idx] is
valid, the current kimage_map_segment() can't collect the source pages
at all since they are not marked with IND_DESTINATION|IND_SOURCE as
normal segment does. In that situation, we can take the direct mapping
address of image->segment_cma[idx] which is more efficient, instead of
collecting source pages and vmap().
^ permalink raw reply
* Re: [PATCHv2 2/2] kernel/kexec: Fix IMA when allocation happens in CMA area
From: Baoquan He @ 2025-11-07 9:34 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.
>
> 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);
This judgement can be put above the addr/size/eaddr assignment lines?
If you agree, maybe you can update the patch log by adding more details
to explain the root cause so that people can understand it easier.
> /*
> * 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 V1] ima: avoid duplicate policy rules insertions
From: Roberto Sassu @ 2025-11-07 9:44 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: Lennart Poettering
In-Reply-To: <20251106181404.3429710-1-taherafahimi@linux.microsoft.com>
On Thu, 2025-11-06 at 18:14 +0000, 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.
+ Lennart
Hi Tahera
thanks for the patch!
Wouldn't be better to enhance systemd-soft-reboot to not send the same
IMA policy again?
Thanks
Roberto
> 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;
> }
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Lennart Poettering @ 2025-11-07 10:56 UTC (permalink / raw)
To: Roberto Sassu
Cc: Tahera Fahimi, zohar, roberto.sassu, dmitry.kasatkin,
eric.snowberg, paul, jmorris, serge, linux-integrity,
linux-security-module, linux-kernel, code
In-Reply-To: <1cc67c25a141aef8982840898a6e7397cbdf10d9.camel@huaweicloud.com>
On Fr, 07.11.25 10:44, Roberto Sassu (roberto.sassu@huaweicloud.com) wrote:
> On Thu, 2025-11-06 at 18:14 +0000, 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.
>
> + Lennart
>
> Hi Tahera
>
> thanks for the patch!
>
> Wouldn't be better to enhance systemd-soft-reboot to not send the same
> IMA policy again?
the soft-reboot logic doesn't load the IMA policy. It's just that
soft-reboot means we reexec PID1: the old pid1 gets replaced by the
new one. And that new PID1 then initializes as it usually would, and
loads security policies again. It currently has support for selinux
policies, ima, ipe, smack.
These policies are supposed to *replace* whatever was loaded
before. Looking at our IMA logic, this doesn't happen right now
though, it just adds stuff:
https://github.com/systemd/systemd/blob/main/src/core/ima-setup.c
Is there a way to replace the old IMA policy with the new, with the
current IMA userspace interface? If so, we should probably make use of
that in systemd, and replace the policy that way. Or in other words:
under the assumption that one can flush out the old IMA policy and
replace it with a new one, I think this should be fixed in systemd,
not the kernel. (of there's no api for flushing out the old
policy/replacing it with the new, then of course we need something
like that in the kernel first).
My understanding of IMA is kinda limited though. I just know what we
do in our codebase.
Lennart
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Roberto Sassu @ 2025-11-07 11:56 UTC (permalink / raw)
To: Lennart Poettering
Cc: Tahera Fahimi, zohar, roberto.sassu, dmitry.kasatkin,
eric.snowberg, paul, jmorris, serge, linux-integrity,
linux-security-module, linux-kernel, code
In-Reply-To: <aQ3QV03_PtB4qg32@gardel-login>
On Fri, 2025-11-07 at 11:56 +0100, Lennart Poettering wrote:
> On Fr, 07.11.25 10:44, Roberto Sassu (roberto.sassu@huaweicloud.com) wrote:
>
> > On Thu, 2025-11-06 at 18:14 +0000, 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.
> >
> > + Lennart
> >
> > Hi Tahera
> >
> > thanks for the patch!
> >
> > Wouldn't be better to enhance systemd-soft-reboot to not send the same
> > IMA policy again?
>
> the soft-reboot logic doesn't load the IMA policy. It's just that
> soft-reboot means we reexec PID1: the old pid1 gets replaced by the
> new one. And that new PID1 then initializes as it usually would, and
> loads security policies again. It currently has support for selinux
> policies, ima, ipe, smack.
>
> These policies are supposed to *replace* whatever was loaded
> before. Looking at our IMA logic, this doesn't happen right now
> though, it just adds stuff:
From a functional perspective. As far as integrity is concerned, you
would probably agree that just replacing PID 1 does not mean resetting
the integrity of the system to a known state (all the other processes
are still running). Due to that, I think the concept of soft-reset
should not be applied to the kernel.
> https://github.com/systemd/systemd/blob/main/src/core/ima-setup.c
>
> Is there a way to replace the old IMA policy with the new, with the
> current IMA userspace interface? If so, we should probably make use of
No, only the IMA boot policies specified in the kernel command line can
be reset (only once, and not completely, secure boot rules still
persist despite user space loads a new policy). New rules are append-
only.
> that in systemd, and replace the policy that way. Or in other words:
> under the assumption that one can flush out the old IMA policy and
> replace it with a new one, I think this should be fixed in systemd,
> not the kernel. (of there's no api for flushing out the old
> policy/replacing it with the new, then of course we need something
> like that in the kernel first).
Assuming that technically it is feasible to flush the old IMA policy
(except for the permanent secure boot rules). What it would be the
additional value of changing the policy on the fly on the same system
as before, but with a different PID 1?
Regarding the duplicate IMA policy load, I guess you could probably
store the digest of the currently loaded policy in the systemd state
and not doing it again after soft-reboot, if the digest matches.
Roberto
> My understanding of IMA is kinda limited though. I just know what we
> do in our codebase.
>
> Lennart
^ 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-07 19:28 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: <b9eb78105115a00731b3677a5f3a39d5dde4d2ec.camel@linux.ibm.com>
On Thu, 2025-11-06 at 17:15 -0500, Mimi Zohar wrote:
> 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.
In the case when the compressed kernel module hasn't previously been measured or
appraised before loading the kernel module, we need to "collect" the file data
hash on READING_MODULE_COMPRESSED, but defer appraising/measuring it.
An alternative to your suggestion of re-reading the original kernel module data
to calculate the digest or defining a new hook, would be to define "collect" as
a new "action" and pass the kernel_read_file_id enumeration to
process_measurement(). IMA_COLLECTED already exists. Only IMA_COLLECT would
need to be defined. The new collect "action" should be limited to
func=MODULE_CHECK.
The downside of this alternative is that it requires a new collect rule:
collect func=MODULE_CHECK mask=MAY_READ uid=0
appraise func=MODULE_CHECK appraise_type=imasig|modsig
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>
From: Jarkko Sakkinen @ 2025-11-09 4:34 UTC (permalink / raw)
To: Jonathan McDowell
Cc: Roberto Sassu, Peter Huewe, Jason Gunthorpe, Linus Torvalds,
James Bottomley, linux-integrity, linux-kernel, zohar
In-Reply-To: <aQj2wZrnV7vgoAcq@earth.li>
On Mon, Nov 03, 2025 at 06:38:57PM +0000, Jonathan McDowell wrote:
> On Mon, Oct 27, 2025 at 09:38:55PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
> > > On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
> > > > From: Jonathan McDowell <noodles@meta.com>
> > > >
> > > > There are situations where userspace might reasonably desire exclusive
> > > > access to the TPM, or the kernel's internal context saving + flushing
> > > > may cause issues, for example when performing firmware upgrades. Extend
> > > > the locking already used for avoiding concurrent userspace access to
> > > > prevent internal users of the TPM when /dev/tpm<n> is in use.
> > > >
> > > > The few internal users who already hold the open_lock are changed to use
> > > > tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
> > > > functions changing to obtain read access to the open_lock. We return
> > > > -EBUSY when another user has exclusive access, rather than adding waits.
> > > >
> > > > Signed-off-by: Jonathan McDowell <noodles@meta.com>
> > > > ---
> > > > v2: Switch to _locked instead of _internal_ for function names.
> > > > v3: Move to end of patch series.
> > > >
> > > > drivers/char/tpm/tpm-chip.c | 53 +++++++++++++++++++++++++------
> > > > drivers/char/tpm/tpm-dev-common.c | 8 ++---
> > > > drivers/char/tpm/tpm.h | 2 ++
> > > > drivers/char/tpm/tpm2-space.c | 5 ++-
> > > > 4 files changed, 52 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > > index ba906966721a..687f6d8cd601 100644
> > > > --- a/drivers/char/tpm/tpm-chip.c
> > > > +++ b/drivers/char/tpm/tpm-chip.c
> > > > @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
> > > > EXPORT_SYMBOL_GPL(tpm_chip_stop);
> > > >
> > > > /**
> > > > - * tpm_try_get_ops() - Get a ref to the tpm_chip
> > > > + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
> > > > * @chip: Chip to ref
> > > > *
> > > > * The caller must already have some kind of locking to ensure that chip is
> > > > @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
> > > > *
> > > > * Returns -ERRNO if the chip could not be got.
> > > > */
> > > > -int tpm_try_get_ops(struct tpm_chip *chip)
> > > > +int tpm_try_get_ops_locked(struct tpm_chip *chip)
> > > > {
> > > > int rc = -EIO;
> > > >
> > > > @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
> > > > put_device(&chip->dev);
> > > > return rc;
> > > > }
> > > > -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
> > > >
> > > > /**
> > > > - * tpm_put_ops() - Release a ref to the tpm_chip
> > > > + * tpm_put_ops_locked() - Release a ref to the tpm_chip
> > > > * @chip: Chip to put
> > > > *
> > > > - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
> > > > - * be kfree'd.
> > > > + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
> > > > + * chip may be kfree'd.
> > > > */
> > > > -void tpm_put_ops(struct tpm_chip *chip)
> > > > +void tpm_put_ops_locked(struct tpm_chip *chip)
> > > > {
> > > > tpm_chip_stop(chip);
> > > > mutex_unlock(&chip->tpm_mutex);
> > > > up_read(&chip->ops_sem);
> > > > put_device(&chip->dev);
> > > > }
> > > > +
> > > > +/**
> > > > + * tpm_try_get_ops() - Get a ref to the tpm_chip
> > > > + * @chip: Chip to ref
> > > > + *
> > > > + * The caller must already have some kind of locking to ensure that chip is
> > > > + * valid. This function will attempt to get the open_lock for the chip,
> > > > + * ensuring no other user is expecting exclusive access, before locking the
> > > > + * chip so that the ops member can be accessed safely. The locking prevents
> > > > + * tpm_chip_unregister from completing, so it should not be held for long
> > > > + * periods.
> > > > + *
> > > > + * Returns -ERRNO if the chip could not be got.
> > > > + */
> > > > +int tpm_try_get_ops(struct tpm_chip *chip)
> > > > +{
> > > > + if (!down_read_trylock(&chip->open_lock))
> > > > + return -EBUSY;
> > >
> > > Hi Jonathan
> > >
> > > do I understand it correctly, that a process might open the TPM with
> > > O_EXCL, and this will prevent IMA from extending a PCR until that
> > > process closes the file descriptor?
> > >
> > > If yes, this might be a concern, and I think an additional API to
> > > prevent such behavior would be needed (for example when IMA is active,
> > > i.e. there is a measurement policy loaded).
> >
> > Also this would be a problem with hwrng.
> >
> > This probably needs to be refined somehow. I don't have a solution at
> > hand but "invariant" is that in-kernel caller should override user space
> > exclusion, even when O_EXCL is used.
>
> Kernel access is exactly what caused the issue for me, in particular the HW
> RNG access during a firmware upgrade. My patch to be able to disable the HW
> RNG at runtime has landed in -next, which helps a lot, but it really would
> be nice to be able to say "Hands off, I'm busy with this", which is what led
> to this patch set.
If there is a situation when kernel needs to be excluded from itself,
then there should really be a kernel uapi to implement that use case.
I'd rather have e.g. ioctl (perhaps just picking one possible tool for
the job) for firmware upgrade than allow user space to arbitarily lock
TPM access.
>
> To James' query about the fact the upgrade process should be properly
> handled, I think the issue is probably that the HMAC context saving around
> HW RNG access hit errors that were not gracefully handled, and we marked the
> TPM as disabled in tpm2_load_null, causing failure mid-upgrade.
>
> J.
>
> --
> What have you got in your pocket?
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] tpm_crb: add missing loc parameter to kerneldoc
From: Jarkko Sakkinen @ 2025-11-09 4:41 UTC (permalink / raw)
To: Stuart Yoder
Cc: linux-integrity, peterhuewe, jgg, sudeep.holla, Prachotan.Bathi,
linux-kernel
In-Reply-To: <20251028020921.214189-1-stuart.yoder@arm.com>
On Mon, Oct 27, 2025 at 09:09:21PM -0500, Stuart Yoder wrote:
> Update the kerneldoc parameter definitions for __crb_go_idle
> and __crb_cmd_ready to include the loc parameter.
>
> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> ---
> drivers/char/tpm/tpm_crb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index c75a531cfb98..0eb48429f73d 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -179,6 +179,7 @@ static int crb_try_pluton_doorbell(struct crb_priv *priv, bool wait_for_complete
> *
> * @dev: crb device
> * @priv: crb private data
> + * @loc: locality
> *
> * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> * The device should respond within TIMEOUT_C by clearing the bit.
> @@ -233,6 +234,7 @@ static int crb_go_idle(struct tpm_chip *chip)
> *
> * @dev: crb device
> * @priv: crb private data
> + * @loc: locality
> *
> * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> * and poll till the device acknowledge it by clearing the bit.
> --
> 2.34.1
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] hwrng: tpm: Do not enable by default
From: Jarkko Sakkinen @ 2025-11-09 4:43 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Huewe, linux-integrity, Linux Kernel Mailing List,
Ilias Apalodimas, Jens Wiklander, OP-TEE TrustedFirmware,
linux-crypto
In-Reply-To: <9cbee028-81a7-4be6-aa31-907c7cc683e3@siemens.com>
On Tue, Oct 28, 2025 at 06:46:39AM +0100, Jan Kiszka wrote:
> On 27.10.25 20:51, Jarkko Sakkinen wrote:
> > On Tue, Oct 21, 2025 at 02:46:15PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> As seen with optee_ftpm, which uses ms-tpm-20-ref [1], a TPM may write
> >> the current time epoch to its NV storage every 4 seconds if there are
> >> commands sent to it. The 60 seconds periodic update of the entropy pool
> >> that the hwrng kthread does triggers this, causing about 4 writes per
> >> requests. Makes 2 millions per year for a 24/7 device, and that is a lot
> >> for its backing NV storage.
> >>
> >> It is therefore better to make the user intentionally enable this,
> >> providing a chance to read the warning.
> >>
> >> [1] https://github.com/Microsoft/ms-tpm-20-ref
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >
> > Looking at DRBG_* from [1] I don't see anything you describe. If OPTEE
> > writes NVRAM, then the implementation is broken.
>
> It's not OP-TEE, but it might be indeed an artifact of the reference
> implementation that the fTPM is using because it is also targeting
> simulation:
>
> https://github.com/microsoft/ms-tpm-20-ref/blob/ee21db0a941decd3cac67925ea3310873af60ab3/TPMCmd/tpm/src/main/ExecCommand.c#L99
> (Page 942 in [1])
>
> -> ... ->
>
> https://github.com/microsoft/ms-tpm-20-ref/blob/main/TPMCmd/tpm/src/subsystem/Time.c#L68
> (Page 1075 in [1])
>
> >
> > Also AFAIK, it is pre-seeded per power cycle. There's nothing that even
> > distantly relates on using NVRAM.
> >
> > [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-4-Supporting-Routines-Code.pdf
> >
> > BR, Jarkko
>
> Given how detailed [1] is, we likely need to address that directly there
> to avoid spreading this issue into fTPMs. Fact is, that there firmware
> implementations out there which exactly do what [1] suggests: writing to
> NV every 4 seconds on every command.
We don't reference code as a certified hardware product, sorry.
>
> Jan
>
> --
> Siemens AG, Foundational Technologies
> Linux Expert Center
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: Remove the ad-hoc compilation flag CAAM_DEBUG
From: Jarkko Sakkinen @ 2025-11-09 4:44 UTC (permalink / raw)
To: Ye Bin
Cc: a.fatoum, kernel, James.Bottomley, zohar, dhowells, paul, jmorris,
serge, linux-integrity, keyrings, linux-security-module, yebin10
In-Reply-To: <20251028132254.841715-1-yebin@huaweicloud.com>
On Tue, Oct 28, 2025 at 09:22:54PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Fix the broken design based on Jarkko Sakkinen's suggestions as follows:
>
> 1. Remove the ad-hoc compilation flag (i.e., CAAM_DEBUG).
> 2. Substitute pr_info calls with pr_debug calls.
>
> Closes: https://patchwork.kernel.org/project/linux-integrity/patch/20251024061153.61470-1-yebin@huaweicloud.com/
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> security/keys/trusted-keys/trusted_caam.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/keys/trusted-keys/trusted_caam.c
> index 601943ce0d60..c903ee7328ca 100644
> --- a/security/keys/trusted-keys/trusted_caam.c
> +++ b/security/keys/trusted-keys/trusted_caam.c
> @@ -28,16 +28,10 @@ static const match_table_t key_tokens = {
> {opt_err, NULL}
> };
>
> -#ifdef CAAM_DEBUG
> static inline void dump_options(const struct caam_pkey_info *pkey_info)
> {
> - pr_info("key encryption algo %d\n", pkey_info->key_enc_algo);
> + pr_debug("key encryption algo %d\n", pkey_info->key_enc_algo);
> }
> -#else
> -static inline void dump_options(const struct caam_pkey_info *pkey_info)
> -{
> -}
> -#endif
>
> static int get_pkey_options(char *c,
> struct caam_pkey_info *pkey_info)
> --
> 2.34.1
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Return early on allocation failure and drop goto
From: Jarkko Sakkinen @ 2025-11-09 4:47 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <20251029163157.119000-1-thorsten.blum@linux.dev>
On Wed, Oct 29, 2025 at 05:31:56PM +0100, Thorsten Blum wrote:
> Return ERR_PTR(-ENOMEM) immediately if memory allocation fails, instead
> of using goto and returning a NULL pointer, and remove the now-unused
> 'out' label.
>
> At the call site, check 'ascii_buf' with IS_ERR() and propagate the
> error code returned by datablob_format().
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/keys/encrypted-keys/encrypted.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index be1f2118447c..25df00b7dbe9 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -276,7 +276,7 @@ static char *datablob_format(struct encrypted_key_payload *epayload,
>
> ascii_buf = kmalloc(asciiblob_len + 1, GFP_KERNEL);
> if (!ascii_buf)
> - goto out;
> + return ERR_PTR(-ENOMEM);
>
> ascii_buf[asciiblob_len] = '\0';
>
> @@ -288,7 +288,6 @@ static char *datablob_format(struct encrypted_key_payload *epayload,
> bufp = &ascii_buf[len];
> for (i = 0; i < (asciiblob_len - len) / 2; i++)
> bufp = hex_byte_pack(bufp, iv[i]);
> -out:
> return ascii_buf;
> }
>
> @@ -932,8 +931,8 @@ static long encrypted_read(const struct key *key, char *buffer,
> goto out;
>
> ascii_buf = datablob_format(epayload, asciiblob_len);
> - if (!ascii_buf) {
> - ret = -ENOMEM;
> + if (IS_ERR(ascii_buf)) {
> + ret = PTR_ERR(ascii_buf);
> goto out;
> }
>
> --
> 2.51.0
>
No thank you as this does not really fix anything.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: Jarkko Sakkinen @ 2025-11-09 4:50 UTC (permalink / raw)
To: Bagas Sanjaya
Cc: Linux Kernel Mailing List, Linux Documentation, Linux Integrity,
Peter Huewe, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <20251104131312.23791-1-bagasdotme@gmail.com>
On Tue, Nov 04, 2025 at 08:13:12PM +0700, Bagas Sanjaya wrote:
> The last section heading in TPM security docs is formatted as title
> heading instead. As such, it shows up as TPM toctree entry. Demote it
> to section heading as appropriate.
>
> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
> Documentation/security/tpm/tpm-security.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/security/tpm/tpm-security.rst b/Documentation/security/tpm/tpm-security.rst
> index 4f633f2510336b..bf73bbe66db2fa 100644
> --- a/Documentation/security/tpm/tpm-security.rst
> +++ b/Documentation/security/tpm/tpm-security.rst
> @@ -153,7 +153,7 @@ protect key sealing and parameter decryption to protect key unsealing
> and random number generation.
>
> Null Primary Key Certification in Userspace
> -===========================================
> +-------------------------------------------
>
> Every TPM comes shipped with a couple of X.509 certificates for the
> primary endorsement key. This document assumes that the Elliptic
>
> base-commit: 27600b51fbc8b9a4eba18c8d88d7edb146605f3f
> --
> An old man doll... just what I always wanted! - Clara
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Should I pick this?
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] tpm: add WQ_PERCPU to alloc_workqueue users
From: Jarkko Sakkinen @ 2025-11-09 4:53 UTC (permalink / raw)
To: Marco Crivellari
Cc: linux-kernel, linux-integrity, Tejun Heo, Lai Jiangshan,
Frederic Weisbecker, Sebastian Andrzej Siewior, Michal Hocko,
Peter Huewe, Jason Gunthorpe
In-Reply-To: <20251106162800.331872-1-marco.crivellari@suse.com>
On Thu, Nov 06, 2025 at 05:28:00PM +0100, Marco Crivellari wrote:
> 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
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] hwrng: tpm: Do not enable by default
From: Jan Kiszka @ 2025-11-09 10:04 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, linux-integrity, Linux Kernel Mailing List,
Ilias Apalodimas, Jens Wiklander, OP-TEE TrustedFirmware,
linux-crypto
In-Reply-To: <aRAb7KEPmPmoyQbm@kernel.org>
On 09.11.25 05:43, Jarkko Sakkinen wrote:
> On Tue, Oct 28, 2025 at 06:46:39AM +0100, Jan Kiszka wrote:
>> On 27.10.25 20:51, Jarkko Sakkinen wrote:
>>> On Tue, Oct 21, 2025 at 02:46:15PM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> As seen with optee_ftpm, which uses ms-tpm-20-ref [1], a TPM may write
>>>> the current time epoch to its NV storage every 4 seconds if there are
>>>> commands sent to it. The 60 seconds periodic update of the entropy pool
>>>> that the hwrng kthread does triggers this, causing about 4 writes per
>>>> requests. Makes 2 millions per year for a 24/7 device, and that is a lot
>>>> for its backing NV storage.
>>>>
>>>> It is therefore better to make the user intentionally enable this,
>>>> providing a chance to read the warning.
>>>>
>>>> [1] https://github.com/Microsoft/ms-tpm-20-ref
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Looking at DRBG_* from [1] I don't see anything you describe. If OPTEE
>>> writes NVRAM, then the implementation is broken.
>>
>> It's not OP-TEE, but it might be indeed an artifact of the reference
>> implementation that the fTPM is using because it is also targeting
>> simulation:
>>
>> https://github.com/microsoft/ms-tpm-20-ref/blob/ee21db0a941decd3cac67925ea3310873af60ab3/TPMCmd/tpm/src/main/ExecCommand.c#L99
>> (Page 942 in [1])
>>
>> -> ... ->
>>
>> https://github.com/microsoft/ms-tpm-20-ref/blob/main/TPMCmd/tpm/src/subsystem/Time.c#L68
>> (Page 1075 in [1])
>>
>>>
>>> Also AFAIK, it is pre-seeded per power cycle. There's nothing that even
>>> distantly relates on using NVRAM.
>>>
>>> [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-4-Supporting-Routines-Code.pdf
>>>
>>> BR, Jarkko
>>
>> Given how detailed [1] is, we likely need to address that directly there
>> to avoid spreading this issue into fTPMs. Fact is, that there firmware
>> implementations out there which exactly do what [1] suggests: writing to
>> NV every 4 seconds on every command.
>
> We don't reference code as a certified hardware product, sorry.
>
Means what?
Jan
--
Siemens AG, Foundational Technologies
Linux Expert Center
^ permalink raw reply
* Re: [PATCH] Documentation: tpm: tpm-security: Demote "Null Primary Key Certification in Userspace" section
From: Bagas Sanjaya @ 2025-11-09 12:29 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Linux Kernel Mailing List, Linux Documentation, Linux Integrity,
Peter Huewe, Jason Gunthorpe, Jonathan Corbet
In-Reply-To: <aRAdiUB9otJk5i9U@kernel.org>
On 11/9/25 11:50, Jarkko Sakkinen wrote:
> On Tue, Nov 04, 2025 at 08:13:12PM +0700, Bagas Sanjaya wrote:
>> The last section heading in TPM security docs is formatted as title
>> heading instead. As such, it shows up as TPM toctree entry. Demote it
>> to section heading as appropriate.
>>
>> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> ---
>> Documentation/security/tpm/tpm-security.rst | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/security/tpm/tpm-security.rst b/Documentation/security/tpm/tpm-security.rst
>> index 4f633f2510336b..bf73bbe66db2fa 100644
>> --- a/Documentation/security/tpm/tpm-security.rst
>> +++ b/Documentation/security/tpm/tpm-security.rst
>> @@ -153,7 +153,7 @@ protect key sealing and parameter decryption to protect key unsealing
>> and random number generation.
>>
>> Null Primary Key Certification in Userspace
>> -===========================================
>> +-------------------------------------------
>>
>> Every TPM comes shipped with a couple of X.509 certificates for the
>> primary endorsement key. This document assumes that the Elliptic
>>
>> base-commit: 27600b51fbc8b9a4eba18c8d88d7edb146605f3f
>> --
>> An old man doll... just what I always wanted! - Clara
>>
>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> Should I pick this?
>
Of course!
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Tahera Fahimi @ 2025-11-10 19:06 UTC (permalink / raw)
To: Anirudh Venkataramanan, zohar, roberto.sassu, dmitry.kasatkin,
eric.snowberg, paul, jmorris, serge, linux-integrity,
linux-security-module, linux-kernel, code
In-Reply-To: <b36a6508-1b2a-4c87-b3b5-9af0b402dc0b@linux.microsoft.com>
On 11/6/2025 12:32 PM, Anirudh Venkataramanan wrote:
> 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.
Hi Ahirudh, Thanks for your feedback.
>>
>> 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.
The declaration of j is at the beginning to adhere proper kernel style and ancient compile support.
>> +
>> + 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?
The purpose of this patch is to prohibit two exact same rule.
We can have other approaches like merging the new rule to the previously existing rule, ignore
new rule if a similar rule exists. However, this approaches would add more complexity to the code
and are not the purpose of this patch.
> 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:
> ...
> ...
I don't think having two goto in the code will improve its readability.
>> +
>> + 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;
>> }
I
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Roberto Sassu @ 2025-11-11 9:46 UTC (permalink / raw)
To: Tahera Fahimi, Anirudh Venkataramanan, zohar, roberto.sassu,
dmitry.kasatkin, eric.snowberg, paul, jmorris, serge,
linux-integrity, linux-security-module, linux-kernel, code
In-Reply-To: <14c61ba5-437f-496d-8356-5712ddb37d47@linux.microsoft.com>
On Mon, 2025-11-10 at 11:06 -0800, Tahera Fahimi wrote:
> On 11/6/2025 12:32 PM, Anirudh Venkataramanan wrote:
> > 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.
> Hi Ahirudh, Thanks for your feedback.
> > >
> > > 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.
> The declaration of j is at the beginning to adhere proper kernel style and ancient compile support.
> > > +
> > > + 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?
>
> The purpose of this patch is to prohibit two exact same rule.
Why would an administrator attempt to load the same rule twice?
How likely is the case where one would like to combine multiple signed
IMA policies with common rules?
Unless there is a realistic use case, it would be better to patch user
space first.
Thanks
Roberto
> We can have other approaches like merging the new rule to the previously existing rule, ignore
> new rule if a similar rule exists. However, this approaches would add more complexity to the code
> and are not the purpose of this patch.
>
> > 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:
> > ...
> > ...
> I don't think having two goto in the code will improve its readability.
>
> > > +
> > > + 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;
> > > }
> I
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Mimi Zohar @ 2025-11-11 11:40 UTC (permalink / raw)
To: Tahera Fahimi, roberto.sassu, dmitry.kasatkin, eric.snowberg,
paul, jmorris, serge, linux-integrity, linux-security-module,
linux-kernel, code
Cc: Lennart Poettering
In-Reply-To: <20251106181404.3429710-1-taherafahimi@linux.microsoft.com>
[Cc'ing Lennart Poettering]
On Thu, 2025-11-06 at 18:14 +0000, 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.
>
> Signed-off-by: Tahera Fahimi <taherafahimi@linux.microsoft.com>
Sorry for the delay in responding ...
Before trying to fix the "problem", let's try to understand it first. At least
on my test system (-rc5), kexec is working as designed. On boot, systemd
replaces the existing builtin IMA policy with a custom IMA policy. The arch
specific policies are not affected, as they are persistent. On a soft reboot
(kexec), the IMA custom policy is re-loaded as expected.
To verify the above behavior, extend the IMA policy before the soft reboot.
Notice after the soft reboot that the original custom IMA policy is loaded and
not the extended IMA policy. Roberto, if there is a problem with this behavior,
we'll discuss it independently of this proposed patch.
The question is why are you seeing duplicate IMA policy rules? What is special
about your environment?
--
thanks,
Mimi
^ 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-13 4:06 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: <0dfec96bf98b1c18d51bf40f4329c3ede48a9f32.camel@linux.ibm.com>
On Fri, Nov 07, 2025 at 02:28:13PM -0500, Mimi Zohar wrote:
>On Thu, 2025-11-06 at 17:15 -0500, Mimi Zohar wrote:
>> 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.
>
>In the case when the compressed kernel module hasn't previously been measured or
>appraised before loading the kernel module, we need to "collect" the file data
>hash on READING_MODULE_COMPRESSED, but defer appraising/measuring it.
>
>An alternative to your suggestion of re-reading the original kernel module data
>to calculate the digest or defining a new hook, would be to define "collect" as
>a new "action" and pass the kernel_read_file_id enumeration to
>process_measurement(). IMA_COLLECTED already exists. Only IMA_COLLECT would
>need to be defined. The new collect "action" should be limited to
>func=MODULE_CHECK.
>
>The downside of this alternative is that it requires a new collect rule:
>collect func=MODULE_CHECK mask=MAY_READ uid=0
>appraise func=MODULE_CHECK appraise_type=imasig|modsig
Thank for suggesting an alternative! I've implemented the idea in
https://github.com/coiby/linux/tree/in_kernel_decompression_ima_collect
Note besides a new collect rule, another change is needed. Currently,
process_measurement only accepts enum ima_hooks thus it can't tell if
it's READING_MODULE_COMPRESSED so to only do collect action. So I
create a fake MODULE_COMPRESSED_CHECK func.
And for the idea of re-reading the original kernel module data, it has
been implemented in
https://github.com/coiby/linux/tree/in_kernel_decompression_ima_no_lsm_hook_paul
Both branches have applied your requested three changes including
respecting the 80 char line limit. Additionally, I made a change to the
IPE LSM because of the new READING_MODULE_COMPRESSED kernel_read_file_id
enumerate.
After comparing the two implementations, personally I prefer re-reading
the original kernel module data because the change is smaller and it's
more user-friendly. But if there are other reasons I don't know, I'll
post the patches of the new collect action approach to the mailing list.
--
Best regards,
Coiby
^ permalink raw reply
* [PATCH] KEYS: encrypted: Use pr_fmt()
From: Thorsten Blum @ 2025-11-13 12:35 UTC (permalink / raw)
To: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-integrity, keyrings, linux-security-module,
linux-kernel
Use pr_fmt() to automatically prefix all pr_<level>() log messages with
"encrypted_key: " and remove all manually added prefixes.
Reformat the code accordingly and avoid line breaks in log messages.
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/keys/encrypted-keys/encrypted.c | 74 +++++++++++-------------
security/keys/encrypted-keys/encrypted.h | 2 +-
2 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 513c09e2b01c..a8e8bf949b4b 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -11,6 +11,8 @@
* See Documentation/security/keys/trusted-encrypted.rst
*/
+#define pr_fmt(fmt) "encrypted_key: " fmt
+
#include <linux/uaccess.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -84,8 +86,7 @@ static int aes_get_sizes(void)
tfm = crypto_alloc_skcipher(blkcipher_alg, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm)) {
- pr_err("encrypted_key: failed to alloc_cipher (%ld)\n",
- PTR_ERR(tfm));
+ pr_err("failed to alloc_cipher (%ld)\n", PTR_ERR(tfm));
return PTR_ERR(tfm);
}
ivsize = crypto_skcipher_ivsize(tfm);
@@ -106,15 +107,14 @@ static int valid_ecryptfs_desc(const char *ecryptfs_desc)
int i;
if (strlen(ecryptfs_desc) != KEY_ECRYPTFS_DESC_LEN) {
- pr_err("encrypted_key: key description must be %d hexadecimal "
- "characters long\n", KEY_ECRYPTFS_DESC_LEN);
+ pr_err("key description must be %d hexadecimal characters long\n",
+ KEY_ECRYPTFS_DESC_LEN);
return -EINVAL;
}
for (i = 0; i < KEY_ECRYPTFS_DESC_LEN; i++) {
if (!isxdigit(ecryptfs_desc[i])) {
- pr_err("encrypted_key: key description must contain "
- "only hexadecimal characters\n");
+ pr_err("key description must contain only hexadecimal characters\n");
return -EINVAL;
}
}
@@ -180,7 +180,7 @@ static int datablob_parse(char *datablob, const char **format,
keyword = strsep(&datablob, " \t");
if (!keyword) {
- pr_info("encrypted_key: insufficient parameters specified\n");
+ pr_info("insufficient parameters specified\n");
return ret;
}
key_cmd = match_token(keyword, key_tokens, args);
@@ -188,7 +188,7 @@ static int datablob_parse(char *datablob, const char **format,
/* Get optional format: default | ecryptfs */
p = strsep(&datablob, " \t");
if (!p) {
- pr_err("encrypted_key: insufficient parameters specified\n");
+ pr_err("insufficient parameters specified\n");
return ret;
}
@@ -206,20 +206,20 @@ static int datablob_parse(char *datablob, const char **format,
}
if (!*master_desc) {
- pr_info("encrypted_key: master key parameter is missing\n");
+ pr_info("master key parameter is missing\n");
goto out;
}
if (valid_master_desc(*master_desc, NULL) < 0) {
- pr_info("encrypted_key: master key parameter \'%s\' "
- "is invalid\n", *master_desc);
+ pr_info("master key parameter \'%s\' is invalid\n",
+ *master_desc);
goto out;
}
if (decrypted_datalen) {
*decrypted_datalen = strsep(&datablob, " \t");
if (!*decrypted_datalen) {
- pr_info("encrypted_key: keylen parameter is missing\n");
+ pr_info("keylen parameter is missing\n");
goto out;
}
}
@@ -227,8 +227,8 @@ static int datablob_parse(char *datablob, const char **format,
switch (key_cmd) {
case Opt_new:
if (!decrypted_datalen) {
- pr_info("encrypted_key: keyword \'%s\' not allowed "
- "when called from .update method\n", keyword);
+ pr_info("keyword \'%s\' not allowed when called from .update method\n",
+ keyword);
break;
}
*decrypted_data = strsep(&datablob, " \t");
@@ -236,29 +236,27 @@ static int datablob_parse(char *datablob, const char **format,
break;
case Opt_load:
if (!decrypted_datalen) {
- pr_info("encrypted_key: keyword \'%s\' not allowed "
- "when called from .update method\n", keyword);
+ pr_info("keyword \'%s\' not allowed when called from .update method\n",
+ keyword);
break;
}
*hex_encoded_iv = strsep(&datablob, " \t");
if (!*hex_encoded_iv) {
- pr_info("encrypted_key: hex blob is missing\n");
+ pr_info("hex blob is missing\n");
break;
}
ret = 0;
break;
case Opt_update:
if (decrypted_datalen) {
- pr_info("encrypted_key: keyword \'%s\' not allowed "
- "when called from .instantiate method\n",
+ pr_info("keyword \'%s\' not allowed when called from .instantiate method\n",
keyword);
break;
}
ret = 0;
break;
case Opt_err:
- pr_info("encrypted_key: keyword \'%s\' not recognized\n",
- keyword);
+ pr_info("keyword \'%s\' not recognized\n", keyword);
break;
}
out:
@@ -362,22 +360,21 @@ static struct skcipher_request *init_skcipher_req(const u8 *key,
tfm = crypto_alloc_skcipher(blkcipher_alg, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm)) {
- pr_err("encrypted_key: failed to load %s transform (%ld)\n",
- blkcipher_alg, PTR_ERR(tfm));
+ pr_err("failed to load %s transform (%ld)\n", blkcipher_alg,
+ PTR_ERR(tfm));
return ERR_CAST(tfm);
}
ret = crypto_skcipher_setkey(tfm, key, key_len);
if (ret < 0) {
- pr_err("encrypted_key: failed to setkey (%d)\n", ret);
+ pr_err("failed to setkey (%d)\n", ret);
crypto_free_skcipher(tfm);
return ERR_PTR(ret);
}
req = skcipher_request_alloc(tfm, GFP_KERNEL);
if (!req) {
- pr_err("encrypted_key: failed to allocate request for %s\n",
- blkcipher_alg);
+ pr_err("failed to allocate request for %s\n", blkcipher_alg);
crypto_free_skcipher(tfm);
return ERR_PTR(-ENOMEM);
}
@@ -406,13 +403,10 @@ static struct key *request_master_key(struct encrypted_key_payload *epayload,
if (IS_ERR(mkey)) {
int ret = PTR_ERR(mkey);
-
if (ret == -ENOTSUPP)
- pr_info("encrypted_key: key %s not supported",
- epayload->master_desc);
+ pr_info("key %s not supported", epayload->master_desc);
else
- pr_info("encrypted_key: key %s not found",
- epayload->master_desc);
+ pr_info("key %s not found", epayload->master_desc);
goto out;
}
@@ -457,7 +451,7 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
skcipher_request_free(req);
crypto_free_skcipher(tfm);
if (ret < 0)
- pr_err("encrypted_key: failed to encrypt (%d)\n", ret);
+ pr_err("failed to encrypt (%d)\n", ret);
else
dump_encrypted_data(epayload, encrypted_datalen);
out:
@@ -596,16 +590,16 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
if (decrypted_data) {
if (!user_decrypted_data) {
- pr_err("encrypted key: instantiation of keys using provided decrypted data is disabled since CONFIG_USER_DECRYPTED_DATA is set to false\n");
+ pr_err("instantiation of keys using provided decrypted data is disabled since CONFIG_USER_DECRYPTED_DATA is set to false\n");
return ERR_PTR(-EINVAL);
}
if (strlen(decrypted_data) != decrypted_datalen * 2) {
- pr_err("encrypted key: decrypted data provided does not match decrypted data length provided\n");
+ pr_err("decrypted data provided does not match decrypted data length provided\n");
return ERR_PTR(-EINVAL);
}
for (i = 0; i < strlen(decrypted_data); i++) {
if (!isxdigit(decrypted_data[i])) {
- pr_err("encrypted key: decrypted data provided must contain only hexadecimal characters\n");
+ pr_err("decrypted data provided must contain only hexadecimal characters\n");
return ERR_PTR(-EINVAL);
}
}
@@ -614,7 +608,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
if (format) {
if (!strcmp(format, key_format_ecryptfs)) {
if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
- pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
+ pr_err("keylen for the ecryptfs format must be equal to %d bytes\n",
ECRYPTFS_MAX_KEY_BYTES);
return ERR_PTR(-EINVAL);
}
@@ -622,8 +616,8 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
payload_datalen = sizeof(struct ecryptfs_auth_tok);
} else if (!strcmp(format, key_format_enc32)) {
if (decrypted_datalen != KEY_ENC32_PAYLOAD_LEN) {
- pr_err("encrypted_key: enc32 key payload incorrect length: %d\n",
- decrypted_datalen);
+ pr_err("enc32 key payload incorrect length: %d\n",
+ decrypted_datalen);
return ERR_PTR(-EINVAL);
}
}
@@ -689,7 +683,7 @@ static int encrypted_key_decrypt(struct encrypted_key_payload *epayload,
ret = datablob_hmac_verify(epayload, format, master_key, master_keylen);
if (ret < 0) {
- pr_err("encrypted_key: bad hmac (%d)\n", ret);
+ pr_err("bad hmac (%d)\n", ret);
goto out;
}
@@ -699,7 +693,7 @@ static int encrypted_key_decrypt(struct encrypted_key_payload *epayload,
ret = derived_key_decrypt(epayload, derived_key, sizeof derived_key);
if (ret < 0)
- pr_err("encrypted_key: failed to decrypt key (%d)\n", ret);
+ pr_err("failed to decrypt key (%d)\n", ret);
out:
up_read(&mkey->sem);
key_put(mkey);
diff --git a/security/keys/encrypted-keys/encrypted.h b/security/keys/encrypted-keys/encrypted.h
index 1809995db452..7b05c66bafa6 100644
--- a/security/keys/encrypted-keys/encrypted.h
+++ b/security/keys/encrypted-keys/encrypted.h
@@ -41,7 +41,7 @@ static inline void dump_hmac(const char *str, const u8 *digest,
unsigned int hmac_size)
{
if (str)
- pr_info("encrypted_key: %s", str);
+ pr_info("%s", str);
print_hex_dump(KERN_ERR, "hmac: ", DUMP_PREFIX_NONE, 32, 1, digest,
hmac_size, 0);
}
--
2.51.1
^ permalink raw reply related
* [PATCH] KEYS: encrypted: Replace deprecated strcpy and improve get_derived_key
From: Thorsten Blum @ 2025-11-13 13:58 UTC (permalink / raw)
To: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn
Cc: linux-hardening, Thorsten Blum, linux-integrity, keyrings,
linux-security-module, linux-kernel
Determine 'key_name' before allocating memory for 'derived_buf' to only
allocate as many bytes as needed. Currently, we potentially allocate one
more byte than necessary when 'key_name' is "ENC_KEY".
strcpy() is deprecated and uses an additional strlen() internally; use
memcpy() directly to copy 'key_name' since we already know its length
and that it is guaranteed to be NUL-terminated.
Also reuse 'key_name_len' when copying 'master_key' instead of calling
strlen() again.
Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/keys/encrypted-keys/encrypted.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 15841466b5d4..b16a5b8b935b 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -12,6 +12,7 @@
*/
#include <linux/uaccess.h>
+#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
@@ -330,23 +331,18 @@ static int get_derived_key(u8 *derived_key, enum derived_key_type key_type,
const u8 *master_key, size_t master_keylen)
{
u8 *derived_buf;
- unsigned int derived_buf_len;
-
- derived_buf_len = strlen("AUTH_KEY") + 1 + master_keylen;
- if (derived_buf_len < HASH_SIZE)
- derived_buf_len = HASH_SIZE;
+ size_t derived_buf_len;
+ const char *key_name;
+ size_t key_name_len;
+ key_name = key_type ? "AUTH_KEY" : "ENC_KEY";
+ key_name_len = strlen(key_name) + 1;
+ derived_buf_len = max(key_name_len + master_keylen, HASH_SIZE);
derived_buf = kzalloc(derived_buf_len, GFP_KERNEL);
if (!derived_buf)
return -ENOMEM;
-
- if (key_type)
- strcpy(derived_buf, "AUTH_KEY");
- else
- strcpy(derived_buf, "ENC_KEY");
-
- memcpy(derived_buf + strlen(derived_buf) + 1, master_key,
- master_keylen);
+ memcpy(derived_buf, key_name, key_name_len);
+ memcpy(derived_buf + key_name_len, master_key, master_keylen);
sha256(derived_buf, derived_buf_len, derived_key);
kfree_sensitive(derived_buf);
return 0;
--
2.51.1
^ permalink raw reply related
* Re: [PATCH] KEYS: encrypted: Replace deprecated strcpy and improve get_derived_key
From: Eric Biggers @ 2025-11-13 17:29 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn, linux-hardening, linux-integrity,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20251113135831.98587-1-thorsten.blum@linux.dev>
On Thu, Nov 13, 2025 at 02:58:31PM +0100, Thorsten Blum wrote:
> Determine 'key_name' before allocating memory for 'derived_buf' to only
> allocate as many bytes as needed. Currently, we potentially allocate one
> more byte than necessary when 'key_name' is "ENC_KEY".
>
> strcpy() is deprecated and uses an additional strlen() internally; use
> memcpy() directly to copy 'key_name' since we already know its length
> and that it is guaranteed to be NUL-terminated.
>
> Also reuse 'key_name_len' when copying 'master_key' instead of calling
> strlen() again.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/keys/encrypted-keys/encrypted.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index 15841466b5d4..b16a5b8b935b 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -12,6 +12,7 @@
> */
>
> #include <linux/uaccess.h>
> +#include <linux/minmax.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/slab.h>
> @@ -330,23 +331,18 @@ static int get_derived_key(u8 *derived_key, enum derived_key_type key_type,
> const u8 *master_key, size_t master_keylen)
> {
> u8 *derived_buf;
> - unsigned int derived_buf_len;
> -
> - derived_buf_len = strlen("AUTH_KEY") + 1 + master_keylen;
> - if (derived_buf_len < HASH_SIZE)
> - derived_buf_len = HASH_SIZE;
> + size_t derived_buf_len;
> + const char *key_name;
> + size_t key_name_len;
>
> + key_name = key_type ? "AUTH_KEY" : "ENC_KEY";
> + key_name_len = strlen(key_name) + 1;
> + derived_buf_len = max(key_name_len + master_keylen, HASH_SIZE);
> derived_buf = kzalloc(derived_buf_len, GFP_KERNEL);
> if (!derived_buf)
> return -ENOMEM;
> -
> - if (key_type)
> - strcpy(derived_buf, "AUTH_KEY");
> - else
> - strcpy(derived_buf, "ENC_KEY");
> -
> - memcpy(derived_buf + strlen(derived_buf) + 1, master_key,
> - master_keylen);
> + memcpy(derived_buf, key_name, key_name_len);
> + memcpy(derived_buf + key_name_len, master_key, master_keylen);
> sha256(derived_buf, derived_buf_len, derived_key);
> kfree_sensitive(derived_buf);
> return 0;
This changes the resulting derived key when key_type == 0 &&
master_keylen >= 24, because different bytes are passed to sha256().
- Eric
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Replace deprecated strcpy and improve get_derived_key
From: Thorsten Blum @ 2025-11-13 20:23 UTC (permalink / raw)
To: Eric Biggers
Cc: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn, linux-hardening, linux-integrity,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20251113172913.GC1792@sol>
On 13. Nov 2025, at 18:29, Eric Biggers wrote:
> [...]
>
> This changes the resulting derived key when key_type == 0 &&
> master_keylen >= 24, because different bytes are passed to sha256().
I see, thanks. I'll submit a v2.
^ 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