Skip to content

OVMF: include UninstallMemAttrProtocol patch #420069

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OVMF: include UninstallMemAttrProtocol patch #420069

wants to merge 1 commit into from

Conversation

surfaceflinger
Copy link
Member

@surfaceflinger surfaceflinger commented Jun 25, 2025 •
edited
Loading

Newer UEFI specs have some memory protection features which break secure boot shim used at least in RHEL 9. Trying to boot RHEL 9 or clones using our OVMF builds will end up with this on serial

!!!! X64 Exception Type - 0E(#PF - Page-Fault)  CPU Apic ID - 00000000 !!!!
ExceptionData - 0000000000000003  I:0 R:0 U:0 W:1 P:1 PK:0 SS:0 SGX:0
RIP  - 000000007516FC60, CS  - 0000000000000038, RFLAGS - 0000000000210006
RAX  - 0000000075188000, RCX - 0000000075188000, RDX - 0000000000024000
RBX  - 0000000000000000, RSP - 000000007EF78FC8, RBP - 000000007D5C7818
RSI  - 0000000000000000, RDI - 0000000075188000
R8   - 00000000751AC000, R9  - 000000007AD6B6FD, R10 - 000000007C6A3DB9
R11  - 0000000000000000, R12 - 000000007E9EC018, R13 - 000000007CCB1000
R14  - 000000007CC1F3B8, R15 - 000000007CC1F3C0
DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
GS   - 0000000000000030, SS  - 0000000000000030
CR0  - 0000000080010033, CR2 - 0000000075188000, CR3 - 000000007EC01000
CR4  - 0000000000000668, CR8 - 0000000000000000
DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
GDTR - 000000007E9E1000 0000000000000047, LDTR - 0000000000000000
IDTR - 000000007E2CB018 0000000000000FFF,   TR - 0000000000000000
FXSAVE_STATE - 000000007EF78C20
!!!! Find image based on IP(0x7516FC60) (No PDB)  (ImageBase=000000000103E1C0, EntryPoint=000000000103EA40) !!!!

This patch is applied in Debian and Fedora, didn't check anywhere else. They additionally enable it by default on non-secureboot variant, in case of this PR It'll be required to add this to libvirtd domain config:

<domain>
  [...]
  <commandline xmlns="http://libvirt.org/schemas/domain/qemu/1.0">
    <arg value='-fw_cfg'/>
    <arg value='opt/org.tianocore/UninstallMemAttrProtocol,string=y'/>
  </commandline>
</domain>

we could enable it too by setting --pcd PcdUninstallMemAttrProtocol=TRUE build flag but idk if this will be correct:

++ lib.optionals (!secureBoot) [ "--pcd PcdUninstallMemAttrProtocol=TRUE" ]

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

Add a 👍 reaction to pull requests you find important.

@surfaceflinger
Copy link
Member Author

420 69 epic

@surfaceflinger surfaceflinger force-pushed the ovmf-UninstallMemAttrPr branch from a8def94 to 11b932c Compare June 25, 2025 23:20
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Jun 25, 2025
@surfaceflinger
Copy link
Member Author

surfaceflinger commented Jun 26, 2025 •
edited
Loading

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review
Commit: 11b932cb61c11ebae3d9eaed966f76e063ba42df

x86_64-linux

⏩ 2 packages marked as broken and skipped:
  • htcondor
  • qubes-core-vchan-xen
❌ 3 packages failed to build:
  • diffoscope
  • diffoscope.dist
  • diffoscope.man
✅ 74 packages built:
  • OVMF
  • OVMF-cloud-hypervisor
  • OVMF-cloud-hypervisor.fd
  • OVMF.fd
  • OVMFFull
  • OVMFFull.fd
  • appvm
  • collectd
  • docker-machine-kvm2
  • fdroidserver
  • fdroidserver.dist
  • gnome-boxes
  • goldboot
  • guestfs-tools
  • libguestfs (python313Packages.guestfs)
  • libguestfs-with-appliance
  • libguestfs-with-appliance.guestfsd
  • libguestfs.guestfsd (python313Packages.guestfs.guestfsd)
  • librenms
  • libvirt
  • libvirt-glib
  • libvirt-glib.dev
  • libvirt-glib.devdoc
  • libvmi
  • libvmi.dev
  • libvmi.lib
  • lxd-lts
  • mgmt
  • minikube
  • multipass
  • ocamlPackages.ocaml_libvirt
  • perl538Packages.SysVirt
  • perl538Packages.SysVirt.devdoc
  • perl540Packages.SysVirt
  • perl540Packages.SysVirt.devdoc
  • podman-bootc
  • prometheus-libvirt-exporter
  • python312Packages.guestfs
  • python312Packages.guestfs.guestfsd
  • python312Packages.libvirt
  • python312Packages.libvirt.dist
  • python312Packages.xen
  • python312Packages.xen.boot
  • python312Packages.xen.dev
  • python312Packages.xen.doc
  • python312Packages.xen.man
  • python313Packages.libvirt
  • python313Packages.libvirt.dist
  • xen (python313Packages.xen)
  • xen.boot (python313Packages.xen.boot)
  • xen.dev (python313Packages.xen.dev)
  • xen.doc (python313Packages.xen.doc)
  • xen.man (python313Packages.xen.man)
  • qemu_xen
  • qemu_xen.debug
  • qemu_xen.doc
  • qemu_xen.ga
  • quickemu
  • quickgui
  • quickgui.debug
  • quickgui.pubcache
  • rubyPackages.ruby-libvirt
  • rubyPackages_3_1.ruby-libvirt
  • rubyPackages_3_2.ruby-libvirt
  • rubyPackages_3_4.ruby-libvirt
  • vagrant
  • virt-manager
  • virt-manager-qt
  • virt-top
  • virt-v2v
  • virt-viewer
  • virtnbdbackup
  • virtnbdbackup.dist
  • xen-guest-agent

diffoscope failing filename tests probably because of me using zfs

@adamcstephens
Copy link
Contributor

420 69 epic

What?

Is there an upstream edk2 report for this patch? If it's critical for some distributions, and both those distros are including it, I would hope to see it being upstreamed.

@surfaceflinger
Copy link
Member Author

What?

look at pr id

Is there an upstream edk2 report for this patch? If it's critical for some distributions, and both those distros are including it, I would hope to see it being upstreamed.

yes, but stalebot closed it tianocore/edk2#10667

also just noticed that Dasharo reverts this completely, so it's also being done on actual hardware Dasharo/edk2#236

@adamcstephens
Copy link
Contributor

It seems like upstream considers this a bug in the shim, and even Debian isn't enabling this by default. Is there any reason you can't disable secureboot until RedHat fixes this on their end?

@surfaceflinger
Copy link
Member Author

Secure boot is off in my vm, these features are always enabled

@adamcstephens
Copy link
Contributor

What's the scope of impact here? Just RHEL-based distros?

@surfaceflinger
Copy link
Member Author

surfaceflinger commented Jun 27, 2025 •
edited
Loading

no idea, I only have NixOS and Alma Linux 9 in vms
all threads I could find on the internet talk about RHEL and clones

@surfaceflinger
Copy link
Member Author

Any chance this gets merged? It shouldn't affect anything unless qemu is started with -fw_cfg opt/org.tianocore/UninstallMemAttrProtocol,string=y

SigmaSquadron
SigmaSquadron approved these changes Aug 25, 2025
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

This looks alright to me, especially since it's only adding an optional patch that won't affect normal OVMF calls, but I'll defer to @adamcstephens' judgement here.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SigmaSquadron SigmaSquadron SigmaSquadron approved these changes

@adamcstephens adamcstephens Awaiting requested review from adamcstephens

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@mjoerg mjoerg Awaiting requested review from mjoerg

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@surfaceflinger @adamcstephens @SigmaSquadron