Hi Roberto,
it seems that the recent changes in IMA in linux-next caused a
regression: namely it triggers an Oops when booting with the options
ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
It hits a NULL dereference at ima_match_policy() like:
[ 10.766220] ==================================================================
[ 10.767415] BUG: KASAN: null-ptr-deref in ima_match_policy+0xf7/0xb80
[ 10.768503] Read of size 8 at addr 0000000000000000 by task systemd/1
[ 10.769574]
[ 10.770046] ==================================================================
[ 10.771813] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 10.773049] #PF: supervisor read access in kernel mode
[ 10.773977] #PF: error_code(0x0000) - not-present page
[ 10.774842] PGD 0 P4D 0
[ 10.775302] Oops: 0000 [#1] SMP KASAN PTI
[ 10.776231] CPU: 0 PID: 1 Comm: systemd Tainted: G B 5.7.0-rc7-next-20200526-1.g3f79a08-vanilla #1
[ 10.777882] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014
[ 10.779790] RIP: 0010:ima_match_policy+0xf7/0xb80
[ 10.780620] Code: ae 96 ff e8 ab 28 00 00 4c 89 f7 48 89 c3 e8 b0 e9 bf ff 49 89 1e e8 38 ae 96 ff 48 8b 2d 21 2c 8a 02 48 89 ef e8 f9 e8 bf ff <48> 8b 5d 00 c7 44 24 04 00 00 00 00 48 39 dd 0f 84 74 05 00 00 8b
[ 10.783569] RSP: 0018:ffffc9000001fc80 EFLAGS: 00010286
[ 10.784476] RAX: 0000000000000001 RBX: 0000000000000104 RCX: ffffffff91368791
[ 10.786274] RDX: 0000000000000000 RSI: 0000000000000246 RDI: 0000000000000246
[ 10.787679] RBP: 0000000000000000 R08: ffff88800fdfbd80 R09: fffffbfff27dda0d
[ 10.789089] R10: ffffffff93eed067 R11: fffffbfff27dda0c R12: 0000000000000001
[ 10.790484] R13: ffff88800fdfbd80 R14: 0000000000000000 R15: 000000000000030c
[ 10.792015] FS: 00007f9368b9f940(0000) GS:ffff88806c600000(0000) knlGS:0000000000000000
[ 10.793647] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 10.794613] CR2: 0000000000000000 CR3: 00000000626b8000 CR4: 00000000000006f0
[ 10.796064] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 10.797347] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 10.798576] Call Trace:
[ 10.798993] ? ima_lsm_policy_change+0x2b0/0x2b0
[ 10.799753] ? inode_init_owner+0x1a0/0x1a0
[ 10.800484] ? _raw_spin_lock+0x7a/0xd0
[ 10.801592] ima_must_appraise.part.0+0xb6/0xf0
[ 10.802313] ? ima_fix_xattr.isra.0+0xd0/0xd0
[ 10.803167] ima_must_appraise+0x4f/0x70
[ 10.804004] ima_post_path_mknod+0x2e/0x80
[ 10.804800] do_mknodat+0x396/0x3c0
[ 10.805563] ? do_file_open_root+0x290/0x290
[ 10.806535] do_syscall_64+0x44/0xb0
[ 10.807301] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 10.808360] RIP: 0033:0x7f936713ba90
[ 10.808996] Code: b8 ff ff ff ff c3 0f 1f 40 00 85 ff 49 89 f0 75 41 48 8b 01 89 c1 48 39 c8 75 37 89 d6 4c 89 c7 48 89 c2 b8 85 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 08 f3 c3 66 0f 1f 44 00 00 48 8b 15 d1 73 2c
It's a KVM instance without any TPM stuff, just passed the options
above. I could trigger the same bug on a bare metal, too.
Then I performed bisection and it spotted the commit:
6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
ima: Switch to ima_hash_algo for boot aggregate
Actually reverting this commit fixed the Oops again.
I haven't looked at the change deeply yet, so just reporting.
Please let me know if you come up with a fix.
Thanks!
Takashi
On Thu, 28 May 2020 17:35:16 +0200,
Takashi Iwai wrote:
>
> Hi Roberto,
>
> it seems that the recent changes in IMA in linux-next caused a
> regression: namely it triggers an Oops when booting with the options
> ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
And further experiment revealed that passing only ima_template_fmt=d
is enough for triggering the bug. Other formats don't matter.
(snip)
> It's a KVM instance without any TPM stuff, just passed the options
> above. I could trigger the same bug on a bare metal, too.
>
> Then I performed bisection and it spotted the commit:
> 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
> ima: Switch to ima_hash_algo for boot aggregate
>
> Actually reverting this commit fixed the Oops again.
So, looking at the fact above (triggered by "d") and this bisection
result, it seems that the issue is specific to ima_eventdigest_init().
The difference from others is that this has a check by
ima_template_hash_algo_allowed(), and currently the check allows only
SHA1 and MD5, while now SHA256 is assigned as default. So I tested
adding SHA256 there like below, and it seems working.
Hopefully I'm heading to a right direction...
thanks,
Takashi
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -13,7 +13,8 @@
static bool ima_template_hash_algo_allowed(u8 algo)
{
- if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_MD5)
+ if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_SHA256 ||
+ algo == HASH_ALGO_MD5)
return true;
return false;
> From: [email protected] [mailto:linux-integrity-
> [email protected]] On Behalf Of Takashi Iwai
> On Thu, 28 May 2020 17:35:16 +0200,
> Takashi Iwai wrote:
> >
> > Hi Roberto,
> >
> > it seems that the recent changes in IMA in linux-next caused a
> > regression: namely it triggers an Oops when booting with the options
> > ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
>
> And further experiment revealed that passing only ima_template_fmt=d
> is enough for triggering the bug. Other formats don't matter.
>
> (snip)
> > It's a KVM instance without any TPM stuff, just passed the options
> > above. I could trigger the same bug on a bare metal, too.
> >
> > Then I performed bisection and it spotted the commit:
> > 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
> > ima: Switch to ima_hash_algo for boot aggregate
> >
> > Actually reverting this commit fixed the Oops again.
>
> So, looking at the fact above (triggered by "d") and this bisection
> result, it seems that the issue is specific to ima_eventdigest_init().
> The difference from others is that this has a check by
> ima_template_hash_algo_allowed(), and currently the check allows only
> SHA1 and MD5, while now SHA256 is assigned as default. So I tested
> adding SHA256 there like below, and it seems working.
>
> Hopefully I'm heading to a right direction...
Hi Takashi
boot_aggregate is the only entry for which there is no file descriptor.
The file descriptor is used to recalculate the digest if it is not SHA1
or MD5. For boot_aggregate, we should use instead
ima_calc_boot_aggregate(). I will provide a patch.
I see that the .file member of event_data in
ima_add_boot_aggregate() is not initialized. Can you please try
to set .file to NULL?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> thanks,
>
> Takashi
>
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -13,7 +13,8 @@
>
> static bool ima_template_hash_algo_allowed(u8 algo)
> {
> - if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_MD5)
> + if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_SHA256 ||
> + algo == HASH_ALGO_MD5)
> return true;
>
> return false;
On Thu, 28 May 2020 19:36:55 +0200,
Roberto Sassu wrote:
>
> > From: [email protected] [mailto:linux-integrity-
> > [email protected]] On Behalf Of Takashi Iwai
> > On Thu, 28 May 2020 17:35:16 +0200,
> > Takashi Iwai wrote:
> > >
> > > Hi Roberto,
> > >
> > > it seems that the recent changes in IMA in linux-next caused a
> > > regression: namely it triggers an Oops when booting with the options
> > > ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
> >
> > And further experiment revealed that passing only ima_template_fmt=d
> > is enough for triggering the bug. Other formats don't matter.
> >
> > (snip)
> > > It's a KVM instance without any TPM stuff, just passed the options
> > > above. I could trigger the same bug on a bare metal, too.
> > >
> > > Then I performed bisection and it spotted the commit:
> > > 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
> > > ima: Switch to ima_hash_algo for boot aggregate
> > >
> > > Actually reverting this commit fixed the Oops again.
> >
> > So, looking at the fact above (triggered by "d") and this bisection
> > result, it seems that the issue is specific to ima_eventdigest_init().
> > The difference from others is that this has a check by
> > ima_template_hash_algo_allowed(), and currently the check allows only
> > SHA1 and MD5, while now SHA256 is assigned as default. So I tested
> > adding SHA256 there like below, and it seems working.
> >
> > Hopefully I'm heading to a right direction...
>
> Hi Takashi
>
> boot_aggregate is the only entry for which there is no file descriptor.
> The file descriptor is used to recalculate the digest if it is not SHA1
> or MD5. For boot_aggregate, we should use instead
> ima_calc_boot_aggregate(). I will provide a patch.
>
> I see that the .file member of event_data in
> ima_add_boot_aggregate() is not initialized. Can you please try
> to set .file to NULL?
Tested and it didn't help. The field was already zero-initialized via
C99-style initialization, I believe.
thanks,
Takashi
>
> Thanks
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
>
> > thanks,
> >
> > Takashi
> >
> > --- a/security/integrity/ima/ima_template_lib.c
> > +++ b/security/integrity/ima/ima_template_lib.c
> > @@ -13,7 +13,8 @@
> >
> > static bool ima_template_hash_algo_allowed(u8 algo)
> > {
> > - if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_MD5)
> > + if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_SHA256 ||
> > + algo == HASH_ALGO_MD5)
> > return true;
> >
> > return false;
>
> From: Takashi Iwai [mailto:[email protected]]
> On Thu, 28 May 2020 19:36:55 +0200,
> Roberto Sassu wrote:
> >
> > > From: [email protected] [mailto:linux-integrity-
> > > [email protected]] On Behalf Of Takashi Iwai
> > > On Thu, 28 May 2020 17:35:16 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > Hi Roberto,
> > > >
> > > > it seems that the recent changes in IMA in linux-next caused a
> > > > regression: namely it triggers an Oops when booting with the options
> > > > ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
> > >
> > > And further experiment revealed that passing only
> ima_template_fmt=d
> > > is enough for triggering the bug. Other formats don't matter.
> > >
> > > (snip)
> > > > It's a KVM instance without any TPM stuff, just passed the options
> > > > above. I could trigger the same bug on a bare metal, too.
> > > >
> > > > Then I performed bisection and it spotted the commit:
> > > > 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
> > > > ima: Switch to ima_hash_algo for boot aggregate
> > > >
> > > > Actually reverting this commit fixed the Oops again.
> > >
> > > So, looking at the fact above (triggered by "d") and this bisection
> > > result, it seems that the issue is specific to ima_eventdigest_init().
> > > The difference from others is that this has a check by
> > > ima_template_hash_algo_allowed(), and currently the check allows only
> > > SHA1 and MD5, while now SHA256 is assigned as default. So I tested
> > > adding SHA256 there like below, and it seems working.
> > >
> > > Hopefully I'm heading to a right direction...
> >
> > Hi Takashi
> >
> > boot_aggregate is the only entry for which there is no file descriptor.
> > The file descriptor is used to recalculate the digest if it is not SHA1
> > or MD5. For boot_aggregate, we should use instead
> > ima_calc_boot_aggregate(). I will provide a patch.
> >
> > I see that the .file member of event_data in
> > ima_add_boot_aggregate() is not initialized. Can you please try
> > to set .file to NULL?
>
> Tested and it didn't help. The field was already zero-initialized via
> C99-style initialization, I believe.
Found the issue.
ima_evendigest_init() returns an error and after that IMA is not
initialized. Unfortunately, ima_must_appraise() does not check
ima_policy_flag, so the kernel crashes when ima_match_policy()
tries to evaluate the policy which is not loaded (ima_rules = NULL).
if you add at the beginning of ima_must_appraise()
if (!ima_policy_flag)
return 0;
the kernel should not crash.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> thanks,
>
> Takashi
>
> >
> > Thanks
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Li Jian, Shi Yanli
> >
> > > thanks,
> > >
> > > Takashi
> > >
> > > --- a/security/integrity/ima/ima_template_lib.c
> > > +++ b/security/integrity/ima/ima_template_lib.c
> > > @@ -13,7 +13,8 @@
> > >
> > > static bool ima_template_hash_algo_allowed(u8 algo)
> > > {
> > > - if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_MD5)
> > > + if (algo == HASH_ALGO_SHA1 || algo == HASH_ALGO_SHA256 ||
> > > + algo == HASH_ALGO_MD5)
> > > return true;
> > >
> > > return false;
> >
On Fri, 29 May 2020 09:33:34 +0200,
Roberto Sassu wrote:
>
> > From: Takashi Iwai [mailto:[email protected]]
> > On Thu, 28 May 2020 19:36:55 +0200,
> > Roberto Sassu wrote:
> > >
> > > > From: [email protected] [mailto:linux-integrity-
> > > > [email protected]] On Behalf Of Takashi Iwai
> > > > On Thu, 28 May 2020 17:35:16 +0200,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > Hi Roberto,
> > > > >
> > > > > it seems that the recent changes in IMA in linux-next caused a
> > > > > regression: namely it triggers an Oops when booting with the options
> > > > > ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
> > > >
> > > > And further experiment revealed that passing only
> > ima_template_fmt=d
> > > > is enough for triggering the bug. Other formats don't matter.
> > > >
> > > > (snip)
> > > > > It's a KVM instance without any TPM stuff, just passed the options
> > > > > above. I could trigger the same bug on a bare metal, too.
> > > > >
> > > > > Then I performed bisection and it spotted the commit:
> > > > > 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
> > > > > ima: Switch to ima_hash_algo for boot aggregate
> > > > >
> > > > > Actually reverting this commit fixed the Oops again.
> > > >
> > > > So, looking at the fact above (triggered by "d") and this bisection
> > > > result, it seems that the issue is specific to ima_eventdigest_init().
> > > > The difference from others is that this has a check by
> > > > ima_template_hash_algo_allowed(), and currently the check allows only
> > > > SHA1 and MD5, while now SHA256 is assigned as default. So I tested
> > > > adding SHA256 there like below, and it seems working.
> > > >
> > > > Hopefully I'm heading to a right direction...
> > >
> > > Hi Takashi
> > >
> > > boot_aggregate is the only entry for which there is no file descriptor.
> > > The file descriptor is used to recalculate the digest if it is not SHA1
> > > or MD5. For boot_aggregate, we should use instead
> > > ima_calc_boot_aggregate(). I will provide a patch.
> > >
> > > I see that the .file member of event_data in
> > > ima_add_boot_aggregate() is not initialized. Can you please try
> > > to set .file to NULL?
> >
> > Tested and it didn't help. The field was already zero-initialized via
> > C99-style initialization, I believe.
>
> Found the issue.
>
> ima_evendigest_init() returns an error and after that IMA is not
> initialized. Unfortunately, ima_must_appraise() does not check
> ima_policy_flag, so the kernel crashes when ima_match_policy()
> tries to evaluate the policy which is not loaded (ima_rules = NULL).
>
> if you add at the beginning of ima_must_appraise()
>
> if (!ima_policy_flag)
> return 0;
>
> the kernel should not crash.
Confirmed. The patch below fixed the Oops.
When you cook up a proper patch with that change, feel free to put my
tested-by tag
Reported-and-tested-by: Takashi Iwai <[email protected]>
Thanks!
Takashi
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -53,6 +53,9 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
if (!ima_appraise)
return 0;
+ if (!ima_policy_flag)
+ return 0;
+
security_task_getsecid(current, &secid);
return ima_match_policy(inode, current_cred(), secid, func, mask,
IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
On Fri, 2020-05-29 at 09:45 +0200, Takashi Iwai wrote:
> On Fri, 29 May 2020 09:33:34 +0200,
> Roberto Sassu wrote:
> >
> > > From: Takashi Iwai [mailto:[email protected]]
> > > On Thu, 28 May 2020 19:36:55 +0200,
> > > Roberto Sassu wrote:
> > > >
> > > > > From: [email protected] [mailto:linux-integrity-
> > > > > [email protected]] On Behalf Of Takashi Iwai
> > > > > On Thu, 28 May 2020 17:35:16 +0200,
> > > > > Takashi Iwai wrote:
> > > > > >
> > > > > > Hi Roberto,
> > > > > >
> > > > > > it seems that the recent changes in IMA in linux-next caused a
> > > > > > regression: namely it triggers an Oops when booting with the options
> > > > > > ima_policy=tcb ima_template_fmt='d-ng|n-ng|d|ng'
> > > > >
> > > > > And further experiment revealed that passing only
> > > ima_template_fmt=d
> > > > > is enough for triggering the bug. Other formats don't matter.
> > > > >
> > > > > (snip)
> > > > > > It's a KVM instance without any TPM stuff, just passed the options
> > > > > > above. I could trigger the same bug on a bare metal, too.
> > > > > >
> > > > > > Then I performed bisection and it spotted the commit:
> > > > > > 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7
> > > > > > ima: Switch to ima_hash_algo for boot aggregate
> > > > > >
> > > > > > Actually reverting this commit fixed the Oops again.
> > > > >
> > > > > So, looking at the fact above (triggered by "d") and this bisection
> > > > > result, it seems that the issue is specific to ima_eventdigest_init().
> > > > > The difference from others is that this has a check by
> > > > > ima_template_hash_algo_allowed(), and currently the check allows only
> > > > > SHA1 and MD5, while now SHA256 is assigned as default. So I tested
> > > > > adding SHA256 there like below, and it seems working.
> > > > >
> > > > > Hopefully I'm heading to a right direction...
> > > >
> > > > Hi Takashi
> > > >
> > > > boot_aggregate is the only entry for which there is no file descriptor.
> > > > The file descriptor is used to recalculate the digest if it is not SHA1
> > > > or MD5. For boot_aggregate, we should use instead
> > > > ima_calc_boot_aggregate(). I will provide a patch.
> > > >
> > > > I see that the .file member of event_data in
> > > > ima_add_boot_aggregate() is not initialized. Can you please try
> > > > to set .file to NULL?
> > >
> > > Tested and it didn't help. The field was already zero-initialized via
> > > C99-style initialization, I believe.
> >
> > Found the issue.
> >
> > ima_evendigest_init() returns an error and after that IMA is not
> > initialized. Unfortunately, ima_must_appraise() does not check
> > ima_policy_flag, so the kernel crashes when ima_match_policy()
> > tries to evaluate the policy which is not loaded (ima_rules = NULL).
> >
> > if you add at the beginning of ima_must_appraise()
> >
> > if (!ima_policy_flag)
> > return 0;
> >
> > the kernel should not crash.
>
> Confirmed. The patch below fixed the Oops.
> When you cook up a proper patch with that change, feel free to put my
> tested-by tag
> Reported-and-tested-by: Takashi Iwai <[email protected]>
Thank you for finding this bug. In this case, the "d" field has to be
limited to md5 or sha1 as the field is not large enough for other
algorithms. Just as the IMA Kconfig and the "ima_template=" boot
command line option prevent enabling a sha256 hash with the original
"ima" template, the "ima_template_fmt=" boot command line option
similarly needs to prevent a 'd' field being defined with a larger
digest. Failing to set the new template format should revert to using
the builtin default definition.
thanks,
Mimi