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

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

[Elias Huhsovitz]

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;
+
+            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->%*)];
         }
-
     },
 });
-- 
2.47.3

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

[Dominik Csapak]

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->%*)];
>           }
> -
>       },
>   });

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

[Elias Huhsovitz]

Superseded-By: https://lore.proxmox.com/pve-devel/20260904094456.70309-1-e.huhsovitz@proxmox.com/