Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SNP bit checks on the host and ubuntu guest from instruction set #12

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

SNP bit checks on the host and ubuntu guest from instruction set #12

wants to merge 4 commits into from
+113 −0

Conversation

LakshmiSaiHarika
Copy link
Contributor

@LakshmiSaiHarika LakshmiSaiHarika commented Jun 26, 2024 •
edited
Loading

  1. Added CPU check on the host for all host SEV features from cpuid 0x8000001f
  2. Added host SNP enablement check in BIOS from MSR 0xC0010010 bit 23 & 24
  3. Added additional check on the guest to see if guest security bits are active from MSR 0xc0010131 (MSR_AMD64_SEV)

I validated all the added checks in this PR as follows:

  • Tested CPU support check for SNP on Rome, milan to check the CPU support check which works fine
  • Tested SNP enablement check in Bios which shows correct error message when SME/SNP are disabled in BIOS
  • Tested guest SEV/SEV-ES/SNP bit status on SNP guest and normal guest(w/o SNP enabled)

larrydewey
larrydewey requested changes Jul 8, 2024
View reviewed changes
Copy link
Collaborator

@larrydewey larrydewey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Will you please run these through shellcheck? There are a number of changes which I would like to see applied for this PR before we approve it.

@LakshmiSaiHarika
Copy link
Contributor Author

Hi @larrydewey, I made few changes and did shellcheck... Please let me know if any further changes are required.

larrydewey
larrydewey requested changes Aug 5, 2024
View reviewed changes
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the add-snp-check-via-instruction-set branch from 4ba7408 to 4ff4dcd Compare September 28, 2024 00:58
@LakshmiSaiHarika
Copy link
Contributor Author

Hi @larrydewey I addressed the changes as per the conversation above, please let me know if any additional changes are required

DGonzalezVillal
DGonzalezVillal reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@DGonzalezVillal DGonzalezVillal left a comment

Choose a reason for hiding this comment

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

Just some nits and some overall functionality questions. But overall it lgtm.

docs/snp.md Outdated
Comment on lines 55 to 60
Read the dedicated host cpuid Fn8000_001F[EAX] instruction set to verify if the SNP is on and supported on the host:
```
./snp.sh check-host-snp-cpuid
```


Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be it's own individual command? I see that it is being called when people are setting up the host and launching the guest. Is there additional functionality to be it's own individual command?

tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the add-snp-check-via-instruction-set branch from 4ff4dcd to 699d8b1 Compare October 22, 2024 21:48
DGonzalezVillal
DGonzalezVillal reviewed Dec 6, 2024
View reviewed changes
tools/snp.sh Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
tools/snp.sh Outdated Show resolved Hide resolved
@LakshmiSaiHarika LakshmiSaiHarika marked this pull request as draft January 15, 2025 21:40
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the add-snp-check-via-instruction-set branch from 699d8b1 to 0c98f29 Compare January 23, 2025 01:19
@LakshmiSaiHarika LakshmiSaiHarika changed the title Added additional SNP checks on the host and guest via verification of SNP bit status from instruction set Added additional SNP checks on the host and ubuntu guest via verification of SNP bit status from instruction set Jan 23, 2025
@LakshmiSaiHarika
snp.sh: Verify if SNP(security) bits are set
b11f34d 
This module verifies if all the security bits are set to 1 for any given instruction set

Signed-off-by: Harika Nittala <[email protected]>
@LakshmiSaiHarika
snp.sh: Added CPUID check to verify the SNP feature support in host CPU
49d854c 
This verifies if CPU is capable of SNP based on the SNP bit value present in the CPUID 0x8000001f

Signed-off-by: Harika Nittala <[email protected]>
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the add-snp-check-via-instruction-set branch from 0c98f29 to 8e655eb Compare January 24, 2025 04:50
@LakshmiSaiHarika LakshmiSaiHarika marked this pull request as ready for review January 24, 2025 04:53
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the add-snp-check-via-instruction-set branch from 8e655eb to 235f4a8 Compare January 24, 2025 18:11
@LakshmiSaiHarika LakshmiSaiHarika changed the title Added additional SNP checks on the host and ubuntu guest via verification of SNP bit status from instruction set New SNP bit checks on the host and ubuntu guest from instruction set Jan 24, 2025
@LakshmiSaiHarika LakshmiSaiHarika changed the title New SNP bit checks on the host and ubuntu guest from instruction set SNP bit checks on the host and ubuntu guest from instruction set Jan 24, 2025
AdithyaKrishnan
AdithyaKrishnan reviewed Jan 27, 2025
View reviewed changes
tools/snp.sh Outdated
verify_host_snp_enablement() {
echo -e "Verifying if SME, SNP are enabled in the host from MSR 0xC0010010..."

sudo modprobe msr
Copy link

Choose a reason for hiding this comment

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

if ! sudo modprobe msr; then
>&2 echo "ERROR: Failed to load MSR kernel module. Ensure you have the necessary sudo permissions."
return 1
fi

AdithyaKrishnan
AdithyaKrishnan reviewed Jan 27, 2025
View reviewed changes
tools/snp.sh

case ${guest_linux_distro} in
ubuntu)
ssh_guest_command "sudo DEBIAN_FRONTEND=noninteractive sudo apt install -y msr-tools > /dev/null 2>&1" > /dev/null 2>&1
Copy link

Choose a reason for hiding this comment

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

Are we assuming that the user will have sudo permissions by default?
If yes, disregard my comments.
Else, we might have to modify the code to handle that permission errors aren't misconstrued as actual errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I tested this, this works by default

AdithyaKrishnan
AdithyaKrishnan reviewed Jan 27, 2025
View reviewed changes
Copy link

@AdithyaKrishnan AdithyaKrishnan left a comment

Choose a reason for hiding this comment

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

I believe this code has already been reviewed multiple times.
I couldn't find any major new suggestions apart from what was already provided to you.

@LakshmiSaiHarika
snp.sh: Added MSR check to confirm if SNP is enabled in host BIOS
0136c39 
This verifies if SME, SNP are enabled in the host BIOS settings by reading SME and SNP bit status from MSR 0xC0010010

Bit amd#23 corresponds to the SME bit status
Bit amd#24 corresponds to the SNP bit status

Signed-off-by: Harika Nittala <[email protected]>
@LakshmiSaiHarika
snp.sh: Added guest MSR check to verify SNP bit status
e82f96e 
Added  MSR 0xC0010010 check to validate if guest SEV, SEV-ES and SNP are enabled by reading SEV, SEV-ES and SNP bits from MSR 0xC0010010 instruction set

Bit #0 corresponds to the SEV bit status
Bit amd#1 corresponds to SEV-ES bit status
Bit amd#2 corresponds to SNP bit status

Signed-off-by: Harika Nittala <[email protected]>
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the add-snp-check-via-instruction-set branch from 235f4a8 to e82f96e Compare January 28, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larrydewey larrydewey Awaiting requested review from larrydewey

@DGonzalezVillal DGonzalezVillal Awaiting requested review from DGonzalezVillal

@AdithyaKrishnan AdithyaKrishnan Awaiting requested review from AdithyaKrishnan

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LakshmiSaiHarika @AdithyaKrishnan @larrydewey @DGonzalezVillal