Re: [pve-devel] [PATCH qemu-server 3/7] PVE/API2/Qemu: add permission checks for mapped usb devices

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 19, 2022 1:46 pm, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 99b426e..aa7ddea 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -26,6 +26,7 @@ use PVE::QemuServer::Drive;
>  use PVE::QemuServer::ImportDisk;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer::Machine;
> +use PVE::QemuServer::USB qw(parse_usb_device);
>  use PVE::QemuMigrate;
>  use PVE::RPCEnvironment;
>  use PVE::AccessControl;
> @@ -567,8 +568,12 @@ my $check_vm_create_usb_perm = sub {
>  
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^usb\d+$/;
> +	my $device = parse_usb_device($param->{$opt});
>  
> -	if ($param->{$opt} =~ m/spice/) {
> +	if ($device->{spice}) {
> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> +	} elsif ($device->{mapped}) {
> +	    $rpcenv->check_hw_perm($authuser, $device->{host}, ['Hardware.Use']);

maybe I am overlooking something, but where does $device->{host} come 
from?

parse_usb_device (for a mapped USB device) looks up device in the 
hardware map, asserts it's valid (for the local node), and then either 
returns

{
  vendorid => $map->{vendor},
  productid => $map->{device},
  mapped => 1,
}

or the result of parse_usb_device($map->{path}), with 'mapped' set.

since the lookup in the map doesn't set a 'host' member, wouldn't 
$device->{host} always be undef for mapped devices? maybe this was 
wrongly copied from the PCI code, where the hostpci property string has 
a 'host' property (that with this series, also possibly contains a 
mapping entry ID)? or is this supposed to parse the property string, and 
use the host property from there?

>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
>  	} else {
>  	    die "only root can set '$opt' config for real devices\n";
> @@ -1552,7 +1557,12 @@ my $update_vm_api  = sub {
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} elsif ($opt =~ m/^usb\d+$/) {
> -		    if ($val =~ m/spice/) {
> +		    my $device = PVE::QemuServer::USB::parse_usb_device($val);
> +		    my $host = parse_usb_device($device->{host});
> +		    if ($host->{spice}) {
> +			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    } elsif ($host->{mapped}) {
> +			$rpcenv->check_hw_perm($authuser, $device->{host}, ['Hardware.Use']);

same question here..

>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
>  		    } elsif ($authuser ne 'root@pam') {
>  			die "only root can delete '$opt' config for real devices\n";
> @@ -1613,7 +1623,30 @@ my $update_vm_api  = sub {
>  		    }
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} elsif ($opt =~ m/^usb\d+/) {
> -		    if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) {
> +		    my $olddevice;
> +		    my $oldhost;
> +		    if (defined($conf->{$opt})) {
> +			$olddevice = PVE::QemuServer::USB::parse_usb_device($conf->{$opt});
> +			$oldhost = parse_usb_device($olddevice->{host});

and here

> +		    }
> +		    if (defined($oldhost)) {
> +			if ($oldhost->{spice}) {
> +			    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +			} elsif ($oldhost->{mapped}) {
> +			    $rpcenv->check_hw_perm($authuser, $olddevice->{host}, ['Hardware.Use']);

and here

> +			    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +			} elsif ($authuser ne 'root@pam') {
> +			    die "only root can modify '$opt' config for real devices\n";
> +			}
> +		    }
> +
> +		    my $newdevice = PVE::QemuServer::USB::parse_usb_device($param->{$opt});
> +		    my $newhost = parse_usb_device($newdevice->{host});

and here

> +
> +		    if ($newhost->{spice}) {
> +			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    } elsif ($newhost->{mapped}) {
> +			$rpcenv->check_hw_perm($authuser, $newdevice->{host}, ['Hardware.Use']);

and here

>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
>  		    } elsif ($authuser ne 'root@pam') {
>  			die "only root can modify '$opt' config for real devices\n";
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>