efi/preinstall: enable checks on arm64 platforms, and DGX Spark in particular - #560
efi/preinstall: enable checks on arm64 platforms, and DGX Spark in particular#560alexclewontin wants to merge 5 commits into
Conversation
CheckResult.Warnings is a CompoundError interface that RunChecks leaves nil when no warnings were detected. Unwrap on a nil interface panics, so only unwrap when warnings are present.
05341c2 to
722fa4c
Compare
|
|
||
| // TODO: Implement proper debug authentication checks, once we have the documentation | ||
| // from NVIDIA to do so. | ||
| return platformFirmwareIntegrityMeasured, nil |
There was a problem hiding this comment.
Should you raise a not implemented error instead of returning a success?
There was a problem hiding this comment.
There was a problem hiding this comment.
This is just a placeholder, and exactly the thing that should change before this gets merged
| } | ||
|
|
||
| // isTPMDiscreteNvidia determines whether the default TPM is discrete on NVIDIA systems | ||
| func isTPMDiscreteNvidia(env internal_efi.HostEnvironmentARM64) (bool, error) { |
There was a problem hiding this comment.
AI tells me:
TPM classification can reject a TPM that was already opened successfully
After
RunCheckshas opened and exercised the default TPM, the ARM64 reset-mitigation check independently searches sysfs forDEVNAME=tpm0and requires exactly one result with a readable parent. If udev enumeration returns zero or duplicate matches, cannot read the parent, or represents the opened TPM differently, the whole check now fails withTPM2DeviceErroreven though the TPM itself was usable.
This second lookup is not correlated with theTPMDeviceselected earlier. It should either classify the already-selected device or treat unavailable driver metadata as an inability to determine the optional reset mitigation, rather than as a fatal TPM-device failure. Tests should cover zero/multiple matches and parent/enumeration errors throughRunChecks, not only the direct happy paths.
There was a problem hiding this comment.
However, when checking the DGX Spark, we are indeed in the situation of the happy path.
|
|
||
| // isTPMFirmwareOptee determines whether the default TPM is an OP-TEE firmware TPM, | ||
| // as identified by its backing kernel driver | ||
| func isTPMFirmwareOptee(env internal_efi.HostEnvironment) (bool, error) { |
There was a problem hiding this comment.
In the spirit of keeping things minimal, do we want to handle optee TPMs in this enablement?
There was a problem hiding this comment.
It is a fair question. The only reason I did include it is because it is as close to a non-fragmented, cross-platform API as I could come up with on ARM (I for a while tried to include CoreSight detection in a crossplatform way but determined it was not meaningfully possible)
| errs = append(errs, ce.Unwrap()...) | ||
| } | ||
|
|
||
| if err := checkForKernelIOMMU(env); err != nil { |
There was a problem hiding this comment.
AI tells me:
The new generic ARM64 check treats the existence of any device in the
iommusubsystem as sufficient DMA protection. This DGX Spark exposes three SMMUs under/sys/class/iommu, so that check succeeds, but the boot log reportsiommu: Default domain type: Passthrough. SMMU availability therefore does not establish that devices, particularly externally accessible PCIe devices, are actually attached to translated/protected domains.
Before using this as the ARM64 DMA-security gate, the implementation should verify the relevant devices' active IOMMU domains or otherwise establish that passthrough cannot leave attacker-accessible devices unprotected.
There was a problem hiding this comment.
Yeah AI flagged this for me too. I think a similar gap exists on the AMD64 side, but it is possible that presence of an IOMMU is a weaker guarantee on ARM64 than it is on AMD64? I'll have to dive into that a bit
There was a problem hiding this comment.
So I think in the normal case, the guarantees are basically the same: per the comments above the function definition we are not 100% sure that iommu device existence is sufficient, so those guarantees may not be sufficient, but I don't think they are inherently weaker on ARM.
On ARM64, there are a couple of extra escape hatches (CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=false, arm-smmu.disable-bypass=0). We should ideally not be shipping kernels with the first. Core has a way to put controls on kcmdline arguments, not sure if desktop w FDE has the same. If it does we would deny the second.
I agree that there is danger in allowing iommu passthrough as a default domain type (via kconfig or kcmdline)... but I believe that danger is arch agnostic, and also can change per kernel and even per boot, so checks on the current boot, often in the context of the installer, should not be considered authoritative.
However it is interesting that your Spark has iommu: Default domain type: Passthrough. I checked mine, and I have iommu.passthrough=0 on the command line and further down the boot log
Aug 12 18:08:24 apollo kernel: iommu: Default domain type: Translated (set via kernel command line)
Aug 12 18:08:24 apollo kernel: iommu: DMA domain TLB invalidation policy: lazy mode
We should get to the bottom of that disparity
There was a problem hiding this comment.
However the -nvidia kernel looks like it is configured with
# CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT is not set
# CONFIG_IOMMU_DEFAULT_DMA_STRICT is not set
# CONFIG_IOMMU_DEFAULT_DMA_LAZY is not set
CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y
😱
There was a problem hiding this comment.
@alexclewontin FWIW I also have
mitchell@spark-prod:~$ sudo dmesg | grep "iommu" | grep "Passthrough"
[ 0.105847] iommu: Default domain type: Passthrough
on my Spark's stock Resolute install.
On its Noble-based FastOS partition though, I have the same as you with iommu.passthrough=0 being set (via /etc/default/grub.d/iommu.cfg).
There are a handful of other grub cmdline differences that NVIDIA ships in FastOS (and I don't think we include every single one in dgx-desktop-defaults) - we will probably just need to keep a close eye on these and make sure that anything that needs to be built into the snapped kernel image for FastOS to work does end up there.
There was a problem hiding this comment.
Looks mostly good, and most of the boilerplate looks analogous to the existing amd64 TPM device implementations.
I do have some questions and one important suggested change inline around your usage of /sys/class/dmi/id/product_name as a device identifier.
I did not thoroughly review the tests yet (assuming it may be best for me to do that after the implementation has stabilized), and will need to come back after the stubs are implemented to review those.
|
|
||
| // ARM64 has no generic mechanism to establish that the TPM startup locality | ||
| // is protected by the hardware root of trust, so PCR0 binding cannot be relied | ||
| // on to mitigate an independent reset of a discrete TPM. |
There was a problem hiding this comment.
The claim that there's no generic mechanism for this seems essentially correct based on my read of the spec.
However, did we confirm that this definitely isn't relevant for Spark platforms specifically? (i.e., do we know for sure whether or not the startup locality is available to the OS? (since it seems like that's determining factor of whether this mitigation is enabled in the amd64 implementation)?)
| } | ||
|
|
||
| switch parent.Properties()["DRIVER"] { | ||
| case "optee-ftpm", "ftpm-tee": |
|
|
||
| var ( | ||
| dmiSystemVendorPath = "/sys/class/dmi/id/sys_vendor" | ||
| dmiProductNamePath = "/sys/class/dmi/id/product_name" |
There was a problem hiding this comment.
The only thing that stays the same accross the entire DGX Spark family s product_family.
Here are examples of the modalias for different oem devices. I collected these before kernel 6.19 so the product family was not in there yet. I should definitely have asked for the entire dmidecode output at the time...
# prototypes - the one alex has
dmi:bvnAmericanMegatrendsInternational,LLC.:bvr5.36_0ACUM021:bd09/26/2025:br5.36:svnNVIDIA:pnDGXSpark:pvr5.36_0ACUM021:rvnNVIDIA:rnDGXSpark:rvr5.36_0ACUM021:cvnDefaultstring:ct3:cvrDefaultstring:skuDefaultstring:
# founder edition
dmi:bvnAmericanMegatrendsInternational,LLC.:bvr5.36_0ACUM018:bd08/06/2025:br5.36:svnNVIDIA:pnNVIDIA_DGX_Spark:pvrA.7:rvnNVIDIA:rnP4242:rvrA04:cvnNVIDIA:ct17:cvrA04:sku0000:
# lenovo
dmi:bvnLENOVO:bvr S0QKTOAA:bd10/21/2025:br1.10:efr2.66:svnLENOVO:pn30KL0005
GF:pvrThinkStationPGX:rvnLENOVO:rnINVALID:rvrNotDefined:cvnLENOVO:ct3:cvrN
one:skuLENOVO_MT_30KL_BU_Think_FM_ThinkStationPGX:
# dell
dmi:bvnDellInc.:bvr5.36_1.1.1:bd08/15/2025:br5.36:svnDellInc.:pnDellProMaxwithGB10FCM1253:pvr:rvnDellInc.:rn038RY7:rvrA00:cvnDellInc.:ct3:cvr:sku0DA7:
# asus
dmi:bvnAmericanMegatrendsInternational,LLC.:bvrGX10DGX.0102.2025.1111.1531:bd11/11/2025:br5.36:svnASUSTeKCOMPUTERINC.:pnGX10:pvr5.36_GX10DGX:rvnASUSTeKCOMPUTERINC.:rnGX10:rvrDefaultstring:cvnASUSTeKCOMPUTERINC.:ct17:cvrDefaultstring:skuDefaultstring:
There was a problem hiding this comment.
So after digging, if this is about identifying the manufacturer to stay aligned with the x86 implementation
- x86 relies on /proc/cpuinfo vendor_id for that identification
- we can't because arm does not expose it. However we can identify the CPU in several ways, for example
- Cortex-A725 / Cortex X925 with lscpu
- The manufacturer of the processor with
sudo dmidecode -t 4and a bit of parsing. I confirm the manufacturer is NVIDIA for Spark's founder edition and for the laptops
I'd go with option 2.
However, in the end, that's simply to determine if the TPM is discrete or not. I see that you hardcode the fact that DGX Spark has a discrete TPM. For that, I would use the product family.
There was a problem hiding this comment.
Instead of sudo dmidecode -t 4 I'd suggest using dmidecode --string processor-version or the likes to avoid having to parse potentially differently structured output.
antlassagne
left a comment
antlassagne
left a comment
There was a problem hiding this comment.
Here is a deeper review. Overall my understanding is
- It brings up the whole arm64 checks structure, which makes it a bit verbose
- The actual path that matters is
- "is it arm? yes --> is it nvidia? yes --> is it DGX Spark? --> yes--> TPM is discrete.
- Then the TCG event log is analyzed
- Then we skip HW ROT fusing checks
There is definitely something fishy in the 2 test files that has some many changes.
| } | ||
|
|
||
| // checkHostSecurity is the main entry point for verifying that the host security | ||
| // is sufficient. Errors that can't be resolved or which should prevent further checks from running |
| func checkHostSecurityARM64Generic(env internal_efi.HostEnvironment, log *tcglog.Log, integrity platformFirmwareIntegrityConfig) (platformFirmwareIntegrityConfig, error) { | ||
| var errs []error | ||
|
|
||
| if err := checkSecureBootPolicyPCRForDegradedFirmwareSettings(log); err != nil { |
There was a problem hiding this comment.
That would need a test against what's expected on DGX Spark ideally
| } | ||
|
|
||
| func checkHostSecurityNVIDIADGXSpark(env internal_efi.HostEnvironmentARM64) (platformFirmwareIntegrityConfig, error) { | ||
| // TODO: Implement proper HW ROT fusing checks, once we have the documentation |
There was a problem hiding this comment.
Placeholder to keep a thread open until these TODOs are filled
| @@ -1,7 +1,7 @@ | |||
| // -*- Mode: Go; indent-tabs-mode: t -*- | |||
There was a problem hiding this comment.
This file currently is +4829 -5962
Maybe it's expected, but for the sake of having this branch accepted faster by the maintainers is it possible to make the diff smaller?
There was a problem hiding this comment.
I am going to look at @frederic-hoerni's comment about breaking up the commits a bit. Basically, the churn in this and checks_test.go come from taking all of the tests from
func Test() {
const fixtureData = foo
doAThing(fixtureData)
doASecondThing(fixtureData)
}
to
fixtures := [foo, bar, baz]
func Test() {
for fixtureData in fixtures {
doAThing(fixtureData)
doASecondThing(fixtureData)
}
}
The issue is essentially that in this example, fixture data is a bunch of super intel-specific tests that wouldn't meaningfully exercise the arm64 codepaths. We could keep them how they are, but then we would have to add a bunch of redundant new code testing the arm64 codepaths. This way, we can piggyback on the logic encoded in the doAThing(), doASecondThing() sequence and apply it to any arbitrary test fixture
There was a problem hiding this comment.
Also FWIW if you ignore whitespace-only changes:
$ git diff -w --stat 02f4ce691cbebc106ca3485ecd75f16aaec510da^!
efi/preinstall/check_tpm_intel_test.go | 4 +-
efi/preinstall/checks_amd64_test.go | 107 +++++
efi/preinstall/checks_context_amd64_test.go | 107 +++++
efi/preinstall/checks_context_test.go | 2463 ++++++++++++++++++++++++++++---------------------------------------------------------------------------
efi/preinstall/checks_fixture_amd64_test.go | 112 +++++
efi/preinstall/checks_fixture_null_test.go | 45 ++
efi/preinstall/checks_fixture_test.go | 96 ++++
efi/preinstall/checks_test.go | 1573 ++++++++++++++++++------------------------------------------------
efi/preinstall/export_amd64_test.go | 3 +-
efi/preinstall/export_test.go | 3 +-
internal/efi/{default_env_null.go => default_env_amd64_null.go} | 2 +-
internal/efi/{default_env_not_amd64_test.go => default_env_amd64_null_test.go} | 2 +-
12 files changed, 1553 insertions(+), 2964 deletions(-)
Still big, but not as bad
|
|
||
| // HostEnvironmentARM64 is an interface that abstracts out a host environment specific | ||
| // to ARM64 platforms. | ||
| type HostEnvironmentARM64 interface { |
There was a problem hiding this comment.
One more occurrence of the open question about identifying the devices
|
How can I run the arm64 tests on my amd64 laptop? This shows that they are not executed: It would be good to be able to run all unit tests whichever the architecture of the development computer (eg: on amd64). |
There is unfortunately no great way. The issue is that even if the tests are made not-arch-specific, the code paths that are being tested (in this case "does the arm64 version of CheckHostSecurity correctly reject being run on a not-arm64 platform?") are still arch specific (and in this case amd64 platforms don't even try to compile the arm64 version of CheckHostSecurity). And AFAIK go doesn't let you override build constraints for testing purposes The options I can think of off the top of my head are a) qemu (I have tested this on my amd64 workstation): b) move to runtime dispatch
c) split the files up into some spaghetti:
take all names & psuedocode with a grain of salt, but hopefully illustrative |
It makes it more complicated for developers to run unit tests and debug. Not to mention CI tests.
I would favor this one.
Not this one. And all the legacy |
722fa4c to
e85e7c0
Compare
Refactored. github.com/canonical/cpuid will only compile on amd64, so there is still a little bit of the old scheme left there. I have a patch for that package which would mock some of the ASM on other platforms, to allow the use of constants across platforms |
MitchellAugustin
left a comment
MitchellAugustin
left a comment
There was a problem hiding this comment.
Platform matching criteria LGTM. No immediate concerns with the rest, but are the deleted _amd64 files intentional here?
Deferring my review of the tests again until we have the HW ROT fusing items + any other known outstanding bits implemented
| // Check for "Silicon Debug Interface", returned in bit 11 of %ecx when calling | ||
| // cpuid with %eax=1. | ||
| debugSupported := env.HasCPUIDFeature(cpuid.SDBG) | ||
| debugSupported := env.HasCPUIDFeature(internal_efi.CPUIDFeatureSDBG) |
There was a problem hiding this comment.
What's the reason behind changing these intel checks?
| // - bytes 4…: remaining formatted area | ||
| // - after the formatted area: NUL-terminated strings; string set ends | ||
| // with an additional NUL (empty string sentinel) | ||
| func decodeSMBIOSType4Field(data []byte, fieldOffset uint8) (string, error) { |
There was a problem hiding this comment.
This function LGTM, I copied this function and ran it on both devices, and it worked as intended on both. Also LGTM when comparing with the table spec as described on page 43 here.
(I recall you mentioning a back and forth between @antlassagne and @aglinserer on the feasibility of this but could not find where that conversation was)
| // variant (prefix match on "NVIDIA RTX Spark"). | ||
| func isNvidiaSparkCPUVersion(cpuVersion string) bool { | ||
| return cpuVersion == nvidiaDGXSparkCPUVersion || strings.HasPrefix(cpuVersion, nvidiaRTXSparkCPUVersionPrefix) | ||
| } |
There was a problem hiding this comment.
These match criteria LGTM
5 participants
Opening a draft PR for initial feedback.
This is quite noisy, but a significant portion of the delta is just refactoring checks_test.go and checks_context_test.go to not be so Intel-hyperspecific (and even then, it's mostly just putting the body of each test into a per-fixture loop, so if you look at whitespace insensitive diffs it's much much smaller).
Interested in feedback on the approach at the platform level: the ARM ecosystem is a bit fragmented so I don't really see any way around the switch on vendor/switch on platform approach. Is DMI a sufficient source of info here?
NVIDIA will provide documentation on reading HW ROT and debugger fusing, which will be incorporated before this should be merged.
(first two things are tiny and things that Claude kept calling out so I figured I'd just throw them in as standalone commits)