Re: [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions

[Dominik Csapak <d.csapak@proxmox.com>]

one comment, aside from that

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 9/3/26 2:11 PM, Elias Huhsovitz wrote:
> The mdevscan endpoint requires Sys.Audit or Sys.Modify on '/'. This
> blocks non-admin users from listing mediated device types for a PCI
> mapping, even when they hold Mapping.Use on that mapping.
> 
> Check the permission in the API handler based on the parameter type: For
> a raw PCI ID, require Sys.Audit or Sys.Modify on '/'. For a mapping,
> require Sys.Audit or Sys.Modify on '/', or fall back to requiring
> Mapping.Use, Mapping.Modify or Mapping.Audit on the specific mapping
> path.
> 
> Set the endpoint permission to 'user => all' so the handler performs the
> type-dependent check. This keeps raw PCI IDs, which are not valid ACL
> paths, out of the declarative ACL evaluation.
> 
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> v2: https://lore.proxmox.com/pve-devel/20260827112525.154445-1-e.huhsovitz@proxmox.com/
> v1: https://lore.proxmox.com/pve-devel/20260824112610.148089-1-e.huhsovitz@proxmox.com/
> 
> Changes v2->v3
> --------------
> * re-introduce else-statement in API handler
> * move permissions checks into respective logical branches
>    (previously sepate sesection before core code)
> * update commit message
> 
> Changes v1->v2
> --------------
> * Allow all users in the declarative API permissions.
> * Implement ACL check in API handler by calling
>    check_any and raise_perm_exc.
> * Remove else statement in API handler, since return statement
>    provides implicit branching
> * update commit message
> 
> Changes
> -------
>   PVE/API2/Hardware/PCI.pm | 44 ++++++++++++++++++++++++++++++----------
>   1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/PVE/API2/Hardware/PCI.pm b/PVE/API2/Hardware/PCI.pm
> index 36b9741b..bfcd1fc5 100644
> --- a/PVE/API2/Hardware/PCI.pm
> +++ b/PVE/API2/Hardware/PCI.pm
> @@ -3,10 +3,12 @@ package PVE::API2::Hardware::PCI;
>   use strict;
>   use warnings;
>   
> +use PVE::Exception qw(raise raise_perm_exc);
>   use PVE::JSONSchema qw(get_standard_option);
>   
>   use PVE::QemuServer::PCI::Mdev;
>   use PVE::RESTHandler;
> +use PVE::RPCEnvironment;
>   
>   use base qw(PVE::RESTHandler);
>   
> @@ -180,7 +182,11 @@ __PACKAGE__->register_method({
>       protected => 1,
>       proxyto => "node",
>       permissions => {
> -        check => ['perm', '/', ['Sys.Audit', 'Sys.Modify'], any => 1],
> +        description =>
> +            "For a PCI ID, requires 'Sys.Audit' or 'Sys.Modify' on '/'. For a mapping,"
> +            . " requires the same global permissions, or 'Mapping.Use', 'Mapping.Modify'"
> +            . ", or 'Mapping.Audit' on '/mapping/pci/<id>'.",
> +        user => 'all',
>       },
>       parameters => {
>           additionalProperties => 0,
> @@ -222,20 +228,37 @@ __PACKAGE__->register_method({
>       code => sub {
>           my ($param) = @_;
>   
> -        if ($param->{'pci-id-or-mapping'} =~
> -            m/^(?:[0-9a-fA-F]{4}:)?[0-9a-fA-F]{2}:[0-9a-fA-F]{2}\.[0-9a-fA-F]$/
> -        ) {
> -            return PVE::QemuServer::PCI::Mdev::get_mdev_types($param->{'pci-id-or-mapping'}); # PCI ID
> +        my $id = $param->{'pci-id-or-mapping'};
> +        my $is_pci_id =
> +            $id =~ m/^(?:[0-9a-fA-F]{4}:)?[0-9a-fA-F]{2}:[0-9a-fA-F]{2}\.[0-9a-fA-F]$/;
> +
> +        my $rpcenv = PVE::RPCEnvironment::get();
> +        my $authuser = $rpcenv->get_user();
> +
> +        my $has_global_perms =
> +            $rpcenv->check_any($authuser, '/', ['Sys.Audit', 'Sys.Modify'], 1);
> +
> +        if ($is_pci_id) {
> +            raise_perm_exc("/, " . join("|", ['Sys.Audit', 'Sys.Modify']))
> +                if !$has_global_perms;
> +


I guess this list needs to be a proper list (not a reference) otherwise
this comes out as:

ARRAY(0x64e525f764e8)

(check with the perl cli: `perl -e "print join('|', ['foo', 'bar']);"` )

IMO 3 good ways to solve:

* use  ('Sys.Audit', 'Sys.Modify')
* factor that list out and use $var->@*
* simply write out the string (this way is even longer than
   manually writing "/, Sys.Audit|Sys.Modify"

> +            return PVE::QemuServer::PCI::Mdev::get_mdev_types($id);
>           } else {
> -            my $mapping = $param->{'pci-id-or-mapping'};
> +            if (!$has_global_perms) {
> +                $rpcenv->check_any(
> +                    $authuser,
> +                    "/mapping/pci/$id",
> +                    ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
> +                );
> +            }
>   
>               my $types = {};
> -            my $devices = PVE::Mapping::PCI::find_on_current_node($mapping);
> +            my $devices = PVE::Mapping::PCI::find_on_current_node($id);
>               for my $device ($devices->@*) {
> -                my $id = $device->{path};
> -                next if $id =~ m/;/; # mdev not supported for multifunction devices
> +                my $dev_id = $device->{path};
> +                next if $dev_id =~ m/;/; # mdev not supported for multifunction devices
>   
> -                my $device_types = PVE::QemuServer::PCI::Mdev::get_mdev_types($id);
> +                my $device_types = PVE::QemuServer::PCI::Mdev::get_mdev_types($dev_id);
>   
>                   for my $type_definition ($device_types->@*) {
>                       my $type = $type_definition->{type};
> @@ -247,6 +270,5 @@ __PACKAGE__->register_method({
>   
>               return [sort { $a->{type} cmp $b->{type} } values($types->%*)];
>           }
> -
>       },
>   });