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

[Dominik Csapak]

On 8/1/22 15:01, Fabian Grünbichler wrote:
> On July 19, 2022 1:46 pm, Dominik Csapak wrote:
>> Signed-off-by: Dominik Csapak <d.csapak at 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?
> 

ok, either i did send from the wrong branch, or i redid that already since sending
the patches. my branch here locally already has all of the wrong 'parse_usb_device'
calls replaced with 'parse_property_string' (like with pci)

so in any case that is the correct approach here.
first parse the property string, then parse the usb device from the 'host' property