Re: [pve-devel] [PATCH qemu-server 1/2] fix #4068: implement support for fw_cfg

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Mar 01, 2023 at 10:12:26AM +0100, Leo Nunner wrote:
> Implements support for passing values to the fw_cfg argument for QEMU.
> If the value looks like a file, the backend checks for the correct
> permissions/path and if everything is right, includes it as a file
> instead of as a string.
> 
> Setting the argument requires the VM.Config.Options permission on the
> guest. Including files requires Datastore.Audit and Datastore.Allocate
> on the specific storage.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> RFC: I feel like a more implicit option for passing files would be
> nicer, but I can't really think of a nice way…
> 
>  PVE/API2/Qemu.pm  | 14 ++++++++++++++
>  PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..c03394f 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -639,6 +639,8 @@ my $check_vm_modify_config_perm = sub {
>  	    # the user needs Disk and PowerMgmt privileges to change the vmstate
>  	    # also needs privileges on the storage, that will be checked later
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', 'VM.PowerMgmt' ]);
> +	} elsif ($opt eq 'fw_cfg') {
> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
>  	} else {
>  	    # catches hostpci\d+, args, lock, etc.
>  	    # new options will be checked here
> @@ -1770,6 +1772,18 @@ my $update_vm_api  = sub {
>  		} elsif ($opt eq 'tags') {
>  		    assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
>  		    $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt});
> +		} elsif ($opt eq 'fw_cfg') {
> +		    foreach my $fw_cfg (PVE::Tools::split_list($param->{$opt})) {
> +			my ($opt, $val) = split("=", $fw_cfg);
> +
> +			if (my $storage = PVE::Storage::parse_volume_id($val, 1)) {
> +			    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.Audit', 'Datastore.Allocate']);
> +
> +			    my ($path, undef, $type) = PVE::Storage::path($storecfg, $val);
> +			    die "File $val is not in snippets directory\n" if $type ne "snippets";
> +			}
> +		    }
> +		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 40be44d..c5dea1f 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -723,6 +723,12 @@ EODESCR
>  	description => "List of host cores used to execute guest processes, for example: 0,5,8-11",
>  	optional => 1,
>      },
> +    fw_cfg => {
> +	type => 'string',
> +	optional => 1,
> +	description => 'Pass values to the guest via the fw_cfg parameter.',
> +	format => 'pve-fw-cfg-list',
> +    },
>  };
>  
>  my $cicustom_fmt = {
> @@ -1076,6 +1082,21 @@ sub verify_volume_id_or_absolute_path {
>      return $volid;
>  }
>  
> +PVE::JSONSchema::register_format('pve-fw-cfg', \&verify_fw_cfg);
> +sub verify_fw_cfg {
> +    my ($value, $noerr) = @_;
> +
> +    my $FW_CFG_REGEX = qr/[a-z0-9\.\/:\-]+/;

IMO this is way too restrictive.  If we pass the `name=` prefix in the
`-fw_cfg` parameter we can even allow `=`.  In fact, AFAICT only `,` is
problematic and would need to be doubled. Also if we really just have a
single option with a list... both `,` and `;` will be problematic on our
end, so it's fine to exclude those 2 for now. (Until we add quoting
support to split_list ;-) )

That said, for the use cases I have in mind for us, the current code
would be sufficient, but at least uppercase letters, underscores and
spaces would be nice...

> +
> +    if ($value =~ m/^(opt\/$FW_CFG_REGEX)=($FW_CFG_REGEX)$/) {
> +	return $value;
> +    }
> +
> +    return if $noerr;
> +
> +    die "unable to parse fw_cfg option\n";
> +}
> +
>  my $usb_fmt = {
>      host => {
>  	default_key => 1,
> @@ -4181,6 +4202,20 @@ sub config_to_command {
>  	push @$cmd, '-snapshot';
>      }
>  
> +    if ($conf->{fw_cfg}) {
> +	foreach my $conf (PVE::Tools::split_list($conf->{fw_cfg})) {
> +	    my ($opt, $val) = split("=", $conf);
> +
> +	    push @$cmd, "-fw_cfg";
> +	    if (PVE::Storage::parse_volume_id($val, 1)) {
> +		my $path = PVE::Storage::path($storecfg, $val);
> +		push @$cmd, "$opt,file=$path";
> +	    } else {
> +		push @$cmd, "$opt,string=$val";

^ This would definitely be safer with the `name=` portion included.