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

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

some comments inline

On 8/27/26 1:25 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.
> 
> Reduce indentation by removing redundant else statement. (return
> provides implicit branching)
> 
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> v1: https://lore.proxmox.com/pve-devel/20260824112610.148089-1-e.huhsovitz@proxmox.com/
> 
> 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
> 
> Side Note
> ---------
> Since this pattern of manual permission checks appear quite frequently,
> it might be of interest to provide generic way to deal with such cases
> inside of the permissions block (Similar to overriding the
> hasPermission method in Java Spring Boot)
> 
> For example like this:
> permissions => {
>          user => 'all',
>          custom => my_perm_sub(),
> },
> 
> my_perm_sub would simply return a boolean. This would be an optional
> extension, without requiring major changes in ACL Logic.
> 
> Let me know if this is something you would consider worthwhile.
> 
> Changes
> -------
>   PVE/API2/Hardware/PCI.pm | 71 +++++++++++++++++++++++++++-------------
>   1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/PVE/API2/Hardware/PCI.pm b/PVE/API2/Hardware/PCI.pm
> index 36b9741b..47c4b29b 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,31 +228,50 @@ __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
> -        } else {
> -            my $mapping = $param->{'pci-id-or-mapping'};
> -
> -            my $types = {};
> -            my $devices = PVE::Mapping::PCI::find_on_current_node($mapping);
> -            for my $device ($devices->@*) {
> -                my $id = $device->{path};
> -                next if $id =~ m/;/; # mdev not supported for multifunction devices
> -
> -                my $device_types = PVE::QemuServer::PCI::Mdev::get_mdev_types($id);
> -
> -                for my $type_definition ($device_types->@*) {
> -                    my $type = $type_definition->{type};
> -                    if (!defined($types->{$type})) {
> -                        $types->{$type} = $type_definition;
> -                    }
> +        my $id = $param->{'pci-id-or-mapping'};
> +        my $rpcenv = PVE::RPCEnvironment::get();
> +        my $authuser = $rpcenv->get_user();
> +
> +        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 $has_global_perms =
> +            $rpcenv->check_any($authuser, '/', ['Sys.Audit', 'Sys.Modify'], 1);
> +
> +        if (!$has_global_perms) {
> +            raise_perm_exc("/, " . join("|", ['Sys.Audit', 'Sys.Modify'])) if ($is_pci_id);
> +            $rpcenv->check_any(
> +                $authuser,
> +                "/mapping/pci/$id",
> +                ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
> +            );
> +        }
> +
> +        return PVE::QemuServer::PCI::Mdev::get_mdev_types($id) if ($is_pci_id);
> +
> +        if (!$rpcenv->check_any($authuser, '/', ['Sys.Audit', 'Sys.Modify'], 1)) {

isn't this just $has_global_perms again?

> +            $rpcenv->check_any(
> +                $authuser,
> +                "/mapping/pci/$id",
> +                ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
> +            );

this was also already checked?


IMO these happen because the code flow is not very obvious. Instead
of having a bunch of postif clauses, I'd prefer a simpler approach:


my $id = ...
my $rpcenv = ...
my $authuser = ...

my $has_global_perms = ...
my $is_pci_id = ...


if ($is_pci_id)  {
    raise_perm_exc if !$has_global_perms;
    ...
} else {
    raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
    ...
}

with such code, the existing flow and indentation stays the same,
and it's clear which branch takes which permissions

does that make sense?


> +        }
> +
> +        my $types = {};
> +        my $devices = PVE::Mapping::PCI::find_on_current_node($id);
> +        for my $device ($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($dev_id);
> +
> +            for my $type_definition ($device_types->@*) {
> +                my $type = $type_definition->{type};
> +                if (!defined($types->{$type})) {
> +                    $types->{$type} = $type_definition;
>                   }
>               }
> -
> -            return [sort { $a->{type} cmp $b->{type} } values($types->%*)];
>           }
>   
> +        return [sort { $a->{type} cmp $b->{type} } values($types->%*)];
>       },
>   });