From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id CCD251FF0AE for ; Tue, 01 Sep 2026 14:27:40 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A6586214DC; Tue, 01 Sep 2026 14:27:39 +0200 (CEST) Message-ID: Date: Tue, 1 Sep 2026 14:27:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions. To: Elias Huhsovitz , pve-devel@lists.proxmox.com References: <20260827112525.154445-1-e.huhsovitz@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260827112525.154445-1-e.huhsovitz@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788265652025 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.580 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: CPIBDN2JUJLZK6GNV7SOKTG2Z7J3I5ZO X-Message-ID-Hash: CPIBDN2JUJLZK6GNV7SOKTG2Z7J3I5ZO X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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/'.", > + 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->%*)]; > }, > });