Skip to content

Conversation

@Xynnn007
Copy link
Member

For CoCo, initdata will have a new field to control debugging mode. This will not influence the original overall initdata spec for it will only appear in [data] field.

For CoCo, initdata will have a new field to control debugging mode. This
will not influence the original overall initdata spec for it will only
appear in `[data]` field.

Signed-off-by: Xynnn007 <[email protected]>
For all-in-one KBS, the code changes upon the attestation-service, rvps
and deps/verifier, deps/eventlog will also impact the logic of the KBS.
This patch enables KBS Rust test suites when changes are added upon
those paths.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
Copy link
Member Author

Xynnn007 commented Aug 18, 2025

   Compiling hyper-tls v0.6.0
error[E0433]: failed to resolve: could not find `x86_64` in `arch`
Error:    --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/sev-6.2.1/src/firmware/host/mod.rs:228:45
    |
228 |         let raw_cpuid = unsafe { std::arch::x86_64::__cpuid(0x8000_0001) }
    |                                             ^^^^^^ could not find `x86_64` in `arch`

   Compiling chrono-tz v0.10.3
   Compiling crossbeam-channel v0.5.15

The build error might be related to #902, where KBS Rust test suites were not triggered.

The new version of az-vtpm uses an underlying sev build (SEV) version 6.2.1, which contains the arch-specific statements shown in the error message. Previously, the SEV 6.2.1 crate was located in the verifier crate, and its build was controlled by a feature, which did not trigger CCA build tests.

Now, az-vtpm is referenced by the ITA feature. Therefore, when the ITA feature is enabled, both az-vtpm and SEV 6.2.1 will be built, resulting in an error on the Arm platform.

For CI coverage, Let me add a commit to update KBS Rust CI to be triggered whenever changes introduced in deps/*, attestation-service/* or rvps/* first.

For this issue, probably introducing a x86-64 feature in az-vtpm crate would help? cc @mkulke @yafu-1

@yafu-1
Copy link
Contributor

yafu-1 commented Aug 20, 2025

The new version of az-vtpm uses an underlying sev build (SEV) version 6.2.1, which contains the arch-specific statements shown in the error message. Previously, the SEV 6.2.1 crate was located in the verifier crate, and its build was controlled by a feature, which did not trigger CCA build tests.

Now, az-vtpm is referenced by the ITA feature. Therefore, when the ITA feature is enabled, both az-vtpm and SEV 6.2.1 will be built, resulting in an error on the Arm platform.

For CI coverage, Let me add a commit to update KBS Rust CI to be triggered whenever changes introduced in deps/*, attestation-service/* or rvps/* first.

For this issue, probably introducing a x86-64 feature in az-vtpm crate would help?

Sorry for that. How about disabling the ITA feature on aarch64 since it's not meaningful for aarch64 platforms?

@mythi
Copy link
Contributor

mythi commented Aug 20, 2025

How about disabling the ITA feature on aarch64 since it's not meaningful for aarch64 platforms?

I think you meant to say it's not a very likely setup people would use and I cannot disagree with that. However we need a sustainable solution. Hiding errors may make things worse in the long run.

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this.

This will not influence the original overall initdata spec for it will only appear in [data] field.

This is what I was saying in confidential-containers/confidential-containers#303. The init-data spec allows for anything to be written into the data section, but we do not follow this in kata.

@Xynnn007
Copy link
Member Author

@yafu-1 It can be a good workaround. At the same time we can highlight the issue inside code, use something like a TODO: comment, as @mythi mentioned to explicitly record it. Would you like to put a PR for that?

@mythi
Copy link
Contributor

mythi commented Aug 21, 2025

@yafu-1 It can be a good workaround. At the same time we can highlight the issue inside code, use something like a TODO: comment, as @mythi mentioned to explicitly record it. Would you like to put a PR for that?

Is there a reason to keep the commit that added the failure? Perhaps just revert that.

@yafu-1
Copy link
Contributor

yafu-1 commented Aug 21, 2025

@yafu-1 It can be a good workaround. At the same time we can highlight the issue inside code, use something like a TODO: comment, as @mythi mentioned to explicitly record it. Would you like to put a PR for that?

Sure. I can handle that.

@yafu-1
Copy link
Contributor

yafu-1 commented Aug 21, 2025

@yafu-1 It can be a good workaround. At the same time we can highlight the issue inside code, use something like a TODO: comment, as @mythi mentioned to explicitly record it. Would you like to put a PR for that?

Is there a reason to keep the commit that added the failure? Perhaps just revert that.

In my understanding, to support SNP attestation reports newer than v2 on azure CVM, that commit is required.

@mythi
Copy link
Contributor

mythi commented Aug 21, 2025

@yafu-1 It can be a good workaround. At the same time we can highlight the issue inside code, use something like a TODO: comment, as @mythi mentioned to explicitly record it. Would you like to put a PR for that?

Is there a reason to keep the commit that added the failure? Perhaps just revert that.

In my understanding, to support SNP attestation reports newer than v2 on azure CVM, that commit is required.

OK, sounds like we cannot support that until the sev crate is fixed.

@yafu-1
Copy link
Contributor

yafu-1 commented Aug 21, 2025

In my understanding, to support SNP attestation reports newer than v2 on azure CVM, that commit is required.

OK, sounds like we cannot support that until the sev crate is fixed.

Just to confirm, do you mean building intel-trust-authority-as on aarch64 failed here?

@mythi
Copy link
Contributor

mythi commented Aug 21, 2025

In my understanding, to support SNP attestation reports newer than v2 on azure CVM, that commit is required.

OK, sounds like we cannot support that until the sev crate is fixed.

Just to confirm, do you mean building intel-trust-authority-as on aarch64 failed here?

I mean my opinion is we should revert rather than disable ITA on aarch64.

@yafu-1
Copy link
Contributor

yafu-1 commented Aug 21, 2025

Just to confirm, do you mean building intel-trust-authority-as on aarch64 failed here?

I mean my opinion is we should revert rather than disable ITA on aarch64.

Since sev is intended for x86_64, it's also not suitable to fix in sev crate. Do you have any ideas to fix the issue?

@mythi
Copy link
Contributor

mythi commented Aug 21, 2025

Just to confirm, do you mean building intel-trust-authority-as on aarch64 failed here?

I mean my opinion is we should revert rather than disable ITA on aarch64.

Since sev is intended for x86_64, it's also not suitable to fix in sev crate. Do you have any ideas to fix the issue?

sev crate was working fine even on arm until the 6.2.x releases when they added this cpuid check (at least for the ITA feature where it's totally fine to have an ARM Trustee binary for the kbs+remote AS functionality). I'm not sure that cpuid check is needed for the snp report/certificate checks or not but on a high-level I think Trustee should run the same on x86 and arm (we have some caveats like Intel not publishing arm .deb packages for quote verification libs).

One idea for the fix to make az-cvm-vtpm to not unconditionally depend on sev on a workspace level: https://github.com/kinvolk/azure-cvm-tooling/blob/29ef1581561e85a3a2313f80c738aabe064c4bc4/az-cvm-vtpm/Cargo.toml#L48?

@mythi
Copy link
Contributor

mythi commented Aug 21, 2025

One idea for the fix to make az-cvm-vtpm to not unconditionally depend on sev on a workspace level: https://github.com/kinvolk/azure-cvm-tooling/blob/29ef1581561e85a3a2313f80c738aabe064c4bc4/az-cvm-vtpm/Cargo.toml#L48?

But I think sev needs fixing too. Perhaps they could have more feature gates for selected functionality.

@mkulke
Copy link
Contributor

mkulke commented Aug 21, 2025

sev crate was working fine even on arm until the 6.2.x releases when they added this cpuid check (at least for the ITA feature where it's totally fine to have an ARM Trustee binary for the kbs+remote AS functionality). I'm not sure that cpuid check is needed for the snp report/certificate checks or not but on a high-level I think Trustee should run the same on x86 and arm (we have some caveats like Intel not publishing arm .deb packages for quote verification libs).

I think I'd agree with @mythi that the culprit is probably in the sev crate (or how we use it. but I think there is no way to import SNP types without that cpuid code path being compiled currently). It should be ok in principle to have a verifier on ARM verify evidence from x86_64 TEEs.

cc @DGonzalezVillal

@mkulke
Copy link
Contributor

mkulke commented Aug 21, 2025

sth like:

        // Determine SEV-SNP CPU generation in order to parse platform status accordingly.
        // On non-x86_64 targets, bail out with an Unknown error so the crate still compiles.
        #[cfg(target_arch = "x86_64")]
        let raw_cpuid = unsafe { std::arch::x86_64::__cpuid(0x8000_0001) }
            .eax
            .to_le_bytes();
        #[cfg(not(target_arch = "x86_64"))]
        {
            return Err(crate::error::UserApiError::Unknown);
        }

@yafu-1
Copy link
Contributor

yafu-1 commented Aug 29, 2025

@mythi @mkulke Thanks for the suggestion, and sorry for the late reply — I just got back from holiday. A fix has already been merged in the sev crate by @fitzthum:
virtee/sev#329

@fitzthum
Copy link
Member

fitzthum commented Sep 5, 2025

@yafu-1 can you rebase this PR

@Xynnn007
Copy link
Member Author

Xynnn007 commented Sep 5, 2025

@yafu-1 can you rebase this PR

Oh. Let me do this. But before that, we are involving another discussion about this kata-containers/kata-containers#11587 (comment)

some points are raised by @burgerdev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants