* [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions.
@ 2026-08-27 11:25 Elias Huhsovitz
2026-09-01 12:27 ` Dominik Csapak
0 siblings, 1 reply; 4+ messages in thread
From: Elias Huhsovitz @ 2026-08-27 11:25 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.
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)) {
+ $rpcenv->check_any(
+ $authuser,
+ "/mapping/pci/$id",
+ ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
+ );
+ }
+
+ 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->%*)];
},
});
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions.
2026-08-27 11:25 [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions Elias Huhsovitz
@ 2026-09-01 12:27 ` Dominik Csapak
2026-09-02 8:51 ` Elias Huhsovitz
0 siblings, 1 reply; 4+ messages in thread
From: Dominik Csapak @ 2026-09-01 12:27 UTC (permalink / raw)
To: Elias Huhsovitz, pve-devel
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->%*)];
> },
> });
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions.
2026-09-01 12:27 ` Dominik Csapak
@ 2026-09-02 8:51 ` Elias Huhsovitz
2026-09-02 10:56 ` Dominik Csapak
0 siblings, 1 reply; 4+ messages in thread
From: Elias Huhsovitz @ 2026-09-02 8:51 UTC (permalink / raw)
To: Dominik Csapak, pve-devel
Thanks for the feedback! Just one small stylistic question below.
On Tue Sep 1, 2026 at 2:27 PM CEST, Dominik Csapak wrote:
> some comments inline
>
[...]
>> +
>> + 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?
Sorry, these shouldn't have been here, i formatted the wrong branch by
mistake. The check is indeeed redundant.
>> + $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?
I just have a small question here:
Do you propse A or B:
A:
if ($is_pci_id) {
raise_perm_exc if !$has_global_perms;
# business logic ....
} else {
raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
# business logic ...
}
# END OF SUBROUTINE
B:
if ($is_pci_id) {
raise_perm_exc if !$has_global_perms;
} else {
raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
}
# business logic ...
# END OF SUBROUTINE
Becuase i prefer B. It seems much simpler to me since the permission logic and
business logic aren't intertwined. I would even like to go as far as
factoring the logic into its own subroutine, since the business logic
should be completely separate from the authorization.
C:
check_custom_perm($is_pci_id);
# business logic ...
# END OF SUBROUTINE
This also doesnt change the business logic, and i can keep the if-else
statement if you prefer.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions.
2026-09-02 8:51 ` Elias Huhsovitz
@ 2026-09-02 10:56 ` Dominik Csapak
0 siblings, 0 replies; 4+ messages in thread
From: Dominik Csapak @ 2026-09-02 10:56 UTC (permalink / raw)
To: Elias Huhsovitz, pve-devel
On 9/2/26 10:51 AM, Elias Huhsovitz wrote:
> Thanks for the feedback! Just one small stylistic question below.
>
> On Tue Sep 1, 2026 at 2:27 PM CEST, Dominik Csapak wrote:
>> some comments inline
>>
>
> [...]
>>> +
>>> + 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?
>
> Sorry, these shouldn't have been here, i formatted the wrong branch by
> mistake. The check is indeeed redundant.
>
>>> + $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?
>
>
> I just have a small question here:
>
> Do you propse A or B:
>
> A:
>
> if ($is_pci_id) {
> raise_perm_exc if !$has_global_perms;
> # business logic ....
> } else {
> raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
> # business logic ...
> }
>
> # END OF SUBROUTINE
>
> B:
>
> if ($is_pci_id) {
> raise_perm_exc if !$has_global_perms;
> } else {
> raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
> }
>
> # business logic ...
>
> # END OF SUBROUTINE
>
> Becuase i prefer B. It seems much simpler to me since the permission logic and
> business logic aren't intertwined. I would even like to go as far as
> factoring the logic into its own subroutine, since the business logic
> should be completely separate from the authorization.
>
> C:
>
> check_custom_perm($is_pci_id);
>
> # business logic ...
>
> # END OF SUBROUTINE
>
>
> This also doesnt change the business logic, and i can keep the if-else
> statement if you prefer.
Actually i would prefer A, because the two branches are by definition
very different (aside from one permission check), so having it split
up early makes sense to me.
With B and C, we at least have to duplicate one if/else (namely
on the 'is it a pci id or not' and that has a good chance
to diverge in the future (e.g. imagine we introduce a 3rd branch
that needs again different permissions, with A this is
more or less automatic since each branch needs it's own check.
with B and C two different parts of the code have to argue
about the same branching.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-02 10:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-27 11:25 [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions Elias Huhsovitz
2026-09-01 12:27 ` Dominik Csapak
2026-09-02 8:51 ` Elias Huhsovitz
2026-09-02 10:56 ` Dominik Csapak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox