Re: [PATCH v2] ima: defer arch_ima_get_secureboot() call to IMA init time

From: Chester Lin <clin@suse.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Mimi Zohar <zohar@linux.ibm.com>,
	linux-efi <linux-efi@vger.kernel.org>,
	linux-integrity <linux-integrity@vger.kernel.org>,
	linux-security-module@vger.kernel.org,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	"open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" 
	<linuxppc-dev@lists.ozlabs.org>,
	jlee@suse.com
Subject: Re: [PATCH v2] ima: defer arch_ima_get_secureboot() call to IMA init time
Date: Wed, 14 Oct 2020 17:35:31 +0800	[thread overview]
Message-ID: <20201014093531.GA9408@linux-8mug> (raw)
In-Reply-To: <CAMj1kXFZVR46_oeYTxJ59q-7u+zFCFtOQuSQoiEzKLhXzpydow@mail.gmail.com>

Hi Ard & Mimi,

On Tue, Oct 13, 2020 at 06:59:21PM +0200, Ard Biesheuvel wrote:
> On Tue, 13 Oct 2020 at 18:46, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > [Cc'ing linuxppc-dev@lists.ozlabs.org]
> >
> > On Tue, 2020-10-13 at 10:18 +0200, Ard Biesheuvel wrote:
> > > Chester reports that it is necessary to introduce a new way to pass
> > > the EFI secure boot status between the EFI stub and the core kernel
> > > on ARM systems. The usual way of obtaining this information is by
> > > checking the SecureBoot and SetupMode EFI variables, but this can
> > > only be done after the EFI variable workqueue is created, which
> > > occurs in a subsys_initcall(), whereas arch_ima_get_secureboot()
> > > is called much earlier by the IMA framework.
> > >
> > > However, the IMA framework itself is started as a late_initcall,
> > > and the only reason the call to arch_ima_get_secureboot() occurs
> > > so early is because it happens in the context of a __setup()
> > > callback that parses the ima_appraise= command line parameter.
> > >
> > > So let's refactor this code a little bit, by using a core_param()
> > > callback to capture the command line argument, and deferring any
> > > reasoning based on its contents to the IMA init routine.
> > >
> > > Cc: Chester Lin <clin@suse.com>
> > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > Cc: James Morris <jmorris@namei.org>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > Link: https://lore.kernel.org/linux-arm-kernel/20200904072905.25332-2-clin@suse.com/
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > v2: rebase onto series 'integrity: improve user feedback for invalid bootparams'
> >
> > Thanks, Ard.  Based on my initial, limited testing on Power, it looks
> > good, but I'm hesistant to include it in the integrity 5.10 pull
> > request without it having been in linux-next and some additional
> > testing.  It's now queued in the next-integrity-testing branch awaiting
> > some tags.
> >

Tested-by: Chester Lin <clin@suse.com>

I have tested this patch on x86 VM.

* System configuration:
  - Platform: QEMU/KVM
  - Firmware: EDK2/OVMF + secure boot enabled.
  - OS: SLE15-SP2 + SUSE's kernel-vanilla (=linux v5.9) + the follow commits
    from linux-next and upstream:
    * [PATCH v2] ima: defer arch_ima_get_secureboot() call to IMA init time
      https://www.spinics.net/lists/linux-efi/msg20645.html
    * e4d7e2df3a09 "ima: limit secure boot feedback scope for appraise"
    * 7fe2bb7e7e5c "integrity: invalid kernel parameters feedback"
    * 4afb28ab03d5 "ima: add check for enforced appraise option"

* Logs with UEFI secure boot enabled:

  [    0.000000] Linux version 5.9.0-858-g865c50e1d279-1.g8764d18-vanilla (geeko@b
  uildhost) (gcc (SUSE Linux) 10.2.1 20200825 [revision c0746a1beb1ba073c7981eb09f
  55b3d993b32e5c], GNU ld (GNU Binutils; openSUSE Tumbleweed) 2.34.0.20200325-1) #
  1 SMP Wed Oct 14 04:00:11 UTC 2020 (8764d18)
  [    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-5.9.0-858-g865c50e1d279-1.
  g8764d18-vanilla root=UUID=5304c03e-4d8a-4d67-873a-32a32e57cdeb console=ttyS0,11
  5200 resume=/dev/disk/by-path/pci-0000:04:00.0-part4 mitigations=auto ignore_log
  level crashkernel=192M,high crashkernel=72M,low ima_appraise=off
  [    0.000000] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point regi
  sters'
  [    0.000000] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
  [    0.000000] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
  ....
  ....
  [    1.720309] ima: Secure boot enabled: ignoring ima_appraise=off option
  [    1.720314] ima: No TPM chip found, activating TPM-bypass!
  [    1.722129] ima: Allocated hash algorithm: sha256
  ....

> 
> Thanks. No rush as far as I am concerned, although I suppose Chester
> may want to rebase his arm64 IMA enablement series on this.

Yes, I've finished coding but still verifying it. As you have suggested,
My v2 patch will separate the get_sb_mode() from arch/x86 so that other
architectures can reuse it.

Thanks,
Chester

> 
> Suggestion: can we take the get_sb_mode() code from ima_arch.c in
> arch/x86, and generalize it for all EFI architectures? That way, we
> can enable 32-bit ARM and RISC-V seamlessly once someone gets around
> to enabling IMA on those platforms. In fact, get_sb_mode() itself
> should probably be factored out into a generic helper for use outside
> of IMA as well (Xen/x86 has code that does roughly the same already)
> 