[pve-devel] [PATCH v4 qemu-server 08/12] Verify VM-specific CPU configs seperately

[Fabian Grünbichler]

On October 7, 2019 2:47 pm, Stefan Reiter wrote:
> $cpu_fmt is being reused for custom CPUs as well as VM-specific CPU
> settings. The "pve-vm-cpu-conf" format is introduced to verify a config
> specifically for use as VM-specific settings.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> v3 -> v4:
> * use is_custom_model
> 
> v2 -> v3:
> * move $cpu_fmt->{flags} changes here, to avoid having broken checks between
>   patches
> 
> 
>  PVE/QemuServer.pm           |  2 +-
>  PVE/QemuServer/CPUConfig.pm | 70 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 67 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 58e2944..04ab1c2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -505,7 +505,7 @@ EODESCR
>  	optional => 1,
>  	description => "Emulated CPU type.",
>  	type => 'string',
> -	format => $PVE::QemuServer::CPUConfig::cpu_fmt,
> +	format => 'pve-vm-cpu-conf',
>      },
>      parent => get_standard_option('pve-snapshot-name', {
>  	optional => 1,
> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
> index 9876757..fb8e852 100644
> --- a/PVE/QemuServer/CPUConfig.pm
> +++ b/PVE/QemuServer/CPUConfig.pm
> @@ -90,9 +90,9 @@ my @supported_cpu_flags = (
>      'hv-evmcs',
>      'aes'
>  );
> -my $cpu_flag = qr/[+-](@{[join('|', @supported_cpu_flags)]})/;
> +my $cpu_flag_re = qr/([+-])(@{[join('|', @supported_cpu_flags)]})/;

maybe it makes sense to pull out both REs?

>  
> -our $cpu_fmt = {
> +my $cpu_fmt = {
>      cputype => {
>  	description => "Emulated CPU type. Can be default or custom name (custom model names must be prefixed with 'custom-').",
>  	type => 'string',
> @@ -125,14 +125,76 @@ our $cpu_fmt = {
>      flags => {
>  	description => "List of additional CPU flags separated by ';'."
>  		     . " Use '+FLAG' to enable, '-FLAG' to disable a flag."
> -		     . " Currently supported flags: @{[join(', ', @supported_cpu_flags)]}.",
> +		     . " Custom CPU models can specify any flag supported by"
> +		     . " QEMU/KVM, VM-specific flags must be from the following"
> +		     . " set for security reasons: @{[join(', ', @supported_cpu_flags)]}.",
>  	format_description => '+FLAG[;-FLAG...]',
>  	type => 'string',
> -	pattern => qr/$cpu_flag(;$cpu_flag)*/,
> +	pattern => qr/[+-][a-zA-Z0-9\-_\.]+(;[+-][a-zA-Z0-9\-_\.]+)*/,
>  	optional => 1,
>      },
>  };
>  
> +# $cpu_fmt describes both the CPU config passed as part of a VM config, as well
> +# as the definition of a custom CPU model. There are some slight differences
> +# though, which we catch in the custom verification function below.
> +sub parse_cpu_conf_basic {
> +    my ($cpu_str, $noerr) = @_;
> +
> +    my $cpu = eval { PVE::JSONSchema::parse_property_string($cpu_fmt, $cpu_str) };
> +    if ($@) {
> +        die $@ if !$noerr;
> +        return undef;
> +    }
> +
> +    # required, but can't be made "optional => 0" since it's not included as a
> +    # seperate property in config file

required, but only encoded in section header?

> +    if (!$cpu->{cputype}) {
> +	die "CPU is missing cputype\n" if !$noerr;
> +	return undef;
> +    }
> +
> +    return $cpu;
> +}
> +
> +PVE::JSONSchema::register_format('pve-vm-cpu-conf', \&verify_vm_cpu_conf);
> +sub verify_vm_cpu_conf {
> +    my ($cpu_str, $noerr) = @_;
> +
> +    my $cpu = parse_cpu_conf_basic($cpu_str, $noerr);
> +    return undef if !$cpu;
> +
> +    my $cputype = $cpu->{cputype};
> +
> +    if (is_custom_model($cputype)) {
> +	# Custom model only has to exist, all props are allowed
> +	my $config = load_custom_model_conf();
> +	my $cputype_id = $cputype;
> +	$cputype_id =~ s/^custom-//;
> +	return $cpu if $config && defined($config->{ids}->{$cputype_id});

this is very similar to get_model_by_name, see my other comments there 
and think about re-using it here..

> +
> +	die "Custom cputype '$cputype_id' not found\n" if !$noerr;
> +	return undef;
> +    }
> +
> +    # Only built-in models get here
> +    if (!defined($cpu_vendor_list->{$cputype})) {
> +	die "Default cputype '$cputype' not found\n" if !$noerr;

s/Default/Built-in/

or even

"(Built-in) cputype '$cputype' is not defined (missing 'custom-' prefix?)\n"

> +	return undef;
> +    }
> +
> +    if ($cpu->{flags} && $cpu->{flags} !~ m/$cpu_flag_re(;$cpu_flag_re)*/) {
> +	die "VM-specific CPU flags must be a subset of: @{[join(', ', @supported_cpu_flags)]}\n"
> +	    if !$noerr;
> +	return undef;
> +    }
> +
> +    die "Property 'reported-model' not allowed in VM-specific CPU config.\n"
> +	if defined($cpu->{'reported-model'});
> +
> +    return $cpu;
> +}
> +
>  # Section config settings
>  my $defaultData = {
>      # shallow copy, since SectionConfig modifies propertyList internally
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>