Re: [PATCH qemu-server v3 11/19] api: check the custom firmware of the bios option

[Fiona Ebner <f.ebner@proxmox.com>]

Am 27.10.26 um 11:14 AM schrieb Christian Ludwig:
> When the bios option is set via the POST/PUT {vmid}/config endpoints,
> check that the user may access the custom firmware image, that it is an
> efi-firmware volume, and that the BIOS type is 'ovmf'. Like for ISO
> images, whether the image is usable on the node is only checked when the
> VM is started. The option keeps requiring the VM.Config.Options
> permission.
> 
> A cloned VM references the same firmware image, like an ISO image, so a
> clone is only considered shared if the image is on a shared storage.
> 
> Report custom firmware images in the local disks of the migration
> precondition check, so that clients can tell why a migration is not
> possible.
> 
> Signed-off-by: Christian Ludwig <christian_ludwig@genua.de>
> ---
>  src/PVE/API2/Qemu.pm | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index 922c6599..3338edc2 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -29,6 +29,7 @@ use PVE::GuestImport;
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
>  use PVE::QemuServer::Agent;
> +use PVE::QemuServer::BIOS;
>  use PVE::QemuServer::Blockdev;
>  use PVE::QemuServer::BlockJob;
>  use PVE::QemuServer::Cloudinit;
> @@ -107,6 +108,20 @@ my $foreach_volume_with_alloc = sub {
>      }
>  };
>  
> +my $check_bios_param = sub {

Style nit: please use "my sub check_bios_param {"

> +    my ($param) = @_;
> +
> +    return if !defined($param->{bios});
> +
> +    eval {
> +        PVE::QemuServer::BIOS::check_bios(PVE::QemuServer::BIOS::parse_bios($param->{bios}));
> +    };
> +    if (my $err = $@) {
> +        chomp($err);
> +        raise_param_exc({ bios => $err });
> +    }
> +};
> +
>  my $check_drive_param = sub {
>      my ($param, $storecfg, $extra_checks) = @_;
>  
> @@ -277,6 +292,13 @@ my $check_storage_access = sub {
>          "/storage/$settings->{vmstatestorage}",
>          ['Datastore.AllocateSpace'],
>      ) if defined($settings->{vmstatestorage});
> +
> +    if (defined($settings->{bios})) {
> +        my $firmware = PVE::QemuServer::BIOS::parse_bios($settings->{bios})->{firmware};
> +        PVE::Storage::check_volume_access(
> +            $rpcenv, $authuser, $storecfg, $vmid, $firmware, 'efi-firmware',
> +        ) if defined($firmware);

Style nit: multiline post-if should be avoided.

> +    }
>  };
>  
>  my $check_storage_access_clone = sub {
> @@ -319,6 +341,13 @@ my $check_storage_access_clone = sub {
>      $rpcenv->check($authuser, "/storage/$conf->{vmstatestorage}", ['Datastore.AllocateSpace'])
>          if defined($conf->{vmstatestorage});
>  
> +    # like ISO images, a custom firmware image is simply referenced by the clone
> +    if (my $firmware = PVE::QemuServer::BIOS::get_bios_firmware($conf)) {
> +        my ($sid) = PVE::Storage::parse_volume_id($firmware);
> +        my $scfg = PVE::Storage::storage_config($storecfg, $sid);
> +        $sharedvm = 0 if !$scfg->{shared};
> +    }
> +
>      return $sharedvm;
>  };
>  
> @@ -1365,6 +1394,7 @@ __PACKAGE__->register_method({
>              &$check_cpu_model_access($rpcenv, $authuser, $param);
>  
>              $check_drive_param->($param, $storecfg);
> +            $check_bios_param->($param);
>  
>              PVE::QemuServer::Network::add_random_macs($param);
>          }
> @@ -2203,6 +2233,8 @@ my $update_vm_api = sub {
>          $rpcenv, $authuser, $storecfg, $vmid, $param, undef, $extraction_storage,
>      );
>  
> +    $check_bios_param->($param);
> +
>      PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
>  
>      my $load_and_check_config = sub {
> @@ -5348,6 +5380,11 @@ __PACKAGE__->register_method({
>                              type => 'boolean',
>                              description => 'True if the disk is a cdrom.',
>                          },
> +                        is_firmware => {
> +                            type => 'boolean',
> +                            description => 'True if the volume is a custom firmware image.',
> +                            optional => 1,
> +                        },
>                          is_unused => {
>                              type => 'boolean',
>                              description => 'True if the disk is unused.',