* [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions
@ 2026-09-03 12:11 Elias Huhsovitz
2026-09-04 7:17 ` Dominik Csapak
2026-09-04 9:49 ` superseded: " Elias Huhsovitz
0 siblings, 2 replies; 3+ messages in thread
From: Elias Huhsovitz @ 2026-09-03 12:11 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions
2026-09-03 12:11 [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions Elias Huhsovitz
@ 2026-09-04 7:17 ` Dominik Csapak
2026-09-04 9:49 ` superseded: " Elias Huhsovitz
1 sibling, 0 replies; 3+ messages in thread
From: Dominik Csapak @ 2026-09-04 7:17 UTC (permalink / raw)
To: Elias Huhsovitz, pve-devel
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->%*)];
> }
> -
> },
> });
^ permalink raw reply [flat|nested] 3+ messages in thread
* superseded: [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions
2026-09-03 12:11 [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions Elias Huhsovitz
2026-09-04 7:17 ` Dominik Csapak
@ 2026-09-04 9:49 ` Elias Huhsovitz
1 sibling, 0 replies; 3+ messages in thread
From: Elias Huhsovitz @ 2026-09-04 9:49 UTC (permalink / raw)
To: Elias Huhsovitz, pve-devel
Superseded-By: https://lore.proxmox.com/pve-devel/20260904094456.70309-1-e.huhsovitz@proxmox.com/
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-04 9:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-03 12:11 [PATCH manager v3] fix #6735: api: pci: allow mdevscan access via mapping permissions Elias Huhsovitz
2026-09-04 7:17 ` Dominik Csapak
2026-09-04 9:49 ` superseded: " Elias Huhsovitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.