Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 May 2020 13:06:34 +0200
From:   Daniel Kiper <daniel.kiper@oracle.com>
To:     Lukasz Hawrylko <lukasz.hawrylko@linux.intel.com>
Cc:     grub-devel@gnu.org, linux-kernel@vger.kernel.org,
        trenchboot-devel@googlegroups.com, x86@kernel.org,
        alexander.burmashev@oracle.com, andrew.cooper3@citrix.com,
        ard.biesheuvel@linaro.org, dpsmith@apertussolutions.com,
        eric.snowberg@oracle.com, javierm@redhat.com,
        kanth.ghatraju@oracle.com, konrad.wilk@oracle.com,
        krystian.hebel@3mdeb.com, michal.zygowski@3mdeb.com,
        mjg59@google.com, phcoder@gmail.com, piotr.krol@3mdeb.com,
        pjones@redhat.com, ross.philipson@oracle.com
Subject: Re: [GRUB PATCH RFC 00/18] i386: Intel TXT secure launcher
Message-ID: <20200507110634.2yvzirauq5md7d2q@tomti.i.net-space.pl>
References: <20200504232132.23570-1-daniel.kiper@oracle.com>
 <2dad6366d2fceb0a9e36f284a8ed5a8ed86d8756.camel@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2dad6366d2fceb0a9e36f284a8ed5a8ed86d8756.camel@linux.intel.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Łukasz,

On Tue, May 05, 2020 at 04:38:02PM +0200, Lukasz Hawrylko wrote:
> On Tue, 2020-05-05 at 01:21 +0200, Daniel Kiper wrote:
> > Hi,
> >
> > This is an RFC patchset for the GRUB introducing the Intel TXT secure launcher.
> > This is a part of larger work known as the TrenchBoot. Patchset can be split
> > into two distinct parts:
> >   - 01-12: preparatory patches,
> >   - 13-18: the Intel TXT secure launcher itself.
> >
> > The initial implementation of the Intel TXT secure launcher works. However,
> > there are still some missing bits and pieces, e.g.:
> >   - SINIT ACM auto loader,
> >   - lack of RMRR support,
> >   - lack of support for MLEs larger than 1 GiB,
> >   - lack of TPM 1.2 support.
> >   - various fixes and cleanups.
> >
> > Commands introduced by this patchset: tpm_type, slaunch, slaunch_module (not
> > required on server platforms) and slaunch_state (useful for checking platform
> > configuration and state; based on tboot's txt-stat).
> >
> > Daniel
> >
>
> Hi Daniel
>
> Your patch looks promising, however I have few concerns.

Below I will be referring to the Intel Trusted Execution Technology
(Intel TXT), Software Development Guide, December 2019, Revision 016.
So, the latest and greatest...

> In OS-MLE table there is a buffer for TPM event log, however I see that
> you are not using it, but instead allocate space somewhere in the

I think that this part requires more discussion. In my opinion we should
have this region dynamically allocated because it gives us more flexibility.
Of course there is a question about the size of this buffer too. I am
not sure about that because I have not checked yet how many log entries
are created by the SINIT ACM. Though probably it should not be large...

> memory. I am just wondering if, from security perspective, it will be
> better to use memory from TXT heap for event log, like we do in TBOOT.

Appendix F, TPM Event Log, has following sentence: There are no
requirements for event log to be in DMA protected memory – SINIT will
not enforce it.

I was thinking about it and it seems to me that the TPM event log does
not require any special protections. Any changes in it can be quickly
detected by comparing hashes with the TPM PCRs. Does not it?

> There is a function that verifies if platform is TXT capable
> -grub_txt_verify_platform(), it only checks SMX and GETSEC features.
> Although BIOS should enforce both VMX and VT-d enabled when enabling
> TXT, I think that adding these check here as redundancy may be a good

The TXT spec has the following pseudocode:

  //
  // Intel TXT detection
  // Execute on all logical processors for compatibility with
  // multiple processor systems
  //
  1. CPUID(EAX=1);
  2. IF (SMX not supported) OR (VMX not supported) {
  3. Fail measured environment startup;
  4. }

However, a few lines above you can find this:

  Lines 1 - 4: Before attempting to launch the measured environment, the
  system software should check that all logical processors support VMX and
  SMX (the check for VMX support is not necessary if the environment to be
  launched will not use VMX).

Hence, AIUI, I am allowed to check SMX only. And I do not think that the
bootloader should enforce VMX. If the kernel wants VMX then it should
check the platform config. The booloader should just look for features
which are really required to properly execute GETSEC[SENTER].

> idea. The same situation is with TPM presence.

The TPM presence is checked in init_txt_heap(). However, we can do it earlier.

> I suggest to add possibility to skip TXT launch when last boot ended
> with TXT error. This option can avoid boot loops when something goes
> wrong.

grub_txt_verify_platform() checks TXT_RESET.STS bit and fails if it is
set. This produces an error during boot. Well, but it does not prevent
it... :-( Probably I have to fix it...

> How will you read LCP from storage? I see that there is slaunch_module
> command that currently you are using only for loading SINIT. In the
> future it can be expanded to support LCP file too, what do you think?

I think that we should add e.g. slaunch_lcp command and do not overload
slaunch_module command. However, I am not planning support for it in the
near feature. I mean I will not be working on it...

> Do not forget to apply changes required by latest Intel's platforms, you
> should check following commits in TBOOT's repository: 2f03b57ffdba,
> fe2dddd742dc.

Sure, will take that into account.

Thank you for your comments,

Daniel

PS By the way, I found an issue in TXT spec. TXT.VER.FSBIF refers to
   TXT.VER.EMIF which does not exist in spec. I suppose that it is
   remnant from previous TXT spec versions. It seems to me that it
   should be changed to TXT.VER.QPIIF. TXT.VER.QPIIF descriptions
   properly, IMO, refers back to TXT.VER.FSBIF.