Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members

[James Bottomley]

On Sun, 2023-10-29 at 15:42 +, Yao, Jiewen wrote:
> > I'd say that's pretty close. A reviewer role is a request for
> > keeping
> > the reviewer in the loop.
> 
> [Jiewen] I am disappointed on that.
> To me, that is NOT a real reviewer. See below description on what is
> "code review".
> https://google.github.io/eng-practices/review/
> https://about.gitlab.com/topics/version-control/what-is-code-review/

Well, that's what someone's view of what a patch review should consist
of, not what a reviewer's role in MAINTAINERS should be.

In general, you really don't want to force people to review patches,
because you'd like a reviewer to be familiar with the area and
comfortable with the patch.  So are you saying that anyone listed as a
reviewer in a particular area should be capable of reviewing any patch?
and further that they should be expected to review every patch? 
Because that's definitely not what the R role in the Linux Kernel would
mean.

I know that's not what happened to me in Confidential Computing,
because I had a very specific area around SEV and SEV-ES secret
injection and really had no familiarity at all with say the memory
acceptance patches.

> Our definition seems more like *a notification receiver*, instead of
> a real code reviewer. I would say, it is a very misleading
> definition.

Actually, I wouldn't, but then I'm more coming from a Linux Kernel
background.  To us, the reviewer list is simply a list of people git
blame might not find who might have the expertise to review the patch
but on whom there would be no expectation that they would review the
patch.

James



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110270): https://edk2.groups.io/g/devel/message/110270
Mute This Topic: https://groups.io/mt/102245264/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/AmdSev: stop using PlatformBootManagerLibGrub

[James Bottomley]

On Thu, 2023-05-04 at 17:08 +0200, Gerd Hoffmann wrote:
> On Thu, May 04, 2023 at 10:16:05AM -0400, James Bottomley wrote:
> > On Thu, 2023-05-04 at 15:32 +0200, Gerd Hoffmann wrote:
> > > Use PlatformBootManagerLib with PcdBootRestrictToFirmware
> > > set to TRUE instead.
> > > 
> > > Signed-off-by: Gerd Hoffmann 
> > > ---
> > >  OvmfPkg/AmdSev/AmdSevX64.dsc | 10 --
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> > > b/OvmfPkg/AmdSev/AmdSevX64.dsc
> > > index 943c4eed9831..b32049194d39 100644
> > > --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> > > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> > > @@ -153,6 +153,7 @@ [LibraryClasses]
> > >   
> > > UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriv
> > > erEntryPoint.inf
> > >   
> > > UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoin
> > > t/UefiApplicationEntryPoint.inf
> > >   
> > > DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/
> > > UefiDevicePathLibDevicePathProtocol.inf
> > > +  NvVarsFileLib|OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf
> > 
> > All additions apart from this look fine, but this one is a security
> > risk: EFI variables represent an unmeasured configuration for SEV
> > boot and, as such, can be used to influence the boot and
> > potentially reveal boot secrets, so the AmdSevPkg was designed to
> > have read only EFI variables that couldn't be subject to outside
> > influence.
> 
> NvVarsFileLib gets disabled already case PcdSecureBootSupported is
> set. Is that good enough?  If not I can extend that to also check
> PcdBootRestrictToFirmware.

I think pcd disabling is good enough, although usually secure boot
isn't enabled for this (problem sharing the signing key if the
variables have to be fixed inside the OVMF file), so it would need to
be a more universal PCD.  What we need to prevent is the addition of a
file on the edk2 partition (which is unencrypted) from being able to
influence the boot configuration.

James



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104031): https://edk2.groups.io/g/devel/message/104031
Mute This Topic: https://groups.io/mt/98683761/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/AmdSev: stop using PlatformBootManagerLibGrub

[James Bottomley]

On Thu, 2023-05-04 at 15:32 +0200, Gerd Hoffmann wrote:
> Use PlatformBootManagerLib with PcdBootRestrictToFirmware
> set to TRUE instead.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 943c4eed9831..b32049194d39 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -153,6 +153,7 @@ [LibraryClasses]
>   
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEn
> tryPoint.inf
>   
> UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/Ue
> fiApplicationEntryPoint.inf
>   
> DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/Uefi
> DevicePathLibDevicePathProtocol.inf
> +  NvVarsFileLib|OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf

All additions apart from this look fine, but this one is a security
risk: EFI variables represent an unmeasured configuration for SEV boot
and, as such, can be used to influence the boot and potentially reveal
boot secrets, so the AmdSevPkg was designed to have read only EFI
variables that couldn't be subject to outside influence.

James



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104009): https://edk2.groups.io/g/devel/message/104009
Mute This Topic: https://groups.io/mt/98683761/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [sbsigntools] [ANNOUNCE] sbsigntools version 0.9.5 available

[James Bottomley]

The fixes since 0.9.4 are

Andreas Schwab (1):
  sbsigntool: add support for RISC-V 64-bit PE/COFF images

Daniel Axtens (1):
  sbvarsign: do not include PKCS#7 attributes

James Bottomley (1):
  Add support for openssl-3

Jeremi Piotrowski (1):
  Fix openssl-3.0 issue involving ASN1 xxx_it

dann frazier (1):
  sbkeysync: Don't ignore errors from insert_new_keys()


The git repository is

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git

The tarball release is

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git/snapshot/sbsigntools-0.9.5.tar.gz

And the openSUSE build service build of various distributions is here:

https://build.opensuse.org/package/show/home:jejb1:UEFI/sbsigntools

James







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101374): https://edk2.groups.io/g/devel/message/101374
Mute This Topic: https://groups.io/mt/97719592/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 1/2] OvmfPkg: Change default to disable MptScsi and PvScsi

[James Bottomley]

On Wed, 2022-12-07 at 15:09 +0100, Ard Biesheuvel wrote:
> So at some point, these drivers will be removed rather than kept
> alive by the core team unless someone steps up.

How important is keeping them alive?  I can volunteer to "maintain"
them which I anticipate won't be much effort (plus I'm used to looking
after obsolete SCSI equipment).  The hardware is obsolete, so the
mechanics of their emulation isn't going to change, the only potential
risk is changes in the guest to host transmission layer that breaks
something.  On the other hand, I've got to say I use virtio-scsi in all
my VM testing environments, so the maintenance will likely only be
reacting when someone else reports a problem.

James



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97082): https://edk2.groups.io/g/devel/message/97082
Mute This Topic: https://groups.io/mt/92635541/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] measurement to command-line/initrd for loading kernel via -kernel option

[James Bottomley]

[pjones added because he's done a huge amount of work to get shim to
measure stuff correctly]
On Tue, 2022-09-20 at 13:24 +, Lu, Ken wrote:
> > > Hi Ard, I think it better let creator to measure instead of
> > > consumer to measure
> > like today's implementation in grub[1]. The creator here means who
> > load/create it. In direct boot, it is OVMF read kernel command line
> > and initrd image. In grub boot, it is grub2.  Because the number of
> > consumer like Linux kernel could be more than 1, but the creator is
> > single.
> > 
> > I agree with this in principle.
> 
> So you are not against to do measurement in loader like current does
> in grub and OVMF, correct? I think it is OK even do twice
> measurements on cmdline and initrd for the corner case.
> In past month, I just submit patch in grub to do CC measurement at 
> https://git.savannah.gnu.org/cgit/grub.git/commit/?id=4c76565b6cb885b7e144dc27f3612066844e2d19

Wait, we have two separate cases: when the kernel boots via grub, which
is not direct boot and grub measures eveything and when we do direct
boot and grub is not involved.  Ideally, we should be able to get to a
stable PCR8,9 for measurement, but grub isn't hugely helpful there
since it dumps every grub command into the PCRs so direct boot can
never match that whatever the EFI stub does.  The TCG spec isn't very
helpful on some things, but it is very clear that once you've measured
something, you don't measure it again, so we do want to avoid measuring
the same thing twice.

> 
> > However, there are corner cases that we would like
> > to cover, such as booting Linux from the EFI shell. 
> 
> I remember Bottomley or someone mentioned to use CONFIG_CMDLINE and
> CONFIG_INITRAMFS_SOURCE, such as 
> https://blog.decentriq.com/swiss-cheese-to-cheddar-securing-amd-sev-snp-early-boot-2/
> for this corner case, especially for confidential container use case
> without grub.

Well, you know, when you talk of the devil he bites your heels ...

Part of the problem is that if you look at the protocol, the LoadImage
measurement seems not to measure the command line.  If we can fix that,
then we can get something that will work both with direct boot (cmdline
is passed to the image) as well as direct executions of the kernel from
the EFI shell.  I think that's what we should aim for.  It would be too
disruptive now to try to get grub also to measure thisorrectly.

I think the key is agreeing with TCG that the argument list of an
executable, loaded by LoadImage should be measured separately.  There
are parts of the spec that hint at this, but by and large it seems to
assume that the measurement of the boot volume entry (which does
contain the command line [usually empty] is sufficient).

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93992): https://edk2.groups.io/g/devel/message/93992
Mute This Topic: https://groups.io/mt/93737108/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Question about signed uefi vars at OS level

[James Bottomley]

On Tue, 2022-07-26 at 10:09 -0300, Rafael Machado wrote:
> Hey everyone
> 
> I have a question for the experts.
> 
> Suppose I have a BIOS feature that can be set from the OS via some OS
> application (.exe) that calls the runtime services set variable ().
> 
> To set this feature I have a UEFI var, that during DXE is processed
> by some uefi module.
> 
> In case I define this UEFI var as signed var
> (EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS or
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCES), at my OS
> application I will have to add the signing key, so it would be
> possible to create new signed data to change the uefi variable as
> needed from the OS level.
> 
> So my question is:
> What is the correct way of creating a UEFI variable that is protected
> and that can be changed, by authorized person only, from OS level
> without the need of embedding my secret at the OS application (.exe)?

You don't give your use case, so it's hard to answer the above. 
However, the signing process of the update must be guarded because of
the need to keep the key secret, so update bundles are usually created
away from the system to be updated to preserve this.  If you want your
application to make arbitrary updates while it's running, you probably
don't want to be using signed variables.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91848): https://edk2.groups.io/g/devel/message/91848
Mute This Topic: https://groups.io/mt/92626771/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.

[James Bottomley]

On Tue, 2022-05-10 at 12:40 +0200, Gerd Hoffmann wrote:
> On Mon, May 09, 2022 at 09:41:02AM -0400, James Bottomley wrote:
> > On Mon, 2022-05-09 at 12:03 +, Yao, Jiewen wrote:
> > > It is possible to switch to other crypt lib.
> > > 
> > > For example, the *mbedtls* version POC can be found at 
> > > https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg
> > > The advantage is: the size is much smaller.
> > > The disadvantage is: some required functions are not available,
> > > such as PKCS7. 
> > 
> > Perhaps as a first step, we should look at our options.  I would
> > say missing functionality is problematic, but not necessarily a
> > killer: we'd have to help the chosen project develop the capability
> > and figure out how to maintain the fork while it was going
> > upstream.
> 
> I don't feel like entering the business of maintaining a tls
> library ...

Me neither, but we already maintain some exceptions like the logic to
break the X509 chain for UEFI, so if we had to tinker around the edges,
I think it's feasible.

> > Other libraries could be:
> > 
> > wolfssl
> 
> Hmm?  Apparently no git repository?

https://github.com/wolfSSL/wolfssl

> > gnutls
> 
> Might be a issue license-wise.

It's LGPL and our use case entirely embeds it so we're using it within
the licence terms.  Since we're effectively linking statically, it
provides a slight problem for distributions because they need to
facilitate relinking, but that's just a nasty mechanical problem

> 
> > boringssl
> 
> Looks like an option worth investigating.
> 
> The "designed to meet Google's needs" and "not intended for general
> use" notes in the toplevel README don't look that great
> though.  Might turnons out to be be difficult to get changes needed
> for edk2 merged (hasn't been a problem so far for me with openssl).

Right, boringssl is effectively Google's fork of openssl for android
which they did because they could never get the openssl people to
accept their patches or pay attention to the embedded bloat problem
(which is currently our problem).

> > LibreSSL
> 
> There was some hype around it after it was forked from openssl in the
> heartbleed aftermath.  More recent news are less enthusiastic:
> https://lwn.net/Articles/841664/

Yes, I'm not hugely enthused about LibreSSL, but I think we do need to
list all the alternatives.

> Another possible option would be to add openssl3 as alternative
> OpensslLib implementation, so platforms can pick the one or the
> other depending on size constrains.

Really, no, we can't.  That would leave the space constrained use case
non functional when openssl 1 goes EOL.  We have to make openssl 3 work
for everything or consider a new crypto provider.

> I've also experimented a bit with CryptoPkg/Driver.  It's not a
> clear win, at least for OVMF.
> 
> PEI FV is larger in any case.  Seems LTO works very well for the
> few hashes needed by TPM support code, and so the overhead added
> by using the crypto service protocol instead of direct linking is
> much larger than the savings by sharing code.
> 
> DXE FV is smaller in the builds with secure boot and smm support,
> seems with the large tls codebase included we have enough wins by
> sharing the crypto code then, so the protocol overhead is worth
> the effort.
> 
> I'm wondering where the crypto algorithm selection in
> CryptoPkg/CryptoPkg.dsc comes from though, specifically for
> MIN_DXE_MIN_SMM.  Why is the crypto feature selection identical
> for DXE and SMM?  Specifically why TLS is enabled for SMM?

I think the idea was that using a static openssl library you could link
the various algorithm providers with it and make small pieces, but that
didn't work out well for openssl which has a massive startup
requirement.  No idea why SMM would require TLS ... I can look at the
code.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89662): https://edk2.groups.io/g/devel/message/89662
Mute This Topic: https://groups.io/mt/90832153/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.

[James Bottomley]

On Mon, 2022-05-09 at 12:03 +, Yao, Jiewen wrote:
> It is possible to switch to other crypt lib.
> 
> For example, the *mbedtls* version POC can be found at 
> https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg
> The advantage is: the size is much smaller.
> The disadvantage is: some required functions are not available, such
> as PKCS7. 

Perhaps as a first step, we should look at our options.  I would say
missing functionality is problematic, but not necessarily a killer:
we'd have to help the chosen project develop the capability and figure
out how to maintain the fork while it was going upstream.  PKCS#7 is
pretty huge, though, it's the entire Cryptographic Message Syntax so I
think us having to develop that for mbedtls makes that one a non
starter.

Other libraries could be:

wolfssl
gnutls
boringssl
LibreSSL

They all seem to do pkcs#7.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89615): https://edk2.groups.io/g/devel/message/89615
Mute This Topic: https://groups.io/mt/90832153/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.

[James Bottomley]

On Mon, 2022-05-09 at 13:27 +0200, Gerd Hoffmann wrote:
[...]
> > 1) Please keep the good work to enable OPENSSL3.0 in your personal
> > branch.
> > 2) If you have some way to control the size, then do it. If there
> > is no much size difference by default, then you can submit to EDKII
> > directly.
> 
> I suspect I wouldn't get it down to 1.1.1 levels even if I find some
> ways to make it smaller than it is in my branch today.  The code for
> the new "provider" concept simply needs space and I think it also
> makes LTO optimization less effective.

Having just looked into converting engine code to provider code, I
would concur with this.  The design of providers, with their many to
many functional mappings, seems designed to promote code bloat.

> Maybe creating our own crypto providers which include only the
> algorithms actually needed by edk2 gets the size down a bit.

What about switching to a different crypto backend?  Since we don't
expose any openssl APIs at all and we wrapper everything we do expose,
it should be possible to switch to one of the non-openssl (or forked
from openssl) variants that value size, like mbedtls or boringssl?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89610): https://edk2.groups.io/g/devel/message/89610
Mute This Topic: https://groups.io/mt/90832153/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3 5/9] OvmfPkg/IntelTdx: Measure Td HobList and Configuration FV

[James Bottomley]

On Wed, 2022-04-20 at 10:16 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Yes for validation (aka sanity-checking the fields, etc).
> > > But for measurement I don't see why the ordering matters.
> > > Whenever you do that before or after consuming the TdHob
> > > should not make a difference.
> > 
> > [Jiewen] I disagree. The order matters from security perspective.
> > If you use it, there is risk that the buggy code will compromise
> > the system before you have chance to measure it.
> 
> Measurement will only record hashes for verification later on.
> It will not prevent running possibly buggy/compromised code.

This is true, but this is also the design of measured boot: it's for
proof of correctness (or not) after the fact.  Secure boot is more the
technology that can prevent boot.

> So, no matter what the order is, you'll figure the system got
> compromised after the fact, when checking the hashes later, and in
> turn take actions like refusing to hand out secrets to the
> compromised system.

Not if the code falsifies the measurement both in the log and to the
TPM.  That's why the requirement of measured boot is you start with a
small rom based root of trust, which can't be updated because it's in
rom.  It measures the next stage (usually PEI) before executing it so
that the measurement in the TPM would change if the next stage (which
is often in flash) got compromised, so any tampering is certain to be
detected and if the compromised code tries to falsify the log, the log
now wouldn't match the TPM, so it can't evade detection.

The requirement from the TCG is that the trusted code measures the
untrusted code through the TPM before executing it to get this
proveable detection of tampering.  The TCG allows you to be elastic
about when you record the measurements in the log as long as you
measure through the TPM at the correct points.

The above applies equally to TPM substitutes like the TDX msrs.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89139): https://edk2.groups.io/g/devel/message/89139
Mute This Topic: https://groups.io/mt/90531017/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] Introduce TdProbe in MdePkg

[James Bottomley]

On Wed, 2022-04-13 at 17:08 +0800, Min Xu wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902
> 
> Bad IO performance in SEC phase is observed after TDX features was
> introduced. (after commit b6b2de884864 - "MdePkg: Support mmio for
> Tdx guest in BaseIoLibIntrinsic").
> 
> This is because IsTdxGuest() will be called in each MMIO operation.
> It is trying to cache the result of the probe in the efi data
> segment. However, that doesn't work in SEC, because the data segment
> is read only (so the write seems to succeed but a read will always
> return the original value), leading to us calling TdIsEnabled() check
> for every mmio we do, which is causing the slowdown because it's very
> expensive.
> 
> TdProbe is introduced in this patch-set. It is called in
> BaseIoLibIntrinsicSev instead of IsTdxGuest. There are 2 versions of
> the TdProbeLib. Null instance of TdProbe always returns TD_PROBE_NON.
> Its OvmfPkg version checks the Ovmf work area to determine the Td
> guest type.

I tested this out with the TPM code: it restores pretty much all of the
lost performance, thanks!

Tested-by: James Bottomley 

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88867): https://edk2.groups.io/g/devel/message/88867
Mute This Topic: https://groups.io/mt/90436749/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Regression: 100x I/O performance slowdown in SEC phase caused by TDX

[James Bottomley]

I'm using a SEC phase which has a TPM driver to experiment with sorting
out measured boot, which is how I noticed (usually SEC doesn't do MMIO)
.  What I'm seeing is after commit b6b2de884864 ("MdePkg: Support mmio
for Tdx guest in BaseIoLibIntrinsic") we get a massive slowdown of
about 100x in TPM performance.  The reason seems to be this addition to
the mmioreadX/mmiowriteX code:

 MemoryFence ();
-*(volatile UINT16 *)Address = Value;
+
+if (IsTdxGuest ()) {
+  TdMmioWrite16 (Address, Value);
+} else {
+  *(volatile UINT16 *)Address = Value;
+}
+
 MemoryFence ();


The problem is that IsTdxGuest () has this structure:

BOOLEAN
EFIAPI
IsTdxGuest (
  VOID
  )
{
  if (mTdxProbed) {
return mTdxEnabled;
  }

  mTdxEnabled = TdIsEnabled ();
  mTdxProbed  = TRUE;

  return mTdxEnabled;
}

Which is trying to cache the result of the probe in the efi data
segment.  However, that doesn't work in SEC, because the data segment
is read only (so the write seems to succeed but a read will always
return the original value), leading to us calling TdIsEnabled() check
for every mmio we do, which is causing the slowdown because it's very
expensive.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88800): https://edk2.groups.io/g/devel/message/88800
Mute This Topic: https://groups.io/mt/90427994/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] TDX patches have broken edk2 bisectability in OVMF

[James Bottomley]

I've identified a serious performance regression in recent edk2, so
I've been trying to identify it by bisection, but it seems that the TDX
patches have broken bisection in edk2.  You can see this by trying to
checkout b6b2de884864 and build it.  It will give you

Active Platform  = /home/jejb/git/edk2/OvmfPkg/OvmfPkgX64.dsc
.

build.py...
/home/jejb/git/edk2/OvmfPkg/OvmfPkgX64.dsc(...): error 4000: Instance of 
library class [TdxLib] is not found
in 
[/home/jejb/git/edk2/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf]
 [X64]
consumed by module [/home/jejb/git/edk2/OvmfPkg/Sec/SecMain.inf]
 

I think I can work around this, but it makes bisection extremely
painful, please don't do it again ...

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88799): https://edk2.groups.io/g/devel/message/88799
Mute This Topic: https://groups.io/mt/90427519/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Does anyone know why the measured boot log seems to be recording the hash of PEIFV wrongly?

[James Bottomley]

When I do a measured boot of OVMF, I get a load of records including
the two EV_EFI_PLATFORM_FIRMWARE_BLOB events, which, according to the
code in Tcg2Pei.c are supposed to be measuring PEIFV and DXEFV from the
uncompressed MEMFD.  However, when I compare the hashes against the
build artifacts, the DXEFV matches, so is correctly measured.  However
the PEIFV doesn't match ... it's like something modified the contents
before the Tcg2Pei.c measurement is taken.

Does anyone know what this modification to PEIFV is?  My next step
would be to go digging in the PEIFV at the time of measurement to see
if I can find the change, but I figured that asking first might be a
lot less work ...

Thanks,

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88273): https://edk2.groups.io/g/devel/message/88273
Mute This Topic: https://groups.io/mt/90143163/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

[James Bottomley]

On Wed, 2021-11-24 at 14:03 +, Yao, Jiewen wrote:
> James
> I am sorry that it is hard for me to understand your point.
> 
> To be honest, I am not sure what is objective on the discussion.
> Are you question the general threat model analysis on UEFI PI
> architecture?

The object is for me to understand why you think eliminating PEI
improves security because I think it moves it in the opposite
direction.

> Or are you trying to persuade me we should include PEI in TDVF,
> because you think it is safer to add code in PEI ?
> Or something else?
> 
> Please enlighten me that.

Somewhere a decision was taken to remove PEI from the OVMF that is used
to bring up TDX on the grounds of "improving security".  I'm struggling
to understand the rationale for this.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84040): https://edk2.groups.io/g/devel/message/84040
Mute This Topic: https://groups.io/mt/86739864/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

[James Bottomley]

On Wed, 2021-11-24 at 11:08 +, Yao, Jiewen wrote:
> > -Original Message-
> > From: Gerd Hoffmann 
[...]
> > There isn't much external input to process in PEI phase.  Virtual
> > machines are a bit different than physical machines.  They need to
> > process some input from the host here which describes the virtual
> > hardware so they can initialize it properly.  For example parse the
> > etc/e820 fw_cfg file to figure how much memory is installed (or
> > parse the td hob in case tdx is used).
> > 
> > That platform-specific code for virtual machine initialization must
> > do careful sanity checking when you don't want trust the VMM of
> > course. Whenever that code lives in SEC or PEI doesn't change the
> > picture much though.

I don't disagree on this because we don't have a rom root of trust in
OVMF ... however it matters for most of the rest of edk2

> [Jiewen] I am not sure what information you are trying to convey.
> I never said that TDVF don’t need check the input after remove PEI.
> The check is always needed, no matter in PEI or SEC. I totally agree.
> 
> However, what I want to say is PEI has much more unnecessary code
> than SEC.
> You never know the quality of the those unnecessary code. Maybe it
> has a security bug, but it is just not triggered.
> For example, can you guarantee PEI code has not bug? Or DXE IPL has
> not bug?

Code removal to thin down the image is orthogonal to the location of
that code ... I don't think anyone objected to securing the path
through PEI by reducing unnecessary code; the doubt is that the
elimination of PEI somehow improves security.

> 
> What I did is a process of risk management - if PEI is removed, I
> don’t need care the risk brought by PEI.

Well, as I said above, you can remove the unnecessary code in PEI (and
SEC and DXE).  However, once you've done that, you don't eliminate risk
by removing PEI because all you do is move the necessary code that was
in PEI to somewhere else.  If you move it to SEC, I agree with Gerd
that it doesn't alter the security position much because SEC is a low
exposure domain like PEI.  If you move it to DXE it actually decreases
your security measurably because DXE is a high exposure security
domain.

> Unless you can prove that the risk of PEI is zero, then the risk is
> there.

But you don't make it zero by eliminating PEI, you simply move the risk
elsewhere because the necessary code still has to execute.

> > > 2. "bugs in PEI code can't be used to exploit the system when it
> > > has transitioned to the DXE domain."
> > > [Jiewen] I disagree. A bug in PEI code may already modify the
> > > HOB, while the HOB is an architecture data input for DXE.
> > > If DXE relies on some malicious data from PEI, DXE might be
> > > exploited later.
> > 
> > Attacking PEI is harder though because the external input it
> > processes is limited when compared to DXE.
> > 
> > Once you are transitioned to DXE you can't call into PEI Modules
> > any more.
> > 
> > So, how would an attacker trick PEI code into modifying HOBs (or
> > carrying out other actions under attackers control)?
> 
> [Jiewen] I would disagree " Attacking PEI is harder though because
> the external input it processes is limited when compared to DXE " 
> First, the number of extern input != the difficulty of the attack.
> Second, the number of extern input != the severity of the
> consequence.

Attacking PEI is harder than attacking DXE because of the limited
exposure to external influences. It doesn't mean it can't be done but
it does mean exploiting the same code can be harder or impossible in
PEI compared to DXE.

The key point is there are some classes of bug that can't be exploited
in PEI because of the limited exposure.  These bugs can be exploited in
DXE because of the wider exposure.  This is why moving code from DXE to
PEI increases the risk.

> On how to trick PEI, if you search the public UEFI BIOS attack
> history in the web, you can find example.
> The attack simply used an integer overflow, to cause buffer overflow.
> And the buffer overflow can be used to inject shellcode, then gain
> the execution privilege.
> Finally, it can control the whole system.
> 
> Modifying HOB is not a difficult task, as long as there is proper
> vulnerability, being used to gain a memory write.
> 
> In security world, we always say: Even if you cannot do it, you
> cannot assume other people cannot do it.
> Unless you can prove it cannot be done. Otherwise, anything might be
> possible.
> 
> That is why people are in favor of crypto notation and formal
> verification to prove something is correct.

There are many ways of improving security.  Formal verification has its
place, but grows exponentially harder with the complexity of the
system.  Separation into multiple security domains is also a common
technique (which also facilitates formal verification because it
reduces the state space).  What I worry about is that elimination of
one of the UEFI security domains 

Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

[James Bottomley]

On Tue, 2021-11-23 at 15:10 +, Yao, Jiewen wrote:
> I would say the PEI owns the system and all memory (including the
> DXE). 
> 
> A bug in PEI may override the loaded DXE memory or the whole system.

That's not the correct way to analyse the security properties.  From
the security point of view this is a trapdoor system: once you go
through the door, you can't go back (the trapdoor being the jump from
PEI to DXE).  The trapdoor isolates the domains and allows you to
analyse the security properties of each separately.  It also allows
separation of exposure ... which is what we use in this case: the PEI
domain has very limited exposure, it's the DXE domain that has full
exposure but, because of the trapdoor, bugs in PEI code can't be used
to exploit the system when it has transitioned to the DXE domain.

> In history I did see PEI security issues. 
> Some security issue in PEI caused system compromised completely. You
> even have no chance to run DXE. 

The security domain analysis above doesn't mean no bug in PEI is ever
exploitable but it does mean that there are fewer exploitability
classes in PEI than DXE because the security domain is much less
exposed.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83951): https://edk2.groups.io/g/devel/message/83951
Mute This Topic: https://groups.io/mt/86739864/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

[James Bottomley]

On Tue, 2021-11-23 at 14:36 +, Yao, Jiewen wrote:
> > This strict isolation between DXE and PEI means that once we're in
> > DXE, any bugs in PEI can't be exploited to attack the DXE
> > environment.  
> 
> [jiewen] I would disagree the statement above. 
> There is not strict isolation. Actually no isolation at all.
> The DXE is loaded by PEI. 

Not in OVMF ... DXE and PEI are actually loaded by SEC.  PEI eventually
jumps to execute DXE but that's after all its own tasks are completed.

> A bug in PEI has global impact and it can definitely be used to
> attack the DXE.

Only if it can be exploited.  Moving things to PEI is mitigating the
exploitability not the bugs.  The point about exploitability and PEI is
that it doesn't read any config files, it can't execute any EFI
binaries and it has no Human Interface modules so can't be influenced
even by a physically present attacker.  No ability to influence is what
removes the ability to exploit.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83949): https://edk2.groups.io/g/devel/message/83949
Mute This Topic: https://groups.io/mt/86739864/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

[James Bottomley]

On Tue, 2021-11-23 at 13:07 +, Yao, Jiewen wrote:
> Comment below only:
> 
> > I am persuaded to let config-a adopt the OVMF way, because the
> > threat model of config-A is same as the normal OVMF.
> > But config-B is NOT.
> > Different threat model drives different solution.
> > I completely don't understand why they must be same.
> 
> I don't understand why you want separate them.  Improving OvmfPkg
> security is good for both OVMF and TDVF.  For TDVF it is clearly much
> more important due to the different threat model, but it is good for
> OVMF too.  Likewise edk2 in general.
> 
> [Jiewen] My answer is very clear as I mentioned multiple times.
> The threat model is different.
> IMHO, talking about "Improving OvmfPkg security" without a clear
> threat model is *meaningless*.
> All mitigation must be based upon threat mode and security objective.
> Otherwise, what you are doing might be either unnecessary or
> insufficient.

Can we take a step back and look at the bigger picture.  The way EFI is
supposed to operate, according to the architecture model:

https://uefi.org/sites/default/files/resources/PI_Spec_1_7_final_Jan_2019.pdf
(Figure 1 and Table 4)

is that SEC is supposed to be as small and compact as possible, so it
could be ROM installed without worrying about upgrade issues.  It's job
is only to get the CPU initialized to the point it can run PEI, measure
PEI and transfer control.  Security depends on the smallness of SEC
because if it's rom installed (as a root of trust ought to be) it can't
be upgraded to fix a bug.

PEI is supposed to initialize the hardware: set up the CPU, memory
Application Processors and board configuration so we're in a known
state with described features for DXE.  It then measures DXE (only if
SEC didn't measure it) and hands off to DXE.  PEI code is designed not
to interact with anything except setup to minimize external
exploitation of internal bugs.

DXE is supposed to contain only runtime code, including device drivers.

The security point here is that the job of PEI is over as soon as it
hands off to DXE, so at that point all the PEI code could be discarded
(it usually isn't, but it could be).

This strict isolation between DXE and PEI means that once we're in DXE,
any bugs in PEI can't be exploited to attack the DXE environment.  This
allows us to minimize DXE which is where most of the main risk of
configuration based exploitation is.

In this security model, you increase security by moving as much code as
you can from DXE to PEI because the true attack surface is DXE.

To a lot of us, cutting out PEI, which requires redistributing code
into DXE sounds like an increase not a reduction in the attack surface
of the code.  You can argue that OVMF doesn't obey the model above and
has a lot of initialization code in DXE anyway, but then the correct
route would be to fix our PEI/DXE separation to recover the security
properties.

> > If you force me to add PEI to config-B. Finally, that causes TDVF
> > config-B is compromised due to an issue in PEI.
> > Who will take the responsibility?  Will you or RedHat take that?
> 
> Bugs happen.  There could also be bugs in the additional SEC code you
> need for platform setup in a non-PEI configuration ...
> 
> [Jiewen] I agree. That is why we need smaller code.
> We can just focus on review that small portion of code what we have
> written for TDVF, instead of the whole PEI.
> We have process to review and maintain the extra TDVF code.

This depends ... if you agree with the security model outlined above,
bugs in PEI are less of a problem because they can't be exploited.  Or
do you not agree with this security model?

Security isn't about total bug elimination, it's about exploit
elimination.  You fix a security vulnerability either by fixing the bug
that can be exploited or removing the ability to exploit it.  This is
the reason for technologies like NX ... they don't stop people
exploiting bugs to write code to the stack, but they do mean that once
you have the code on the stack you can no-longer execute it and the
attackers have to move on to other means of gaining control.

The great thing about exploit elimination vs bug elimination is the
former can usually be done on a whole class of bugs and the latter
requires a whack-a-mole per bug type approach.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83947): https://edk2.groups.io/g/devel/message/83947
Mute This Topic: https://groups.io/mt/86739864/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v12 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status

[James Bottomley]

On Fri, 2021-11-12 at 01:27 +, Ni, Ray wrote:
[...]
> > +
> > +  return (CurrentAttr == Attr);
> 
> 2. I guess a "BOOLEAN" type cast is needed.

It shouldn't.  Unless there's a major screw up in the way BOOLEAN works
in the UEFI API, all logic operations should already be of type BOOLEAN
and if they're not that would be what needs fixing.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83692): https://edk2.groups.io/g/devel/message/83692
Mute This Topic: https://groups.io/mt/86969148/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option

[James Bottomley]

On Fri, 2021-10-22 at 11:48 -0400, Stefan Berger wrote:
> On 10/22/21 11:01 AM, James Bottomley wrote:
> > On Fri, 2021-10-22 at 10:52 -0400, Stefan Berger wrote:
> > 
> > >   along with the quote on the  sha1 bank.
> > The validator shouldn't accept that quote ... it should require a
> > quote covering all banks.  This is the point: you can't fake the
> > quote and the quote should cover all banks to assure you that
> > unextended banks really are.
> 
> Unfortunately this seems to be flawed on the TPM2_Quote level...

In what way?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82541): https://edk2.groups.io/g/devel/message/82541
Mute This Topic: https://groups.io/mt/86487987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option

[James Bottomley]

On Fri, 2021-10-22 at 10:52 -0400, Stefan Berger wrote:
> On 10/22/21 10:17 AM, James Bottomley wrote:
> > On Fri, 2021-10-22 at 09:13 -0400, Stefan Berger wrote:
> > > On 10/22/21 8:40 AM, James Bottomley wrote:
> > > 
> > > > On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
> > > > > On 10/22/21 7:49 AM, James Bottomley wrote:
> > > > > > On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> > > > > > [...]
> > > > > > > I see this also but when I get into Linux and run
> > > > > > > tpm2_pcrread I see the SHA1 bank active but not having
> > > > > > > received any PCR extensions from the firmware, which is
> > > > > > > not supposed to happen.
> > > > > >  
> > > > > > That's not entirely correct: the TCG firmware profile just
> > > > > > requires us to log through at least one bank; it doesn't
> > > > > > require that all active banks be logged.  I've got several
> > > > > > physical systems with three active banks but only one or
> > > > > > two measured through.
> > > > >
> > > > > The problem with this is that you can then fake measured boot
> > > > > on that system using it's unused SHA1 bank and extend into it
> > > > > whatever you want and create a fake log along with it and the
> > > > > quote is going to look alright.
> > > > 
> > > > I don't think you can.  The measured boot PCRs in unused banks
> > > > should always be their default values and the measurement
> > > > software should check for this.  So on a system that only uses
> > > > the sha256 bank, the sha1 bank PCR0-7 should be all zeros ...
> > > > if they aren't this should be a measurement failure.
> > > > 
> > > > That means that if you try to replace the sha256 agile log with
> > > > one containing fake sha1 entries, the attestation still fails
> > > > because the sha256 bank doesn't have default entries.
> > >  
> > > You can still pretend that your system only has an active SHA1
> > > bank and serve the fake log.
> >  
> > Which "You" can fake a TPM quote?  The whole design of the TPM
> > system is supposed to be that what goes into the TPM can't be
> > erased, only updated and we can get definitive proof of the values
> > using a quote.
>  
> What I meant is the admin runs TPM2_PCR_Extend on PCRs 0-7 of the
> unused sha1 bank and extends it with known good values and has a log
> that goes with it and presents these to a validator

Yes, I got all that.

>  along with the quote on the  sha1 bank.

The validator shouldn't accept that quote ... it should require a quote
covering all banks.  This is the point: you can't fake the quote and
the quote should cover all banks to assure you that unextended banks
really are.

> > You can fake the log to be sha1 only but you can't make it match
> > the quote that includes the sha256 banks.
> 
> Yes, that's right. The client must insist that the sha256 bank, and
> any other possible bank, is quoted so that the system cannot just
> pretend that it only has a XYZ [sha1] bank (unlikely for TPM 2),

Impossible per the TPM spec.

>  and ABC banks [sha256] doesn't exist there, even though the SHA256
> matches the true log. A quote by itself doesn't quote all the banks.
> You have to select which banks to quote and the client needs to have
> some control over that it seems to for sure see what the true
> firmware did.

Hey, I'm not going to disagree that the TPM system leaves many ways for
people to shoot themselves in the foot.  The only point I'm making is
that if you use it correctly (which I fully accept is somewhat complex)
you quote all banks and thus can't be tricked into accepting a fake log
through a bank unextended by firmware.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82528): https://edk2.groups.io/g/devel/message/82528
Mute This Topic: https://groups.io/mt/86487987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option

[James Bottomley]

On Fri, 2021-10-22 at 09:13 -0400, Stefan Berger wrote:
> On 10/22/21 8:40 AM, James Bottomley wrote:
> 
> > On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
> > > On 10/22/21 7:49 AM, James Bottomley wrote:
> > > > On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> > > > [...]
> > > > > I see this also but when I get into Linux and run
> > > > > tpm2_pcrread I see the SHA1 bank active but not having
> > > > > received any PCR extensions from the firmware, which is not
> > > > > supposed to happen.
> > > > 
> > > > That's not entirely correct: the TCG firmware profile just
> > > > requires us to log through at least one bank; it doesn't
> > > > require that all active banks be logged.  I've got several
> > > > physical systems with three active banks but only one or two
> > > > measured through.
> > >   
> > > The problem with this is that you can then fake measured boot on
> > > that system using it's unused SHA1 bank and extend into it
> > > whatever you want and create a fake log along with it and the
> > > quote is going to look alright.
> > 
> > I don't think you can.  The measured boot PCRs in unused banks
> > should always be their default values and the measurement software
> > should check for this.  So on a system that only uses the sha256
> > bank, the sha1 bank PCR0-7 should be all zeros ... if they aren't
> > this should be a measurement failure.
> > 
> > That means that if you try to replace the sha256 agile log with one
> > containing fake sha1 entries, the attestation still fails because
> > the sha256 bank doesn't have default entries.
> 
> You can still pretend that your system only has an active SHA1 bank
> and serve the fake log.

Which "You" can fake a TPM quote?  The whole design of the TPM system
is supposed to be that what goes into the TPM can't be erased, only
updated and we can get definitive proof of the values using a quote. 
You can fake the log to be sha1 only but you can't make it match the
quote that includes the sha256 banks.

> at that trusted boot log, SHA1 PCR 0-7 state, and quote then?

You don't just quote the bank you think is being logged ... you should
quote all banks of the TPM; that way you can't be duped in this
fashion.

> > > > >So I think you should drop this patch and I'll change the
> > > > > set of active PCR banks on the swtpm_setup level.
> > > >   
> > > > Even if the firmware deactivated the sha1 bank, the kernel
> > > > expectation problem is still going to exist.
> > >  
> > > Is that older Linux kernels or which part still requires sha1? A
> > > pointer would be good. I would have to revert the change to not
> > > activat ethe SHA1 bank from swtpm_setup if that's going to create
> > > headaches. I thought some hardware TPM 2's today are only
> > > providing a SHA256 bank and so it shouldn't be a problem.
> >  
> > The problem is IMA: it's hash is a kernel config parameter which
> > defaults to sha1.  It then tries to calculate the boot aggregate
> > over the configured hash bank and doesn't check if it's unused.
> > 
> > What IMA should probably be doing is working out which bank the
> > bios is logging through and using that as the hash instead of
> > having it as a Kconfig parameter.
> 
> I think IMA is doing the right thing and extending into SHA1 and
> SHA256 PCRs if the banks are active and with the boot aggregate puts
> a lid on top of the PCRs 0-7(,8-9). IMA may help raise the suspicion
> about abuse of an unused PCR bank by the firmware but looking at the
> measured boot log etc. alone I think is not enough.

The problem is not where IMA extends, it's where it gets the boot
aggregate from.  If the IMA hash is sha1 and a sha1 bank exists, it
will use it alone for the boot aggregate.

> At least a test with a recent kernel seems to work out alright when
> only the SHA256 bank is active.

Well, yes, if IMA is configured as sha1 and no sha1 bank exists, it
will fall back to sha256, but that doesn't cover the boot aggregate
problem above.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82523): https://edk2.groups.io/g/devel/message/82523
Mute This Topic: https://groups.io/mt/86487987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option

[James Bottomley]

On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
> On 10/22/21 7:49 AM, James Bottomley wrote:
> > On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> > [...]
> > > I see this also but when I get into Linux and run tpm2_pcrread I
> > > see the SHA1 bank active but not having received any PCR
> > > extensions from the firmware, which is not supposed to happen.
> > That's not entirely correct: the TCG firmware profile just requires
> > us to log through at least one bank; it doesn't require that all
> > active banks be logged.  I've got several physical systems with
> > three active banks but only one or two measured through.
>  
> The problem with this is that you can then fake measured boot on
> that system using it's unused SHA1 bank and extend into it whatever
> you want and create a fake log along with it and the quote is going
> to look alright.

I don't think you can.  The measured boot PCRs in unused banks should
always be their default values and the measurement software should
check for this.  So on a system that only uses the sha256 bank, the
sha1 bank PCR0-7 should be all zeros ... if they aren't this should be
a measurement failure.

That means that if you try to replace the sha256 agile log with one
containing fake sha1 entries, the attestation still fails because the
sha256 bank doesn't have default entries.

> > The knock on problem the linux kernel is going to have is that we
> > do tend to expect the sha1 bank to be extended into if any others
> > are, so someone is going to have to update expectations ... we
> > should have this in hand already as sha1 is deprecated.
> > 
> > >   So I think you should drop this patch and I'll change the set
> > > of active PCR banks on the swtpm_setup level.
> >  
> > Even if the firmware deactivated the sha1 bank, the kernel
> > expectation problem is still going to exist.
> 
> Is that older Linux kernels or which part still requires sha1? A
> pointer would be good. I would have to revert the change to not
> activat ethe SHA1 bank from swtpm_setup if that's going to create
> headaches. I thought some hardware TPM 2's today are only providing a
> SHA256 bank and so it shouldn't be a problem.

The problem is IMA: it's hash is a kernel config parameter which
defaults to sha1.  It then tries to calculate the boot aggregate over
the configured hash bank and doesn't check if it's unused.

What IMA should probably be doing is working out which bank the bios is
logging through and using that as the hash instead of having it as a
Kconfig parameter.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82520): https://edk2.groups.io/g/devel/message/82520
Mute This Topic: https://groups.io/mt/86487987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option

[James Bottomley]

On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
[...]
> I see this also but when I get into Linux and run tpm2_pcrread I see
> the SHA1 bank active but not having received any PCR extensions from
> the firmware, which is not supposed to happen.

That's not entirely correct: the TCG firmware profile just requires us
to log through at least one bank; it doesn't require that all active
banks be logged.  I've got several physical systems with three active
banks but only one or two measured through.

The knock on problem the
linux kernel is going to have is that we do tend to expect the sha1
bank to be extended into if any others are, so someone is going to have
to update expectations ... we should have this in hand already as sha1
is deprecated.

>  So I think you should drop this patch and I'll change the set of
> active PCR banks on the swtpm_setup level.

Even if the firmware deactivated the sha1 bank, the kernel expectation
problem is still going to exist.

James





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82518): https://edk2.groups.io/g/devel/message/82518
Mute This Topic: https://groups.io/mt/86487987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V6 1/1] OvmfPkg: Enable TDX in ResetVector

[James Bottomley]

On Sat, 2021-09-18 at 06:30 -0500, Brijesh Singh wrote:
> On 9/18/21 12:16 AM, Xu, Min M wrote:
[...]
> > I usually do the development in windows and build the OVMF image
> > with VS2019.
> > If the new feature works, then I cherry-pick the patch-sets to code
> > base in ubuntu 18.04 and build/test the new feature.
> > 
> > The weird thing is that, with VS2019, even the OVMF image is built
> > from edk2-master, such image doesn't work on AMD SEV server either.
> > But if the image is built by Ubuntu 18.04, it does work on AMD SEV
> > server.
> 
> This seems very strange that we are failing to execute the hand
> written assembly code. I am wondering if somehow the VS compiler is
> generating a wrong byte code and thus causing a trap on KVM that
> requires emulation.

This theory should be verifiable by doing objdump on the windows binary
... can someone do that?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80861): https://edk2.groups.io/g/devel/message/80861
Mute This Topic: https://groups.io/mt/85597386/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Question about EDK2 and commit signing

[James Bottomley]

On Mon, 2021-09-13 at 19:31 +, Marvin Häuser wrote:
> Hey Pedro,
> 
> Same point as before really, why would an attacker have access to
> your SSH key but not your GPG key? This scenario leaves out the
> possibly of  an HTTPS over SSH attack, in which case as a security-
> aware person you use 2FA of course ( :) ), which means this is not
> possible without creating a personal access token. There is very
> little reason to do this at all - I never did this before, and I
> don't know anyone who does this with their private or work GitHub
> account (I think a few use it for CI?), at least that I know of. And
> even if you need one, and you give it  push rights to actually push
> with, and you require GPG signatures globally, you again are keeping
> those two factors at least close together, if not in the same spot.

I think the scenario in question was someone hacking into github.  They
can bypass your ssh login requirement without needing your key, because
that's enforced by github but they can't sign your commit unless they
compromise your laptop or token.  There are many ways of hacking a
cloud service besides simply trying to fake the login or extract the
token from the user.

The way we get around this in Linux is with signed tags, but github
doesn't support that workflow.

I still really don't think signed commits adds much, even to github,
because to be informationally useful, all commits have to be signed. 
Plus, anyway, if the entire site is compromised there'll be bigger
problems than checking commit signatures ...

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80667): https://edk2.groups.io/g/devel/message/80667
Mute This Topic: https://groups.io/mt/85538324/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Question about EDK2 and commit signing

[James Bottomley]

On Sat, 2021-09-11 at 19:25 +0100, Pedro Falcato wrote:
> Hi everyone,
> 
> Yesterday, when pushing my first commits to edk2-platforms (as the
> Ext4Pkg maintainer), I noticed that my commits (see 7872c98 and
> 71f3343) stick out like a sore thumb, as I have GPG signing on my
> commits on by default (see git config commit.gpgsign), globally
> across all my projects.

They do?  The gpgsig header is eaten by modern versions of git ... it
only shows up as the verified decoration on github, which most people
likely don't notice, because github has a huge amount of commit bling,
so I'm not sure what you think people would notice.  I suspect even
ancient versions of git understand it's a header even if they can't
parse it.

> Is there an official stance on signed commits? I was thinking that
> commit signing, at least for the maintainers that apply and push
> patches, could be useful as a way to establish authenticity for every
> commit that gets to the edk2 repos.

The general consensus over at the Linux Kernel, which is an email based
project like edk2, is that signed commits don't add anything useful. 
They can't be transmitted from the author in email, so they can only be
added by the committer.  In the current trust model, the committer is
already trusted with access to the tree, so a signature doesn't add
much beyond what's already known (the committer did this) and it can't
add anything further about the authenticity of the actual commit if
author != committer.  The other problem with signed commits is there
are lots of usual git operations (like rebase) where the signature
doesn't survive.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80537): https://edk2.groups.io/g/devel/message/80537
Mute This Topic: https://groups.io/mt/85538324/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V5 1/2] OvmfPkg: Introduce Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb

[James Bottomley]

On Wed, 2021-09-01 at 08:59 +, Yao, Jiewen wrote:
> Hi Min
> I agree with Gerd and Ard in this case.
> 
> It is NOT so obvious that the FTW is produced then consumed in the
> code. What if the attacker prepares some special configuration to
> trigger the FTW process at the first boot, the code will do *read*
> before *write*? That is a potential attack surface.

It's not just that: even if you can ensure nothing in the host changed
the variables, how do you know *your* code inside the guest is updating
them?  In ordinary OVMF we try to ensure that by having the variables
SMM protected so the only update path available to the kernel is via
the setVariable interface, but we can't do that in the confidential
computing case because SMM isn't supported.  That means a random kernel
attacker in the guest can potentially write to the var store too.

At least for the first SEV prototype I had to make the var store part
of the first firmware volume firstly so it got measured but secondly so
it couldn't be used as a source of configuration attacks.

I have a nasty feeling that configuration attacks are going to be the
bane of all confidential computing solutions because they give the
untrusted VMM a wide attack surface.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80107): https://edk2.groups.io/g/devel/message/80107
Mute This Topic: https://groups.io/mt/85242567/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg PlatformBootManagerLib: Move TryRunningQemuKernel()

[James Bottomley]

On Wed, 2021-08-11 at 09:04 +1000, Christoph Willing wrote:
> On 11/8/21 12:26 am, James Bottomley wrote:
> [...]
> > In the working kernel dmesg Gerd requested, what does it mount as
> > root? sda?  In which case what does the kernel say about where it
> > got sda from?
> > 
> Yes it mounts /dev/sda2 as root.
> 
> The boot logs are are now attached at
> https://bugzilla.tianocore.org/show_bug.cgi?id=3504 as well a diff
> between good and bad boots (patched & unpatched code). There's
> nothing obvious (to me) as to why the unpatched code can't find the
> virtual disk.
> 
> My simple minded guess is that PlatformBdsConnectSequence() performs
> some preparatory work that enables the kernel to have access to the
> vm (therefore to the virtual disk) by the time TryRunningQemuKernel()
> is called. At the moment however, TryRunningQemuKernel() is called
> before PlatformBdsConnectSequence() so that preparatory work hasn't
> been done and the disk can't be found.

I suspect the problem is that it no longer creates default devices if
you don't specify them.  If I look at my working version of a command
line boot, it's this:

qemu-kvm \
-drive 
if=pflash,format=raw,unit=0,file=/home/jejb/git/edk2/Build/OvmfX64/DEBUG_GCC5/FV/OVMF.fd,readonly
 \
-debugcon file:debug.log -global isa-debugcon.iobase=0x402 \
-kernel bzImage \
-initrd initrd.img \
-append "console=ttyS0 rd.driver.pre=virtio_scsi,9p,9pnet_virtio 
root=/dev/sda1" \
-m 2048 \
-serial stdio \
-drive if=none,id=sd00,file=debian.img,format=qcow2 \
-fsdev local,security_model=passthrough,id=modules,path=/tmp/lib/modules \
-fsdev local,security_model=passthrough,id=home,path=/home/jejb \
-device virtio-scsi-pci,id=scsi \
-device virtio-9p-pci,fsdev=modules,id=sd01,mount_tag=modules \
-device virtio-9p-pci,fsdev=home,id=sd02,mount_tag=home \
-device scsi-hd,bus=scsi.0,drive=sd00 \
-device e1000,netdev=net0 \
-netdev user,id=net0,hostfwd=tcp::-:22 \
-display vnc=:5 \


#-chardev socket,id=chrtpm,path=`pwd`/tpmdev/swtpm-sock \
#-tpmdev emulator,id=tpm0,chardev=chrtpm \
#-device tpm-tis,tpmdev=tpm0 \

You can ignore all the TPM stuff and my 9p pass throughs for home
directory and /tmp modules directory (to make it easy to boot any
kernel I build).  The relevant lines for you are:

-drive if=none,id=sd00,file=debian.img,format=qcow2 \
-device virtio-scsi-pci,id=scsi \
-device scsi-hd,bus=scsi.0,drive=sd00 \

Which is how I connect /dev/sda to the qcow2 debian image I use to
boot.  You have the drive line but no device lines giving the driver
... if you add device lines can you then get it to boot?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79067): https://edk2.groups.io/g/devel/message/79067
Mute This Topic: https://groups.io/mt/84767423/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg PlatformBootManagerLib: Move TryRunningQemuKernel()

[James Bottomley]

On Tue, 2021-08-10 at 10:10 +1000, Christoph Willing wrote:
> On 10/8/21 12:52 am, James Bottomley wrote:
> > On Mon, 2021-08-09 at 22:53 +1000, Christoph Willing wrote:
> > > With soft feature freeze started, I wonder if this patch could be
> > > reviewed and pushed for edk2-stable202108 tag? I think it has
> > > languished because I didn't initially Cc appropriately - pls add
> > > others as necessary.
> > > 
> > > This patch is a trivial (I think) change which fixes a long
> > > standing
> > > and annoying bug for those booting Qemu with UEFI using external
> > > kernel & initrd.
> > 
> > I'm with Ard on this one: -kernel is working just fine for me and
> > the
> > team at IBM working on Kata containers.  It sounds like this might
> > be a
> > problem local to your environment, so we need to debug it to
> > understand
> > the issue rather than blindly reverse existing commits.
> > 
> Thanks for responding James & Ard.
> 
> Below is the script I'm using to create, then run, the VM. To verify
> that it works normally with UEFI boot, it initially uses the internal
> kernel & initrd.
> 
> The OVMF_CODE & my_VARS lines contain git hash to identify the build
> from which OVMF_CODE.fd & OVMF_VARS.fd were taken; 97fdcg is from a
> build of yesterday's git master.
> 
> After the OS has been installed, I can run the VM multiple times to
> verify that it boots under UEFI OK (I see the TianoCore splash
> screen)
> with internal kernel.
> 
> 
> #!/bin/bash
> 
> /usr/bin/qemu-kvm \
> -name "UEFI Testing" \
> -enable-kvm \
> -cpu kvm64 \
> -smp cores=4 \
> -boot once=c \
> -m 8192 \
> -device intel-hda \
> -device hda-duplex \
> -vga virtio \
> -drive if=pflash,format=raw,file=OVMF_CODE_97fdcb.fd,readonly=on \
> -drive if=pflash,format=raw,file=my_VARS_97fdcb.fd \
> -drive file=disk.img,format=raw,cache=none,index=0,media=disk \
> -cdrom
> /storage/iso/slackware/slackware64-15.0/slackware64-15.0-20210807.iso 
> \
> -daemonize \
> "$@"

There's no definition of a disk device in here.

> To now use external kernel, I add the lines:
> 
> -kernel /var/cache/vmbuilder/boot/15.0/x86_64/vmlinuz \
> -initrd /var/cache/vmbuilder/boot/15.0/x86_64/initrd \
> -append "root=/dev/sda2 rootfstype=ext4 ro vga=0x386" \
> 
> to the script just after "-boot once=c" (but I doubt the exact
> positioning makes any difference).
> 
> In this case, I see the kernel running and initrd unpacked and its
> modules loaded but the root partition is unable to be mounted - the
> disk
> is not visible (running 'ls -l /dev/sd*' in recovery shell gives 'ls:
> /dev/sd*: No such file or directory').
> 
> The last lines of the Qemu screen are:
> 
> /boot/initrd-5.13.8.gz: Loading kernel modules from initrd image:
> insmod /lib/modules/5.13.8/kernel/fs/jbd2/jbd2.ko
> insmod /lib/modules/5.13.8/kernel/fs/mbcache.ko
> insmod /lib/modules/5.13.8/kernel/fs/ext4/ext4.ko
> mount: mounting /dev/sda2 on /mnt failed: No such file or directory

Which looks like why this failed.

Where's the vmm supposed to get /dev/sda from?  It sort of seems like
the CD rom boot script thinks it was mounted as a USB device in this
case.

In the working kernel dmesg Gerd requested, what does it mount as root?
sda?  In which case what does the kernel say about where it got sda
from?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79030): https://edk2.groups.io/g/devel/message/79030
Mute This Topic: https://groups.io/mt/84767423/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/4] Ovmf: Disable the TPM2 platform hierarchy

[James Bottomley]

On Mon, 2021-08-09 at 12:37 -0400, Stefan Berger wrote:
> This series imports code from the edk2-platforms project related to
> changing the password of the TPM2 platform hierarchy and uses it to
> disable the TPM2 platform hierarchy in Ovmf. It addresses the Ovmf
> aspects of the following bugs:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> https://bugzilla.tianocore.org/show_bug.cgi?id=3499

This raises a couple of issues:

   1. Since OVMF is for all x86 virtual platforms not just the PC ones,
  should it be following the PC client spec for everything?  I notice
  you left out Xen and Bhyve ... should they never follow this?
   2. Since OVMF is effectively both the platform and the firmware, what
  attitude should we take to code in edk2-platforms?  There are
  arguments for pulling all the necessary components into OVMF, but it
  could also be argued that the VMM should take care of all the edk2-
  platforms pieces and OVMF should be strictly firmware.

Getting 2. sorted out is probably the more pressing policy issue for
us.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78983): https://edk2.groups.io/g/devel/message/78983
Mute This Topic: https://groups.io/mt/84773154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg PlatformBootManagerLib: Move TryRunningQemuKernel()

[James Bottomley]

On Mon, 2021-08-09 at 22:53 +1000, Christoph Willing wrote:
> With soft feature freeze started, I wonder if this patch could be
> reviewed and pushed for edk2-stable202108 tag? I think it has
> languished because I didn't initially Cc appropriately - pls add
> others as necessary.
> 
> This patch is a trivial (I think) change which fixes a long standing
> and annoying bug for those booting Qemu with UEFI using external
> kernel & initrd.

I'm with Ard on this one: -kernel is working just fine for me and the
team at IBM working on Kata containers.  It sounds like this might be a
problem local to your environment, so we need to debug it to understand
the issue rather than blindly reverse existing commits.

Regards,

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78965): https://edk2.groups.io/g/devel/message/78965
Mute This Topic: https://groups.io/mt/84767423/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

[James Bottomley]

On Mon, 2021-07-26 at 00:55 +, Yao, Jiewen wrote:
> Hi James
> "However, this ran into problems when it was decided AmdSev shouldn't
> have it's own Library."
> 
> I am not clear on the history. Would you please clarify why AmdSev
> should not have its own library?

The history predates me.  It was already done for the Bhyve package
which also has a modified PlatformBootManagerLib when I came along with
this.  However, only having Library in the top level package seems to
be a common edk2 pattern if you run a find.

> It looks not reasonable to me. AmdSev is just a feature. A feature
> may have its own library. We have enough examples.

We do?  Running

find . -name Library -print

only turns up

./FmpDevicePkg/Test/UnitTest/Library

As not following the top level package only pattern.

> Also, the instance name "Grub" is very confusing. I compared
> PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a
> customized PlatformBootManagerLib. 

It's called Grub because it places Grub in the Fv for combined pre-
attestation.  Either SEV or TDX could use this (Although TDX looks
likely not to want to).

> For example, XEN feature removing and PIIX4 difference has nothing to
> do with Grub...
> =
>   PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
>   PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
>   PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
>   PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
> =

It's part of the boot path stripping to make sure there's a hard
failure if Grub fails to execute.  There's a Bugzilla requiring more of
this because a grub only booting platform library needs fewer
extraneous things which could constitute an attack surface for the
injected secret.

> It is a big misleading. Can we move the PlatformBootManagerLibGrub To
> AmdSev now?

I think you probably want to ask around older edk2 package maintainers
and see if there's any reason for this pattern, which seems to be
strongly enforced.  If no-one can remember, then likely it can be
broken.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78175): https://edk2.groups.io/g/devel/message/78175
Mute This Topic: https://groups.io/mt/84375116/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

[James Bottomley]

On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote:
> And I do have one question:
> > May I know what is criteria to put a SEV module to OvmfPkg\AmdSev
> > or OvmfPkg directly?
> > 
> > My original understanding is:
> > If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},
> > then it should be OvmfPkg.
> > If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf},
> > Then it should be in OvmfPkg\AmdSev.
> > 
> > Am I right?
> > 
> 
> I actually don't know the criteria.  What you say sounds reasonable.
> I'll also let James (who introduced the AmdSevX64 target) say what he
> thinks.

The original reason for the AmdSev package was actually for
attestation:  The only way to get attested boot using a standard VM
image for SEV and SEV-ES was to pull grub inside the measurement
envelope and have a stripped down hard failing boot path, so if the key
didn't decode the encrypted boot volume for some reason, the whole
thing would fail without revealing the injected secret.  This stripped
down hard failing boot path is much easier to construct as a separate
target.

Essentially that means that lots of SEV exists outside the AmdSev
directory and things should only be in it if they're either modified to
support the encrypted volume boot path or are only required by it. 
However, this ran into problems when it was decided AmdSev shouldn't
have it's own Library, so the modified boot path now lives in
OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to
me what the criteria are.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78160): https://edk2.groups.io/g/devel/message/78160
Mute This Topic: https://groups.io/mt/84375116/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/AmdSev: introduce EMBED_GRUB=FALSE to skip including Grub image

[James Bottomley]

On Wed, 2021-07-07 at 10:42 +, Dov Murik wrote:
> The AmdSevX64 target includes an embedded Grub image to support
> secure
> (measured) boot of confidential guests with encrypted root images.
> 
> However, it is sometimes convenient to build this target without an
> embedded Grub.  We introduce the EMBED_GRUB setting (defaults to
> TRUE),
> which conditions the generation (grub.sh) and inclusion of the Grub
> image.  Now building AmdSevX64 with -DEMBED_GRUB=FALSE allows it.
> 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Ashish Kalra 
> Cc: Brijesh Singh 
> Cc: Erdem Aktas 
> Cc: James Bottomley 
> Cc: Jiewen Yao 
> Cc: Min Xu 
> Cc: Tom Lendacky 
> Cc: Tobin Feldman-Fitzthum 
> Signed-off-by: Dov Murik 
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc | 16 +++-
>  OvmfPkg/AmdSev/AmdSevX64.fdf |  2 ++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 1d487befae08..ba7d6fe6b749 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -25,7 +25,6 @@ [Defines]
>BUILD_TARGETS  = NOOPT|DEBUG|RELEASE
>SKUID_IDENTIFIER   = DEFAULT
>FLASH_DEFINITION   = OvmfPkg/AmdSev/AmdSevX64.fdf
> -  PREBUILD   = sh OvmfPkg/AmdSev/Grub/grub.sh
>  
>#
># Defines for default states.  These can be changed on the command
> line.
> @@ -40,6 +39,19 @@ [Defines]
>#
>DEFINE BUILD_SHELL = FALSE
>  
> +  #
> +  # Embed Grub into the OVMF image so they are measured together
> when launching
> +  # confidential guest
> +  #
> +  DEFINE EMBED_GRUB  = TRUE
> +
> +!if $(EMBED_GRUB) == TRUE
> +  #
> +  # This step builds the grub.efi binary image if needed
> +  #
> +  PREBUILD   = sh OvmfPkg/AmdSev/Grub/grub.sh
> +!endif
> +
>#
># Device drivers
>#
> @@ -784,7 +796,9 @@ [Components]
>}
>  !endif
>OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> +!if $(EMBED_GRUB) == TRUE
>OvmfPkg/AmdSev/Grub/Grub.inf
> +!endif
>  !if $(BUILD_SHELL) == TRUE
>ShellPkg/Application/Shell/Shell.inf {
>  
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf
> b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index 9977b0f00a18..ee3d96bb813f 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -270,7 +270,9 @@ [FV.DXEFV]
>  INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellC
> ommand.inf
>  !endif
>  INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> +!if $(EMBED_GRUB) == TRUE
>  INF  OvmfPkg/AmdSev/Grub/Grub.inf
> +!endif
>  !if $(BUILD_SHELL) == TRUE
>  INF  ShellPkg/Application/Shell/Shell.inf
>  !endif

This likely isn't enough: the boot pathway of the AmdSev package is
stripped down and designed to fail if grub won't boot, so if you set
EMBED_GRUB = false, you'll likely build a system that won't boot.  This
would still work for the Kata use case, if the kernel and initrd are
plumbed back in, but it won't work for the generic use case.  I think
the change log needs to describe the use cases so we don't end up
getting a load of annoyed people building systems that won't work for
them.

There's also the broader question of whether this should all be
integrated back into OvmfX64Pkg with more determination done at
runtime, so we can build fewer separate binaries?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77553): https://edk2.groups.io/g/devel/message/77553
Mute This Topic: https://groups.io/mt/84041501/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Thu, 2021-06-24 at 00:24 +, Min Xu wrote:
> On 06/22/2021 9:39 PM, Laszlo wrote:
> > I should clarify: the relevant part of my preference is not that
> > "IntelTdx.dsc"
> > contain the *complete* TDVF feature set. The relevant part (for me)
> > is that
> > "OvmfPkgX64.dsc" *not* be over-complicated for the sake of TDX,
> > even
> > considering only the "basic" TDVF feature set. It's fine to
> > implement TDX in two
> > stages ("basic" and "complete"); my point is that even "basic"
> > should not over-
> > complicate "OvmfPkgX64.dsc".
> > 
> Thanks much for the comments and we don't want to make OvmfPkgX64.dsc
> over-complicated either. 
> We have updated the design slides to V0.95 and Slides 6-15 are
> discussing the
> Config-A and Config-B. 
> https://edk2.groups.io/g/devel/files/Designs/2021/0611/TDVF_Design_Review%28v0.95%29.pptx
> Your comment is always welcome!

The mailing list still won't give me that file, can you update it in
the bugzilla:

https://bugzilla.tianocore.org/show_bug.cgi?id=3429

As well, please?

Thanks,

James





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77020): https://edk2.groups.io/g/devel/message/77020
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Thu, 2021-06-10 at 21:38 -0400, James Bottomley wrote:
> On Fri, 2021-06-11 at 01:36 +, Yao, Jiewen wrote:
> > Hi James.
> > I attached the invitation and copied all content below:
> > 
> > ==
> > ## TOPIC
> > 
> > 1. NA
> > 
> > For more info, see here: https://www.tianocore.org/design-meeting/
> > 
> > ---
> > ## Microsoft Teams meeting
> > 
> > ### Join on your computer or mobile app
> > 
> > [Click here to join the meeting](
> > https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTNmZTNhMWEtOWQwNi00ZTdkLWI5NDgtYTFmYjNkOWI0ZDg4%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%2255d36a50-78be-4ced-bc27-3d06c576cc19%22%7d
> > )
> 
> Apparently it's not possible to join from a web browser: is there a
> dial in?

In the absence of a dial in, I'll view the recording.  I did most of my
comments in the email thread anyway and I'll be boarding my next flight
soon.

However, next time can we hold meetings with usable web based meeting
technologies, like zoom or webex?  It's not feasible to demand
downloading gigabytes of app from a remote location ... even when it
works, which this one doesn't seem to do: the app download just keeps
going back to the meeting screen.

Thanks,

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76356): https://edk2.groups.io/g/devel/message/76356
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Fri, 2021-06-11 at 01:36 +, Yao, Jiewen wrote:
> Hi James.
> I attached the invitation and copied all content below:
> 
> ==
> ## TOPIC
> 
> 1. NA
> 
> For more info, see here: https://www.tianocore.org/design-meeting/
> 
> ---
> ## Microsoft Teams meeting
> 
> ### Join on your computer or mobile app
> 
> [Click here to join the meeting](
> https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTNmZTNhMWEtOWQwNi00ZTdkLWI5NDgtYTFmYjNkOWI0ZDg4%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%2255d36a50-78be-4ced-bc27-3d06c576cc19%22%7d
> )

Apparently it's not possible to join from a web browser: is there a
dial in?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76355): https://edk2.groups.io/g/devel/message/76355
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Thu, 2021-06-10 at 22:30 +, Xu, Min M wrote:
> Hi, All
> Thanks much for the valuable comments and discussion about the
> design.
> We have updated the slides (v0.9) in below link. If some comments or
> concerns are not answered/addressed in the new slides, please don't
> hesitate to tell us. We do want to answer/address all the
> comments/concerns. But to be honest it is a rather complicated one
> and we appreciate your feedbacks.
> https://edk2.groups.io/g/devel/files/Designs/2021/0611/TDVF_Design_Review%28v0.9%29.pptx

What's the url of the meeting?  Apparently it isn't in the calendar
entry.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76353): https://edk2.groups.io/g/devel/message/76353
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Wed, 2021-06-09 at 17:47 +0200, Paolo Bonzini wrote:
> On 09/06/21 16:28, James Bottomley wrote:
> > That would cut across the ApEntrypoint and the guidedStructureEnd.
> > However, nothing says anything in the reset vector guided structure
> > has to be data ... so it could equally well be code.  That means we
> > can do guid based entries that contain the 32 bit real and 64 bit
> > entry points.  This would also come with the added advantage that
> > we can scan the OVMF binary to see what entry points it supports.
> 
> Isn't the initial state included in the save area just like for SEV-
> ES?

Initial state of what?  We currently have two entries: one points to
the address and size of the secrets page and the other gives the
address of the ES work area page that's used for AP reset.

>So it's not even QEMU, but rather some external tool that builds
> the encrypted image, that needs to understand that GUIDed structure.

Yes, it's really to make the configuration of the OVMF blob somewhat
introspectable.  The current consumer is QEMU, but I see no reason why
other tools might not use this mechanism as well.

> The GUIDed structure can either include the entry point code; or it 
> could have room for a couple 8-byte pointers since any fixed-size
> area in the GUIDed structure would be just a jump anyway.

Well, exactly ... depending on what the requirement is we can do pretty
much anything with the data contents with the only caveat that it's
currently constructed by an asm file, so we don't quite have the full
macro power of C available.  However, symbol resolution is definitely
possible and quite easy.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76284): https://edk2.groups.io/g/devel/message/76284
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Wed, 2021-06-09 at 13:00 +0200, Laszlo Ersek wrote:
> On 06/09/21 02:58, Xu, Min M wrote:
> > On 06/09/2021 3:33 AM, Laszlo wrote:
> > > On 06/08/21 18:01, James Bottomley wrote:
> > > > On your slide 13 Question: "Open: How will the QEMU find the
> > > > metadata location?" can't you just use the mechanism for SEV
> > > > that's already upstream in both QEMU and OVMF?
> > > 
> > > I think I made the same comment, in different words. (Point (12)
> > > at
> > > <https://listman.redhat.com/archives/edk2-devel-archive/2021-
> > > June/msg00143.html>.)
> > > 
> > So my understanding to this solution is that: 
> > 1) GUID-ed structure chain is started from a fixed GPA in 
> >ResetVector. 
> > 2) Append a TDX-specific GUID-ed structure in the chain
> > 3) Qemu search the GUID-ed chain from the fixed GPA and find the 
> >TDX-specific GUID-ed structure based on TDX-specific GUID.
> > Is the expected process for QEMU?
> 
> This is my understanding, yes; James will know more details though.

Pretty much, yes.  The guided structure is designed as a backwards
table, so you can tell if it's present or not by looking for the table
guid (96b582de-1fb2-45f7-baea-a366c55a082d) at 0xffd0.  If you find
that, it gives you the size of the table as the u16 preceding the GUID.
All entries are of the form

  

You can see how it works in

OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm

Beginning around line 35.

In QEMU you want the function pc_system_ovmf_table_find() which is in
hw/i386/pc_sysfw.c and finds entries by guid.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76278): https://edk2.groups.io/g/devel/message/76278
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Wed, 2021-06-09 at 02:01 +, Xu, Min M wrote:
> On 06/09/2021 12:01 AM, James Bottomley wrote:
[...]
> > On slide 19, the mucking with the reset vector really worries me
> > because we don't have that much space to play with.  Given that
> > you're starting in 32 bit mode and can thus enter anywhere in the
> > lower 4GB, why not simply use a different and TDX specific entry
> > point?
> > 
> If TDVF has a separate DSF/FDF, this is not a problem.
> 
> I once think about below solution, that different mode goes to its
> specific entry point.
> For example:
> real-mode goes to 0xfff0, 
> protected-mode goes to 0xffe0, 
> long-mode goes to 0xffd0. 

That would cut across the ApEntrypoint and the guidedStructureEnd. 
However, nothing says anything in the reset vector guided structure has
to be data ... so it could equally well be code.  That means we can do
guid based entries that contain the 32 bit real and 64 bit entry
points.  This would also come with the added advantage that we can scan
the OVMF binary to see what entry points it supports.

James





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76277): https://edk2.groups.io/g/devel/message/76277
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Thu, 2021-06-03 at 13:51 +, Yao, Jiewen wrote:
> Hi, All
> We plan to do a design review for TDVF in OVMF package.
> 
> 
> The TDVF Design slides for TinaoCore Design Review Meeting (Jun 11)
> is now available in blow link: 
> https://edk2.groups.io/g/devel/files/Designs/2021/0611.
> 
> The Bugzilla is https://bugzilla.tianocore.org/show_bug.cgi?id=3429

It looks like I'll be travelling that day, but should be able to attend
at least the first 45 minutes of the design review from the airport
lounge.

> You can have an offline review first. You comments will be warmly
> welcomed and we will continuously update the slides based on the
> feedbacks.

On TdMailbox and TdHob, we already have two SEV pages in the MEMFD and
since TDX and SEV is an either/or, could we simply not rename both
pages and use them for either boot depending on what CPU type is
detected, so we only have two MEMFD pages, not four?

On your slide 13 Question: "Open: How will the QEMU find the metadata
location?" can't you just use the mechanism for SEV that's already
upstream in both QEMU and OVMF?

On slide 19, the mucking with the reset vector really worries me
because we don't have that much space to play with.  Given that you're
starting in 32 bit mode and can thus enter anywhere in the lower 4GB,
why not simply use a different and TDX specific entry point?

I'm not quite sure why you don't have a PEI phase, since TdxStartupLib
seems effectively to be PEI.

On all the Tcg2 changes: what about installing a vTPM driver that
simply translates to your MSRs?  That way we can use all the standard
TCG code as is?  Plus then we could do SEV-SNP measurement through an
actual vTPM running at higher VMPL or something.

Slide 41: IOMMU operation.  The implication is that you only transition
to unencrypted memory for DMA during the actual operation, so do I have
it correct that the guest writes DMA to encrypted memory, then the
iommu marks the region as unencrypted and transforms the memory to be
in the clear and then transforms it back after the DMA operation
completes?  Given that SEV operates quite happily with always in the
clear DMA buffers, this seems to have the potential to be a performance
problem, but what security does it gain?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76228): https://edk2.groups.io/g/devel/message/76228
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

[James Bottomley]

On Fri, 2021-06-04 at 15:52 +0100, Michael Brown wrote:
> On 04/06/2021 11:43, Michael Brown wrote:
> > On 04/06/2021 11:11, Laszlo Ersek wrote:
> > > And, to reiterate, just because Confidential Computing is the
> > > new hot thing, the use cases for OvmfPkgIa32, OvmfPkgIa32X64,
> > > OvmfPkgX64 do not disappear. Regressing them, or making them
> > > unmaintainable due to skyrocketing complexity, is not acceptable.
> > 
> > Totally agree with this.  Confidential Computing is a very niche
> > use case, and there is no justification for exploding the
> > complexity of the standard OVMF build.
> > 
> > If, several years from now, it ever reaches the point that the
> > majority of real-world workloads are using TDX, then there would be
> > an argument that the complexity cost has to be paid and that the
> > standard OVMF build should include TDX features.  But that's
> > several years away and may never happen.
> 
> Out of interest: does Intel TDX provide any security benefits beyond
> the (much simpler) Intel SGX?

The main benefit is ease of deployment for unmodified applications. 
While you might argue this isn't a "security" benefit, remember that
any security technology that is too complex for most people to deploy
doesn't have much impact, so ease of use is a significant consideration
in security technologies.

> As far as I can tell from the various papers, the fundamental
> difference between TDX and SGX seems to be that TDX deliberately
> increases the attack surface from "just the application code" to
> "entire guest VM, including OS kernel, runtime libraries,
> etc".  Increasing the attack surface while adding complexity is a
> huge cost so I'm assuming that there must be some commensurate
> benefit, but nothing in the documentation I've seen seems to describe
> what this benefit actually is.

The big problems of enclave technology like SGX is rewriting
applications into secure and insecure parts and controlling information
leak across the boundaries of the enclave ... even if you opt to run
the application entirely within the enclave, you still get leaks into
the kernel via syscalls and the machine owner still has a huge amount
of leeway to exfiltrate any secrets in the enclave.

The push towards VM based isolation is because all the handling of the
technology can be done inside an enlightened guest kernel (so any
application will run with no modification) and the guest to host
boundary is a far easier to analyse being a hardware emulation
vmexit/hypercall one rather than the huge and complex syscall
interface.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76080): https://edk2.groups.io/g/devel/message/76080
Mute This Topic: https://groups.io/mt/83283616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[James Bottomley]

On Tue, 2021-06-01 at 14:11 +0200, Laszlo Ersek wrote:
> Ard,
> 
> I'll have a specific question for you below; please feel free to jump
> forward (search for your name). Thanks.
> 
> Dov, my comments below:
> 
> On 05/25/21 07:31, Dov Murik wrote:
> > Booting with SEV prevented the loading of kernel, initrd, and
> > kernel command-line via QEMU fw_cfg interface because they arrive
> > from the VMM which is untrusted in SEV.
> > 
> > However, in some cases the kernel, initrd, and cmdline are not
> > secret but should not be modified by the host.  In such a case, we
> > want to verify inside the trusted VM that the kernel, initrd, and
> > cmdline are indeed the ones expected by the Guest Owner, and only
> > if that is the case go on and boot them up (removing the need for
> > grub inside OVMF in that mode).
> > 
> > This patch series declares a new page in MEMFD which will contain
> > the hashes of these three blobs (kernel, initrd, cmdline), each
> > under its own GUID entry.  This tables of hashes is populated by
> > QEMU before launch, and encrypted as part of the initial VM memory;
> > this makes sure theses hashes are part of the SEV measurement
> > (which has to be approved by the Guest Owner for secret injection,
> > for example).  Note that this requires a new QEMU patch which will
> > be submitted soon.
> > 
> > OVMF parses the table of hashes populated by QEMU (patch 5), and as
> > it reads the fw_cfg blobs from QEMU, it will verify each one
> > against the expected hash (kernel and initrd verifiers are
> > introduced in patch 6, and command-line verifier is introduced in
> > patches 7+8).  This is all done inside the trusted VM context.  If
> > all the hashes are correct, boot of the kernel is allowed to continue.
> > 
> > Any attempt by QEMU to modify the kernel, initrd, cmdline
> > (including dropping one of them), or to modify the OVMF code that
> > verifies those hashes, will cause the initial SEV measurement to
> > change and therefore will be detectable by the Guest Owner during
> > launch before secret injection.
> 
> Please catch the error in my reasoning below.
> 
> The goal is for the guest firmware to ensure the authenticity
> (integrity) of kernel, initrd, cmdline.

That's right

> This is not really different from a normal Secure Boot setup, where
> the guest firmware verifies the kernel image (presented as a UEFI
> application, i.e. with the UEFI stub). It is up to the kernel to
> verify the integrity of the initrd.

Right, but this doesn't happen today (there's no initrd measure in the
kernel), so the only current choice you have is to combine the kernel
and initrd then sign the whole combination, which gives one signature
for both.

>  The command line is not particularly verified (as far as I know?), 

Right, it's not, which means secure boot can't replace the
verification, because the command line is an essential thing to measure
given the things it can do to the booting kernel.

> but if that's a problem, it should be solved even for bare metal
> Secure Boot use cases. (Because, if the "root" user is compromised on
> a running Linux system, they can modify the kernel params for next
> boot in the grub config.)

The usual statement to this is that secure boot doesn't need to do this
because the commandline is a measured boot problem, so grub measures
its entire execution to PCR 8.  However, then you have to grope around
in the measurement log and verify it via a TPM quote, which is
incredibly sophisticated stuff compared to taking a single measurement
hash.  Plus, we have no secure TPM currently for virtual machines, so
there's no measured boot measurement the guest owner can trust.

> The AmdSevX64 platform uses a unified firmware image (executable +
> varstore are presented as one blob, no separate CODE and VARS). There
> is one pflash chip, and the initial guest-owner-side measurement
> covers the whole blob, including the varstore.

That's right, except being part of a single rom volume, the varstore is
read only.  This is a deliberate design choice: the absence of SMM and
the fact that a R/W interface wouldn't get measured properly and also
because NV  config changes can be used to effect secret leakage means
it needs to be immutable.

If it's mutable, you need to store it separately, protect it with
something like SMM and be aware of how the measurement would change
when the store was updated ... which is another thing that's not that
easy because of the way flash operates on overwrite.

Finally there's the secure write problem: the DMA for the variable
write would have to go through an unencrypted buffer (because that's
the only way DMA works in confidential computing today).  For disks,
including boot, we cope with this by using an encrypted disk, so we
take the hardware protected page, encrypt it and place it in the clear
DMA buffer for disk write, meaning confidentiality and integrity are
preserved.  We'd have to add an encryption mechanism to pflash or
something to 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[James Bottomley]

On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
> On 5/25/21 3:08 PM, Dov Murik wrote:
> > Hi Brijesh,
> > 
> > On 25/05/2021 18:48, Brijesh Singh wrote:
> > > On 5/25/21 12:31 AM, Dov Murik wrote:
> > > > Booting with SEV prevented the loading of kernel, initrd, and
> > > > kernel command-line via QEMU fw_cfg interface because they
> > > > arrive from the VMM which is untrusted in SEV.
> > > > 
> > > > However, in some cases the kernel, initrd, and cmdline are not
> > > > secret but should not be modified by the host.  In such a case,
> > > > we want to verify inside the trusted VM that the kernel,
> > > > initrd, and cmdline are indeed the ones expected by the Guest
> > > > Owner, and only if that is the case go on and boot them up
> > > > (removing the need for grub inside OVMF in that mode).
> > > > 
> > > > This patch series declares a new page in MEMFD which will
> > > > contain the hashes of these three blobs (kernel, initrd,
> > > > cmdline), each under its own GUID entry.  This tables of hashes
> > > > is populated by QEMU before launch, and encrypted as part of
> > > > the initial VM memory; this makes sure theses hashes are part
> > > > of the SEV measurement (which has to be approved by the Guest
> > > > Owner for secret injection, for example).  Note that this
> > > > requires a new QEMU patch which will be submitted soon.
> > > 
> > > I have not looked at the patches, but trying to brainstorm if we
> > > can avoid reserving a new page in the MEMFD and use the existing
> > > EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
> > > through the FW_CFG interface in the guest memory.
> > > 
> > > If I understand correctly, then in your proposed approach, guest
> > > owner wants to ensure that the hypevisor passing its preferred
> > > kernel, initrd and cmdline. The guest owner basically knows the
> > > hashes of these components in advance.
> > 
> > Yes, that's correct.
> > 
> > > So, can we do something like this:
> > > 
> > > - The secret blob provided by the guest owner should contains the
> > > hashes (sha384) of these components.
> > > 
> > > - Use openssl API available in the edk2 to calculate the hash
> > > while loading the kernel, initrd and cmdline.
> > 
> > Indeed we do something similar already - we use Sha256HashAll (see
> > patch 5 in this series).
> > 
> > 
> > > - Before booting the kernel, compare the calculated hash with the
> > > one listed in the secret page. If they don't match then fail
> > > otherwise continue.
> > 
> > That is indeed what we do in patch 6 (the calls to our
> > ValidateHashEntry).
> > 
> > 
> > > Did I miss something ?
> > 
> > Thanks for proposing this.
> > 
> > Your approach has the advantage that there's no need for extra
> > pre-allocated MEMFD page for the hashes, and also it makes the QEMU
> > flow simpler (QEMU doesn't need to compute the hashes and put them
> > in that special MEMFD page).  I think that the only change we'll
> > need from QEMU in the x86_load_linux flow (which is when the user
> > supplies -kernel/-initrd) is that it won't modify any memory in a
> > way that the modifies the hashes that Guest Owner expects (for
> > example, avoid writing over the kernel's setup area).
> > 
> > However, the disadvantage is that it unifies boot measurement with
> > the secret injection.  The Guest Owner _must_ inject the hashes,
> > otherwise the system doesn't boot; whereas in our current
> > suggestion the Guest Owner can check the measurement, verify that
> > everything is OK, and just let the guest continue.
> > 
> > But as I write this, I think that maybe without secret injection
> > the guest is not really secure? Because the host could just
> > continue execution of the guest without waiting for measurement
> > check... If the Guest Owner _must_ inject a secret for SEV to be
> > secure in any case, we might as well choose your path and let the
> > Guest Owner inject the table of hashes themselves.
> > 
> > I'd like to hear your (and others') thoughts.
> 
> Brijesh and I had a long discussion over this. I think that if you're
> dealing with a malicious hypervisor, then it could, in fact, measure
> all the components that it wants to be used and, using
> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
> secret page, to the guest and indicate that page as the guest secret.
> Even though the measurement would fail validation by the guest owner,
> the hypervisor could ignore it and continue to run the guest.
> 
> So you need something that proves ownership of the guest secret -
> like a disk key that would fail to unlock the disk if the hypervisor
> is faking the guest secret.
> 
> Does all that make sense?

I think it does for the unencrypted boot case.  For the encrypted boot
case, the HV can't inject the decryption key, because it doesn't know
it, so the interior guest will know there's a problem as soon as it
can't decrypt the image.

But I get the point that we can't rely on the secrets page for hashes

Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

[James Bottomley]

On Thu, 2021-05-06 at 13:57 +0300, Dov Murik wrote:
> 
> On 05/05/2021 22:33, Laszlo Ersek wrote:
> > On 05/05/21 15:11, Brijesh Singh wrote:
> > > On 5/5/21 1:42 AM, Dov Murik wrote:
[...]
> > > > Would it make sense to always use EfiACPIMemoryNVS for the
> > > > injected secret area, even for regular SEV (non-SNP)?
> > > 
> > > Ideally yes. Maybe James had some reasons for choosing the
> > > EfiBootServicesData. If I had to guess, it was mainly because
> > > there no guest kernel support which consumes the SEV secrets
> > > page.
> > 
> > git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
> > reserve the Sev Secret area", 2020-12-14).
> > 
> > Commit bff2811c6d99 makes it clear that the area in question lives
> > in MEMFD.
> > 
> > We're populating the area in the PEI phase. We don't want anything
> > in DXE to overwrite it.
> > 
> > Once the bootloader (and/or perhaps the kernel's EFI stub) fetched
> > the secret from that particular location, there is no need to
> > prevent later parts of the OS (the actual kernel) from repurposing
> > that area. That's why EfiBootServicesData was used.
> > 
> 
> The first use of the secret area was to hold the guest luks disk
> passphrase; this is used in the grub-inside-OVMF (AmdSev package),
> and there was no need to keep that page around for the guest kernel.
> 
> The reason I'm raising this whole point is that we're working now on
> guest-kernel support for reading secrets from that injected page (for
> plain SEV).  We considered either (a) modifying the secrets page
> memory type to reserved here, or (b) add code to the kernel EFI stub
> that would copy this page somewhere else for kernel's later use
> (which seems more work and not sure what's the advantage).

It mirrors the TPM boot log behaviour: the log is stored in boot time
only memory, so the kernel EFI stub has to copy it out.  The reason the
TPM boot log behaves this way is that if the kernel didn't want to
collect it, it still gets freed.  I imagine a similar rationale exists
for the boot secrets: if the kernel isn't interested in them for some
reason, we don't want them to persist.

> Option (b) seems harder and more fragile, and I'm not sure if there
> are any advantages (though I'm definitely not an expert in that
> area).

I think forcing the kernel to consume the secret before exit boot
services is still a good idea because 

   1. If the kernel can't consume the secret it gets freed
   2. Not all secrets are consumed by the kernel, so it can pick the ones
  it wants out and discard the rest
   3. If the kernel is using a secret protection mechanism, that may not
  work for the memfd page, so relocation of the secret might be a more
  secure mechanism.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74801): https://edk2.groups.io/g/devel/message/74801
Mute This Topic: https://groups.io/mt/82479058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

[James Bottomley]

On Wed, 2021-05-05 at 21:33 +0200, Laszlo Ersek wrote:
> On 05/05/21 15:11, Brijesh Singh wrote:
> > On 5/5/21 1:42 AM, Dov Murik wrote:
[...]
> > > Would it make sense to always use EfiACPIMemoryNVS for the
> > > injected secret area, even for regular SEV (non-SNP)?
> > 
> > Ideally yes. Maybe James had some reasons for choosing the
> > EfiBootServicesData. If I had to guess, it was mainly because there
> > no guest kernel support which consumes the SEV secrets page.
> 
> git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
> reserve the Sev Secret area", 2020-12-14).
> 
> Commit bff2811c6d99 makes it clear that the area in question lives in
> MEMFD.
> 
> We're populating the area in the PEI phase. We don't want anything in
> DXE to overwrite it.
> 
> Once the bootloader (and/or perhaps the kernel's EFI stub) fetched
> the secret from that particular location, there is no need to prevent
> later parts of the OS (the actual kernel) from repurposing that area.
> That's why EfiBootServicesData was used.

That's right: originally the design was not to have the boot secrets
survive boot because they should already be copied into their correct,
and presumably protected, locations by the time exit boot services
comes.  The grub code actually shreds the secret in the page once it
consumes it, so the area for a simple disk secret should be empty
anyway.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74800): https://edk2.groups.io/g/devel/message/74800
Mute This Topic: https://groups.io/mt/82479058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Problem: TPM 2.0 event log by OVMF is shown empty in Linux kernel versions after 5.8

[James Bottomley]

On Wed, 2021-04-28 at 10:19 -0700, James Bottomley wrote:
> On Wed, 2021-04-28 at 16:56 +0200, Thore Sommer wrote:
> > TPM2 @ 0x
> > : 54 50 4D 32 4C 00 00 00 04 7F 42 4F 43 48 53
> > 20  TPM2L.BOCHS 
> > 0010: 42 58 50 43 54 50 4D 32 01 00 00 00 42 58 50
> > 43  BXPCTPM2BXPC
> > 0020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00  
> > 0030: 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00  
> > 0040: 00 00 01 00 00 C0 BB 7E 00 00 00
> > 00  ...~
> 
> This is actually pretty much exactly what I see in the working OVMF
> 
> TPM2 @ 0x
> : 54 50 4D 32 4C 00 00 00 04 DB 42 4F 43 48 53
> 20  TPM2L.BOCHS 
> 0010: 42 58 50 43 54 50 4D 32 01 00 00 00 42 58 50
> 43  BXPCTPM2BXPC
> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00  
> 0030: 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00  
> 0040: 00 00 01 00 00 60 BE 7F 00 00 00
> 00  .`..
> 
> So it looks like the empty acpi table theory doesn't fly.

Actually, there's another possibility, which is that you're not booting
via the efi stub.  This is somewhat tricky to get right in grub, so you
can rule this out by booting ovmf to a shell and then executing vmlinuz
directly from the shell.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74582): https://edk2.groups.io/g/devel/message/74582
Mute This Topic: https://groups.io/mt/82391340/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Problem: TPM 2.0 event log by OVMF is shown empty in Linux kernel versions after 5.8

[James Bottomley]

On Wed, 2021-04-28 at 16:56 +0200, Thore Sommer wrote:
> TPM2 @ 0x
> : 54 50 4D 32 4C 00 00 00 04 7F 42 4F 43 48 53 20  TPM2L.BOCHS 
> 0010: 42 58 50 43 54 50 4D 32 01 00 00 00 42 58 50 43  BXPCTPM2BXPC
> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
> 0030: 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
> 0040: 00 00 01 00 00 C0 BB 7E 00 00 00 00  ...~

This is actually pretty much exactly what I see in the working OVMF

TPM2 @ 0x
: 54 50 4D 32 4C 00 00 00 04 DB 42 4F 43 48 53 20  TPM2L.BOCHS 
0010: 42 58 50 43 54 50 4D 32 01 00 00 00 42 58 50 43  BXPCTPM2BXPC
0020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
0030: 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
0040: 00 00 01 00 00 60 BE 7F 00 00 00 00  .`..

So it looks like the empty acpi table theory doesn't fly.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74571): https://edk2.groups.io/g/devel/message/74571
Mute This Topic: https://groups.io/mt/82391340/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Problem: TPM 2.0 event log by OVMF is shown empty in Linux kernel versions after 5.8

[James Bottomley]

On Tue, 2021-04-27 at 09:00 -0500, Lendacky, Thomas wrote:
> On 4/27/21 2:40 AM, Thore Sommer via groups.io wrote:
> > > I don't confirm this.  I have Linux version 5.12.0-rc5+ installed
> > > and I
> > > see the attached in my binary_bios_measurements (I've run it
> > > through
> > > tpm2-eventlog so you can see the actual events).
> > Ok that is interesting.
> > 
> > Here are the steps to reproduce my findings.
> > Necessary tools: Build chain for edk2, swtpm 0.5.2 and qemu 5.2.0
> > 
> > 1. Build OVMF from edk2-stable202102 with
> > -a X64 -a IA32 \
> > -b RELEASE \
> > -D NETWORK_IP6_ENABLE \
> > -D TPM_ENABLE \
> 
> Shouldn't you also have '-D TPM_CONFIG_ENABLE' ?

You only need that if you want the TPM configuration option for the
bios menu.  Without it the TPM should self configure ... I tried it
without and it still works for me (produces the bios log).

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74495): https://edk2.groups.io/g/devel/message/74495
Mute This Topic: https://groups.io/mt/82391340/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Problem: TPM 2.0 event log by OVMF is shown empty in Linux kernel versions after 5.8

[James Bottomley]

On Mon, 2021-04-26 at 21:56 +0200, Thore Sommer wrote:
> Dear Maintainers,
> 
> during my testing with OVMF and swtpm I found out that kernel
> versions newer than 5.8 don't show any information in 
> "/sys/kernel/security/tpm0/binary_bios_measurements" if swtpm
> emulates a TPM 2.0 device. The file is still created but is empty.
> The expected result would be that 
> "/sys/kernel/security/tpm0/binary_bios_measurements" contains the
> TPM event log. TPM 1.2 devices are not affected.

I don't confirm this.  I have Linux version 5.12.0-rc5+ installed and I
see the attached in my binary_bios_measurements (I've run it through
tpm2-eventlog so you can see the actual events).

> With the help of git bisect I found out that the breaking kernel
> commit is 85467f63a05c43364ba0b90d0c05bb89191543fa.
> Reverting this on top the 5.12 release restores the expected
> functionality.
> 
> Thanks to apalos and leiflindholm on the #edk2 IRC channel for
> helping me with that.
> 
> I don't know if this is a bug in OVMF or in the Linux kernel, because
> on a real device with a TPM 2.0 the output was as expected.
> 
> Tested with edk2-ovmf 202102, swtpm 0.5.2 and qemu 5.2.0 on Ubuntu
> 20.04.
> 
> If further information is needed to resolve this problem, I'd be
> happy to provide them.

What that commit did was to allow the event log to be provided by the
ACPI table if one existed rather than always defaulting to it being
provided by the EFI configuration table.  What I suspect has happened
from this:

> [0.017358] ACPI: Reserving TPM2 table memory at [mem 
> 0x7eb77000-0x7eb7704b]

Is that somehow you've got an empty TPM2 table installed in ACPI but I
don't know how you've done this.  On my OVMF boot I'm using the direct
kernel command line and I have secure boot enabled but not activated,
which is why you only see PCRs 0-7 in the log.

James



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74439): https://edk2.groups.io/g/devel/message/74439
Mute This Topic: https://groups.io/mt/82391340/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


---
events:
  - EventNum: 0
PCRIndex: 0
EventType: EV_NO_ACTION
Digest: ""
EventSize: 45
SpecID:
  - Signature: Spec ID Event03
platformClass: 0
specVersionMinor: 0
specVersionMajor: 2
specErrata: 0
uintnSize: 2
numberOfAlgorithms: 4
Algorithms:
  - Algorithm[0]:
algorithmId: sha1
digestSize: 20
  - Algorithm[1]:
algorithmId: sha256
digestSize: 32
  - Algorithm[2]:
algorithmId: sha384
digestSize: 48
  - Algorithm[3]:
algorithmId: sha512
digestSize: 64
vendorInfoSize: 0
  - EventNum: 1
PCRIndex: 0
EventType: EV_S_CRTM_VERSION
DigestCount: 4
Digests:
  - AlgorithmId: sha1
Digest: "1489f923c4dca729178b3e3233458550d8dddf29"
  - AlgorithmId: sha256
Digest: 
"96a296d224f285c67bee93c30f8a309157f0daa35dc5b87e410b78630a09cfc7"
  - AlgorithmId: sha384
Digest: 
"1dd6f7b457ad880d840d41c961283bab688e94e4b59359ea45686581e90feccea3c624b1226113f824f315eb60ae0a7c"
  - AlgorithmId: sha512
Digest: 
"5ea71dc6d0b4f57bf39aadd07c208c35f06cd2bac5fde210397f70de11d439c62ec1cdf3183758865fd387fcea0bada2f6c37a4a17851dd1d78fefe6f204ee54"
EventSize: 2
Event: ""
  - EventNum: 2
PCRIndex: 0
EventType: EV_EFI_PLATFORM_FIRMWARE_BLOB
DigestCount: 4
Digests:
  - AlgorithmId: sha1
Digest: "cea68f7b86a5c90cd0730e4bb94400885f1975a4"
  - AlgorithmId: sha256
Digest: 
"fa3192146583dff3072d8e89f8dce87cf86843fbaee6d155995391c65b2bbc51"
  - AlgorithmId: sha384
Digest: 
"648984f574941e3be030c53edb4d28301bf8c73f1390601b536f70dfd55739745b89b0ebd7641554aca8a1dcdc5e0f62"
  - AlgorithmId: sha512
Digest: 
"9ed9d9d82f1c9b34a29fd523216219ea1248e1a2f1a83c3896140d58e57b7f20b1b99e2adf576e5c4681849568bbc067ee000717d3b2e76d3d02eb5a702f387c"
EventSize: 16
Event:
  BlobBase: 0x82
  BlobLength: 0xe
  - EventNum: 3
PCRIndex: 0
EventType: EV_EFI_PLATFORM_FIRMWARE_BLOB
DigestCount: 4
Digests:
  - AlgorithmId: sha1
Digest: "b36ed6c36e48d3f90e1628033d985e57c5ea5302"
  - AlgorithmId: sha256
Digest: 
"792622964790e2934a5374ce1b88514728a5aa869cf81cb4fd24f607f26d369b"
  - AlgorithmId: sha384
Digest: 
"2721bb22a228aaaf9f996b0ac636e492cabadd1f75144433fa8be205a0a0e9f85793b4b6c5906cd06c8ec58689d47faa"
  - AlgorithmId: sha512
Digest: 
"ad2b880e3579acc20fdde41201b104795bf3919184f2f99f39a8e1114532c46d6d99312a9d59b46422ff0c93ec125a493810ca6accf395a4326eabc4579cfdfa"
EventSize: 16
Event:
  BlobBase: 0x90
  

Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]

[James Bottomley]

On Mon, 2021-04-12 at 11:54 +, Yao, Jiewen wrote:
> I totally agree with you that from security perspective, the best
> idea to isolate AMD SEV/Intel TDX from standard OVMF.

There's a big difference between building tuned binaries and separating
the subsystems entirely.  Ideally we don't want customers running
images to have to build them differently for Intel or AMD (a bit like
how QEMU/KVM hide the VM differences from users), and Confidential
Computing shares a huge amount of interface similarity, so we wouldn't
want that separated.  I think the rule should be that if both Intel and
AMD expose a feature in different ways, OVMF tries to expose a uniform
API for that feature over two differing implementations.

> Do you want to propose move AMD SEV support to another SEC?

You mean have an entirely separate SEC for AMD, OVMF and Intel?  I
really wouldn't do that: much that's in the SEC: page table setup,
memory mapping and decompression is common to all of them.  This all
follows for a lot of the components.

To build separate binaries, we just need separate dsc and fdf files. 
Then I think the goal would be to share as much as possible to avoid
duplicating the maintenance and possibly diverging the user API.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73960): https://edk2.groups.io/g/devel/message/73960
Mute This Topic: https://groups.io/mt/81969494/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest

[James Bottomley]

On Wed, 2021-04-07 at 17:02 +0200, Laszlo Ersek wrote:
> On 04/07/21 02:44, James Bottomley wrote:
> > On Wed, 2021-04-07 at 00:21 +, Xu, Min M wrote:
> > > Hi, Laszlo
> > > 
> > > For Intel TDX supported guest, all processors start in 32-bit
> > > protected mode, while for Non-Td guest, it starts in 16-bit real
> > > mode. To make the ResetVector work on both Td-guest and Non-Td
> > > guest, ResetVector are updated as below:
> > > ---
> > > ---
> > >   ALIGN   16
> > >   resetVector:
> > >   ;
> > >   ; Reset Vector
> > >   ;
> > >   ; This is where the processor will begin execution
> > >   ;
> > >   nop
> > >   nop
> > >   smswax
> > >   testal, 1
> > >   jnz EarlyBspPmEntry
> > >   jmp EarlyBspInitReal16
> > 
> > Well, then use the rel8 jump like the compiler would in this
> > situation:
> > 
> >   smswax
> >   testal, 1
> >   jz  1f
> >   jmp EarlyBspPmEntry
> > 1:
> >   jmp EarlyBspInitReal16
> > 
> > So now both entries can be 32k away.
> 
> The problem is that we need NASM to generate such *shared* entry code
> that behaves correctly when executed in either 16-bit or 32-bit mode.
> 
> The rel8 near jumps ("short jumps") are like that -- for example, the
> "74 cb" opcode decodes to the same "JZ rel8" in both modes.
> 
> But the rel16 ("non-short") near jumps turn into rel32 near jumps
> when decoded in 32-bit mode. For example, "E9 cw" decodes to "JMP
> rel16" in 16-bit mode, but it gets parsed as "E9 cd" (= "JMP rel32")
> in 32-bit mode.
> 
> So the idea is to add more BITS directives, for covering the non-
> short near jumps themselves:

Absolutely ... sorry, I should have said this was just the first thing
I thought of.  The key point is don't do a rel8 jump over the guid
table, use the rel8 jump within the reset vector to sort out the final
destination and use the wider jumps to go over the guid table.  As you
say, we have to be very careful about the wider jumps given the
differences between the entry modes.

James






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73786): https://edk2.groups.io/g/devel/message/73786
Mute This Topic: https://groups.io/mt/81584577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest

[James Bottomley]

On Wed, 2021-04-07 at 00:21 +, Xu, Min M wrote:
> Hi, Laszlo
> 
> For Intel TDX supported guest, all processors start in 32-bit
> protected
> mode, while for Non-Td guest, it starts in 16-bit real mode. To make
> the
> ResetVector work on both Td-guest and Non-Td guest, ResetVector are
> updated as below:
> --
>   ALIGN   16
>   resetVector:
>   ;
>   ; Reset Vector
>   ;
>   ; This is where the processor will begin execution
>   ;
>   nop
>   nop
>   smswax
>   testal, 1
>   jnz EarlyBspPmEntry
>   jmp EarlyBspInitReal16

Well, then use the rel8 jump like the compiler would in this situation:

  smswax
  testal, 1
  jz  1f
  jmp EarlyBspPmEntry
1:
  jmp EarlyBspInitReal16

So now both entries can be 32k away.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73750): https://edk2.groups.io/g/devel/message/73750
Mute This Topic: https://groups.io/mt/81584577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest

[James Bottomley]

On Tue, 2021-04-06 at 14:16 +0200, Laszlo Ersek wrote:
> On 04/06/21 10:11, Xu, Min M wrote:
> > Hi, Singh
> > I have a concern about the sevSnpBlock in ResetVectorVtf0.asm.
> > Actually
> > SEV has inserted 3 blocks in ResetVectorVtf0.asm and the total
> > bytes are
> > (26 + 22 + 20 = 68 bytes). If sevSnpBlock is added, then the total
> > bytes
> > will be (68 +26 = 94 bytes).
> > 
> > I am not sure whether there will be more blocks added in
> > ResetVectorVtf0.asm in the future. But I don't think
> > ResetVectorVtf0.asm
> > is a good place to add these data blobs. Can these data be packed
> > into a
> > single file, for example, SevMetadata.asm, then a pointer is
> > inserted in
> > ResetVectorVtf0.asm which then points to the SevMetadata. In this
> > way we
> > can keep ResetVectorVtf0.asm clean, small and straight forward.
> > 
> > Another reason is that I am working on the Intel TDX which will
> > update the ResetVectorVtf0.asm as well. My change depends on the
> > assumption that the distance between ResetVector(0xfff0) and
> > EarlyBspInitReal16 is less than 128 bytes. The blocks in
> > ResetVectorVtf0.asm make it impossible.

Why?  I think the short jump can cover 32k as an offset.

> That's a problem. These info blocks are placed in the reset vector
> because then they can be found by QEMU easily -- they are not
> compressed, and they appear at a known location in the guest physical
> address space. (More precisely, a GUID-ed structure chain starts at a
> known location, and then QEMU can traverse the chain of structures,
> for learning various bits of information about the firmware.)
> 
> Do we absolutely need a short jump?

Yes, I explained this before:

https://listman.redhat.com/archives/edk2-devel-archive/2020-November/msg01063.html

Sorry, it's buried in the middle about why we can only have 32k worth
of entries.  However, the restriction is 32k not 128 bytes unless Intel
is doing something strange.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73749): https://edk2.groups.io/g/devel/message/73749
Mute This Topic: https://groups.io/mt/81584577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Maintainers: create the "OvmfPkg: Confidential Computing" subsystem

[James Bottomley]

On Thu, 2021-03-11 at 18:13 +0100, Laszlo Ersek wrote:
> On 03/10/21 19:56, Laszlo Ersek wrote:
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3249
> > 
> > Generalize the current OVMF SEV subsystem entry, so that we can use
> > it for Intel TDX in the future, ensuring proper patch circulation
> > for reviews.
> > 
> > Cc: Andrew Fish 
> > Cc: Ard Biesheuvel 
> > Cc: Brijesh Singh 
> > Cc: James Bottomley 
> > Cc: Jiewen Yao 
> > Cc: Jordan Justen 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > Cc: Min Xu 
> > Cc: Philippe Mathieu-Daudé 
> > Cc: Tom Lendacky 
> > 
> > Thanks
> > Laszlo
> > 
> > Laszlo Ersek (2):
> >   Maintainers: refresh the OVMF SEV subsystem after TianoCore #2198
> > and #3077
> >   Maintainers: rename the OVMF SEV subsystem to "Confidential
> > Computing"
> > 
> >  Maintainers.txt | 28 +---
> >  1 file changed, 18 insertions(+), 10 deletions(-)
> > 
> > 
> > base-commit: edd46cd407ea4a0adaa8d6ca86f550c2a4d5c507
> > 
> 
> For merging this series, I still need ACKs from James and Min Xu,
> please.

Acked-by: James Bottomley 

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72672): https://edk2.groups.io/g/devel/message/72672
Mute This Topic: https://groups.io/mt/81235219/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[James Bottomley]

On Wed, 2021-03-10 at 15:20 +0100, Laszlo Ersek wrote:
[...]
> (2) Reviewing this patch makes me realize we've missed some
> "Maintainers.txt" updates in the past, in relation to SEV and/or
> confidential computing.
> 
> Namely, we did not designated any reviewers for the following
> pathnames:
> 
>   OvmfPkg/AmdSev/
>   OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
>   OvmfPkg/Library/PlatformBootManagerLibGrub/
> 
> (from ;), also
> 
>   OvmfPkg/ResetVector/
> 
> (from ;).
> 
> That should be fixed up before adding anything TDX related (I can
> submit a patch series, but first, the next point needs to be
> cleared.)

I'm happy to be added for all of it ... the first three are all me and
the last one I added something to.

> (3) After racking my brain for half an hour, I can find no good way
> to have TDX/SEV separation *plus* a Confidential Computing section in
> "Maintainers.txt". Whatever I managed to think of requires us to
> either duplicate email addresses, or duplicate pathnames ("F:"
> patterns) -- or even both.
> 
> So... can we simply rename the current SEV subsystem to "Confidential
> Computing", and keep both TDX and SEV modules under it? We could
> place a unified email address list there, with Brijesh, James,
> Jiewen, Min, Tom.
> 
> I don't think this should cause any confusion, because:
> 
> - @intel.com emails are clearly closely associated with TDX, and
> @amd.com emails are clearly closely associated with SEV,
> 
> - most filenames will (or do already) include "AmdSev" or "Tdx",
> 
> - future patches should clearly label themselves as "SEV only", "TDX
> only", or "confidential computing in general" -- this should be clear
> from the patch subjects.

That should work ... it's entirely possible that SecretDxe and
SecretPei can work for Intel as well ... we don't know yet, so they may
not need a prefix.

> IOW, there should be no confusion as to who's required to review
> what, but at the same time we'd have a simple solution for cross-
> posting all interested parties.
> 
> Thoughts?

Works for me ... IBM is interested in both SEV and TDX and having them
be as similar as posisble.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72632): https://edk2.groups.io/g/devel/message/72632
Mute This Topic: https://groups.io/mt/81219131/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [sbsigntools] [ANNOUNCE] sbsigntools version 0.9.4 available

[James Bottomley]

The fixes since 0.9.3 are

   AKASHI Takahiro (1):
  sbsign: allow for adding intermediate certificates

James Bottomley (8):
  sbverify: fix verification with intermediate certificates
  Tests: Add intermediate certificate tests to the sign-verify cases
  Fix some openssl 1.1.0 deprecated functions
  sbvarsign: remove unused global variable
  sbverify: refer to unused function
  Fix errors on 32 bit
  Enable -Werror for builds
  docs: add man page for sbkeysync

The git repository is

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git

The tarball release is

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git/snapshot/sbsigntools-0.9.4.tar.gz

And the openSUSE build service build of various distributions is here:

https://build.opensuse.org/package/show/home:jejb1:UEFI/sbsigntools

James


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61212): https://edk2.groups.io/g/devel/message/61212
Mute This Topic: https://groups.io/mt/74837058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [ANNOUNCE] sbsigntools version 0.9.3 available

[James Bottomley]

On Fri, 2020-01-10 at 11:58 +0100, Laszlo Ersek wrote:
> On 01/09/20 19:24, James Bottomley wrote:
> > The fixes since 0.9.2 are
> > 
> >James Bottomley (1):
> >   README: update git location and add mailing list
> > information
> > 
> > Laszlo Ersek (1):
> >   sbvarsign: fix
> > "EFI_VARIABLE_AUTHENTICATION_2.TimeStamp.Year" assignment
> > 
> > Steve McIntyre (1):
> >   Fix PE/COFF checksum calculation
> > 
> > The git repository is
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.gi
> > t
> > 
> > The tarball release is
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.gi
> > t/snapshot/sbsigntools-0.9.3.tar.gz
> > 
> > And the openSUSE build service build of various distributions is
> > here:
> > 
> > https://build.opensuse.org/package/show/home:jejb1:UEFI/sbsigntools
> 
> Awesome, thank you -- the README update is very welcome. It felt
> strange that the latest commit in Jeremy's repo was from 2012 (IIRC),
> an that I had to consult a Debian bug report for finding your repo :)

You're welcome.  While I'm setting up groups.io mailing lists, I should
probably do the same for efitools.

James


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53165): https://edk2.groups.io/g/devel/message/53165
Mute This Topic: https://groups.io/mt/69584997/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Interpretation of specification

[James Bottomley]

On Fri, 2020-01-10 at 11:55 +0100, Laszlo Ersek wrote:
> Right, that's my understanding too -- fully open lists are not
> supported on groups.io (and at least in the edk2 community, most
> participants don't like fully open posting -- I happen to be a fan of
> open posting, FWIW). Worse, for non-subscribers, the best that
> groups.io offers is white-listing per *message*, and not per
> *sender*.

Yes, that's the most annoying feature of groups.io

It can be worked around though: I've got an auto-reply on the moderator
email using procmail that auto-approves every message held for
moderation ... although I'm not sure that would work well on a higher
traffic list.

> > I also think the group rejected this out of hand because it's not
> > in the moderation queue, but I think I've found the flag that holds
> > non-member posts for moderation and thus allows them to be posted
> > instead of rejected.
> 
> Ugh, I didn't know that just holding messages for moderation took a
> separate setting!

Yes, apparently ... I remember having the same trouble with the
openssl-tpm2-engine group.

James


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53166): https://edk2.groups.io/g/devel/message/53166
Mute This Topic: https://groups.io/mt/36573446/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [ANNOUNCE] sbsigntools version 0.9.3 available

[James Bottomley]

The fixes since 0.9.2 are

   James Bottomley (1):
  README: update git location and add mailing list information

Laszlo Ersek (1):
  sbvarsign: fix "EFI_VARIABLE_AUTHENTICATION_2.TimeStamp.Year" 
assignment

Steve McIntyre (1):
  Fix PE/COFF checksum calculation

The git repository is

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git

The tarball release is

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git/snapshot/sbsigntools-0.9.3.tar.gz

And the openSUSE build service build of various distributions is here:

https://build.opensuse.org/package/show/home:jejb1:UEFI/sbsigntools

James


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53091): https://edk2.groups.io/g/devel/message/53091
Mute This Topic: https://groups.io/mt/69584997/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Interpretation of specification

[James Bottomley]

On Thu, 2020-01-09 at 18:17 +0100, Laszlo Ersek wrote:
> Hello James,
> 
> On 01/08/20 20:13, James Bottomley wrote:
> > On Wed, 2020-01-08 at 12:24 +0100, Laszlo Ersek wrote:
> > > I don't know where sbsigntools development occurs (mailing list,
> > > bug
> > > tracker, ...). For now, I'm CC'ing James. (The git repo lives in
> > > his
> > > space on .)
> > 
> > Heh, that's a sore point: since Jeremy Kerr left ubuntu, no-one
> > except
> > me has been maintaining it; Jeremy owns the original tree on the
> > Ubuntu
> > servers but can't update it any more.
> > 
> > I use groups.io for the openssl_tpm2_engine development list, so I
> > suppose I can set one up for sbsigntools ... OK, done.  If you want
> > to
> > you can package the above up as a patch and send it there and I'll
> > apply it.
> 
> I've now posted:
> 
>   [PATCH] sbvarsign: fix
> "EFI_VARIABLE_AUTHENTICATION_2.TimeStamp.Year" assignment
>   Message-Id: <20200109171302.28782-1-ler...@redhat.com>
> 
> to .
> 
> I didn't (and most likely, won't) subscribe to that list, so please
> whitelist this individual posting of mine, on the <https://sbsigntool
> s.groups.io> admin interface.

Heh, trying to dust off the groups.io setup memories as we speak.  I
can't allow fully unmoderated postings, unfortunately.  I also think
the group rejected this out of hand because it's not in the moderation
queue, but I think I've found the flag that holds non-member posts for
moderation and thus allows them to be posted instead of rejected.

James


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53090): https://edk2.groups.io/g/devel/message/53090
Mute This Topic: https://groups.io/mt/36573446/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Interpretation of specification

[James Bottomley]

On Wed, 2020-01-08 at 12:24 +0100, Laszlo Ersek wrote:
> (+James)
> 
> On 01/07/20 19:13, Eugene Khoruzhenko wrote:
> > I think I may have found the problem. I can write the
> > file_name.signed created by your scripts in NT32 emulated
> > environment and in EDKII on Minnow board that I build myself.
> > However, I cannot write the file_name.signed on a commercial
> > device. I can write the same authenticate variable with the same
> > Name/GUID and same cert/key on a device when I create the payload
> > in a UEFI Shell app. So the only difference is creating the signed
> > payload by sbvarsign in Ubuntu vs doing it in UEFI. I compared both
> > the working and non-working payloads and the main difference I see
> > is in the timestamp. For some reason sbvarsign writes the Year as
> > 0x0078 (120) vs the UEFI app writing 0x07e4 (2020). The
> > month/day/hour/min seems to be OK, but the year is really off in
> > the sbvarsign's payload. I cannot prove it, but I think the
> > commercial firmware may be having a sanity check for the
> > timestamp date/time, e.g. compare with the device manufacture date.
> > Since sbvarsign does not allow setting a timestamp separately, I
> > cannot force it to create a payload with the correct year.
> 
> That looks like a bug in sbvarsign. In UEFI-2.8, the EFI_TIME
> structure is defined under 8.3 "Time Services" / GetTime(). The Year
> field is given like this:
> 
>   UINT16 Year; // 1900 - 
> 
> The comment indicates the valid range. It does not indicate that the
> value stored should be relative to the year 1900 (which is what
> sbvarsign appears to assume; 2020-1900=120).
> 
> The UEFI application likely uses GetTime(), for populating
> "EFI_VARIABLE_AUTHENTICATION_2.TimeStamp" with the current time
> (which is prescribed in 8.2.2 "Using the
> EFI_VARIABLE_AUTHENTICATION_2 descriptor").
> 
> After GetTime() returns successfully, the application may still need
> to manually ensure that "Pad1, Nanosecond, TimeZone, Daylight and
> Pad2" are zeroed. This could involve timezone translation (to the
> required UTC), which in some cases could even imply a change to the
> Year field. But that doesn't change the fact that GetTime() initally
> places an absolute year number in the Year field, and not one
> relative to 1900.
> 
> Let me see the "sbsigntool" source code:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.gi
> t
> 
> (Repository URL via
> .)
> 
> Yup, here's the bug (at current master, that is, commit 0dc3d4b5210d,
> "Fix PE/COFF checksum calculation", 2019-07-27):
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.gi
> t/tree/src/sbvarsign.c?id=0dc3d4b5210dae158651d058f7ac68a9f178ae84#n2
> 15
> 
> Quote:
> 
>199  static int set_timestamp(EFI_TIME *timestamp)
>200  {
>201  struct tm *tm;
>202  time_t t;
>203
>204  time();
>205
>206  tm = gmtime();
>207  if (!tm) {
>208  perror("gmtime");
>209  return -1;
>210  }
>211
>212  /* copy to our EFI-specific time structure. Other
> fields (Nanosecond,
>213   * TimeZone, Daylight and Pad) are defined to be zero
> */
>214  memset(timestamp, 0, sizeof(*timestamp));
>215  timestamp->Year = tm->tm_year;
>216  timestamp->Month = tm->tm_mon;
>217  timestamp->Day = tm->tm_mday;
>218  timestamp->Hour = tm->tm_hour;
>219  timestamp->Minute = tm->tm_min;
>220  timestamp->Second = tm->tm_sec;
>221
>222  return 0;
>223  }
> 
> Please refer to POSIX for time(), gmtime(), and "struct tm":
> 
>   https://pubs.opengroup.org/onlinepubs/9699919799/functions/time.htm
> l
>   https://pubs.opengroup.org/onlinepubs/9699919799/functions/gmtime.h
> tml
>   https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/time.h.ht
> ml
> 
> Quoting the last reference:
> 
> > The  header shall declare the tm structure, which shall
> > include at least the following members:
> > 
> > inttm_sec   Seconds [0,60].
> > inttm_min   Minutes [0,59].
> > inttm_hour  Hour [0,23].
> > inttm_mday  Day of month [1,31].
> > inttm_mon   Month of year [0,11].
> > inttm_year  Years since 1900.
> > inttm_wday  Day of week [0,6] (Sunday =0).
> > inttm_yday  Day of year [0,365].
> > inttm_isdst Daylight Savings flag.
> 
> That is, field "tm_year" is relative to 1900.
> 
> The bug is therefore in set_timestamp(), on line 215. It should be
> 
>   timestamp->Year = 1900 + tm->tm_year;
> 
> The bug goes back to commit 953b00481f39 ("sbvarsign: First cut of a
> variable-signing tool", 2012-08-02).
> 
> (Otherwise, gmtime() is a good choice, because it gives us UTC at
> once, which is what EFI_VARIABLE_AUTHENTICATION_2 requires, per UEFI
> spec ("GMT").)
> 
> I don't know where sbsigntools