Re: Oops at boot with linux-next kernel with IMA boot options

[Mimi Zohar]

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:tiwai@xxxxxxx]
> > > On Thu, 28 May 2020 19:36:55 +0200,
> > > Roberto Sassu wrote:
> > > >
> > > > > From: linux-integrity-owner@xxxxxxxxxxxxxxx [mailto:linux-integrity-
> > > > > owner@xxxxxxxxxxxxxxx] 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 <tiwai@xxxxxxx>

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