Change save/restore nvram data logic

antonios-f commented 9 months ago

Save NVRAM data not only if BiosType is Q35_SECURE_BOOT Also save if Q35_OVMF because it also includes EFI boot variables With Q35_OVMF we unable to boot non-fallback bootloaders in place different from /EFI/BOOT/BOOT.EFI

To check this it needed to: 1) Run VM with installed Linux (it doesn't matter what distro) 2) Add a bootloader via efibbotmgr efibootmgr -c -d -L "Some Linux" -l \EFI\\grubx64.efi 3) Look at created efi boot variable efibootmgr -v It's present 4) Shutdown VM 5) Run it again 6) Check efi boot variables again efibootmgr -v 7) We got non bootable VM if fallback bootloader is broken or boot default esp/EFI/BOOT/BOOT.EFI otherwise

mz-pdm commented 7 months ago

The idea looks OK to me, IIRC there are no inherent limitations when keeping NVRAM data. It adds some Vdsm traffic and other machinery, even if the EFI variables are not modified I think, but this doesn't differ from using secure boot.

One thing I suggest to check with this patch is whether the NVRAM data is deleted when switching the VM to secure boot and back. Should it be deleted or not?

antonios-f commented 7 months ago

NVRAM data will be deleted when switching the VM from secure boot, by this trigger

CREATE OR REPLACE FUNCTION remove_nvram_data ()
RETURNS TRIGGER AS $$
BEGIN
    IF OLD.bios_type = 4 AND NEW.bios_type != 4 THEN
        DELETE FROM vm_nvram_data
        WHERE vm_id = OLD.vm_guid;
    END IF;
    RETURN NEW;
END;$$
LANGUAGE plpgsql;

CREATE TRIGGER remove_nvram_data_on_update AFTER
UPDATE
    ON vm_static
FOR EACH ROW
EXECUTE FUNCTION remove_nvram_data();

When switching from Uefi to Secure Boot it will be not deleted.

mz-pdm commented 7 months ago

Now the question is whether it's good to delete the NVRAM data when switching from secure boot. I'd say it's on the safe side -- deleting any possible secrets stored there. It will delete the other EFI variables too though but one shouldn't switch an installed VM from secure boot without a good reason so it doesn't look like a big problem. Do you agree?

antonios-f commented 7 months ago

I think yes.

smelamud commented 7 months ago

CREATE OR REPLACE FUNCTION remove_nvram_data () RETURNS TRIGGER AS $$ BEGIN IF OLD.bios_type = 4 AND NEW.bios_type != 4 THEN

Since now bios_type = 3 can also contain NVRAM data. So, if I understand correctly, switching from it should also delete the data.

smelamud commented 7 months ago

@mz-pdm With this patch, NVRAM data will be available with BIOS type Q35_OVMF (bios_type = 3). But remove_nvram_data will not delete it when switching bios_type from 3 to 2, for example. While it is deleted when switching from 4 to 2. Correct me, if I'm wrong.

mz-pdm commented 7 months ago

@smelamud Ah, I see your point now. Do you think it should be deleted in such a case? There is some reason to delete it when switching from secure boot (due to possible presence of secrets, not to keep them around unnecessarily). Perhaps as a general cleanup in other situations? A related question would be whether there is a confirmation dialog for the given deletion that should be updated too.

smelamud commented 7 months ago

@mz-pdm Yes, as a general cleanup, I think. And the confirmation dialog should be shown, because BIOS type change may be unintentional.

antonios-f commented 6 months ago

The confirmation dialog was added, as you described. Please look at this.

antonios-f commented 4 months ago

Everything has been fixed as posted. Please look at this.

barpavel commented 3 months ago

Everything has been fixed as posted. Please look at this.

Except 2 small suggestions as a follow-up of the previous code review, LGTM.