[PATCH manager] fix #6735: api: pci: extend mdevscan persmissions for mappings

[PATCH manager] fix #6735: api: pci: extend mdevscan persmissions for mappings

[Elias Huhsovitz]

The mdevscan endpoint currently requires Sys.Audit or Sys.Modify on the
root path (/). This prevents non-admin users from querying available
mediated device types for a PCI mapping, even if they have explicit
Mapping.Use permissions on that specific mapping.

Update the permission check to allow access if the user holds
Mapping.Use, Mapping.Modify, or Mapping.Audit on the respective mapping
path (i.e., /mapping/pci/{mapping}). This aligns the `GET
/nodes/{node}/hardware/pci/{pci-id-or-mapping}/mdev` endpoint with the
`GET /cluster/mapping/pci/{id}` endpoint.

Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
 PVE/API2/Hardware/PCI.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Hardware/PCI.pm b/PVE/API2/Hardware/PCI.pm
index 36b9741b..de62bcd8 100644
--- a/PVE/API2/Hardware/PCI.pm
+++ b/PVE/API2/Hardware/PCI.pm
@@ -180,7 +180,18 @@ __PACKAGE__->register_method({
     protected => 1,
     proxyto => "node",
     permissions => {
-        check => ['perm', '/', ['Sys.Audit', 'Sys.Modify'], any => 1],
+        description => "Requires 'Sys.Audit' or 'Sys.Modify' on '/' for PCI IDs,"
+            . " or mapping permissions on the respective mapping.",
+        check => [
+            'or',
+            ['perm', '/', ['Sys.Audit', 'Sys.Modify'], any => 1],
+            [
+                'perm',
+                '/mapping/pci/{pci-id-or-mapping}',
+                ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
+                any => 1,
+            ],
+        ],
     },
     parameters => {
         additionalProperties => 0,
-- 
2.47.3

Re: [PATCH manager] fix #6735: api: pci: extend mdevscan persmissions for mappings

[Dominik Csapak]

hi,

while testing this, i saw the following errors in the syslog:

pveproxy[888583]: Failed to normalize '/mapping/pci/0000:08:00.0'
pveproxy[888583]: internal error: ACL check called for undefined ACL path!

i guess that something trips up the acl path normalize code (the '.' or 
the ':' probably?)

also checking the id for the mapping path is not really correct.
so i think the easiest way here would be to allow the api call
for all users, and explicitely check the permissions in the api
handler itself, after deciding if it's a mapping or a pci id.

you can use 'raise_perm_exc' then to give the user the correct
error message too.

(we do this often when the permissions depend on the actual parameter)

On 8/24/26 1:25 PM, Elias Huhsovitz wrote:
> The mdevscan endpoint currently requires Sys.Audit or Sys.Modify on the
> root path (/). This prevents non-admin users from querying available
> mediated device types for a PCI mapping, even if they have explicit
> Mapping.Use permissions on that specific mapping.
> 
> Update the permission check to allow access if the user holds
> Mapping.Use, Mapping.Modify, or Mapping.Audit on the respective mapping
> path (i.e., /mapping/pci/{mapping}). This aligns the `GET
> /nodes/{node}/hardware/pci/{pci-id-or-mapping}/mdev` endpoint with the
> `GET /cluster/mapping/pci/{id}` endpoint.
> 
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
>   PVE/API2/Hardware/PCI.pm | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Hardware/PCI.pm b/PVE/API2/Hardware/PCI.pm
> index 36b9741b..de62bcd8 100644
> --- a/PVE/API2/Hardware/PCI.pm
> +++ b/PVE/API2/Hardware/PCI.pm
> @@ -180,7 +180,18 @@ __PACKAGE__->register_method({
>       protected => 1,
>       proxyto => "node",
>       permissions => {
> -        check => ['perm', '/', ['Sys.Audit', 'Sys.Modify'], any => 1],
> +        description => "Requires 'Sys.Audit' or 'Sys.Modify' on '/' for PCI IDs,"
> +            . " or mapping permissions on the respective mapping.",
> +        check => [
> +            'or',
> +            ['perm', '/', ['Sys.Audit', 'Sys.Modify'], any => 1],
> +            [
> +                'perm',
> +                '/mapping/pci/{pci-id-or-mapping}',
> +                ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
> +                any => 1,
> +            ],
> +        ],
>       },
>       parameters => {
>           additionalProperties => 0,

[PATCH access-control] rpc: do not check malformed acl path

[Elias Huhsovitz]

Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
Thank you for the feedback. I looked into this a bit more and
provided an experimental patch as a reference.

The Problem
-----------
The core problem is indeed the colon ':', which makes sense.

I did some further testing and the same error occurs when sending a
request to the enpoint:

GET /cluster/mapping/pci/{id}

It outputs the errors:
pveproxy[42711]: Failed to normalize '/mapping/pci/0000:01:00.0'
pveproxy[42711]: internal error: ACL check called for undefined ACL path!
pveproxy[42711]: Use of uninitialized value $path in concatenation (.) or string at /usr/share/perl5/PVE/RPCEnvironment.pm line 325.

Mismatch
---------
I think explicitely checking the permissions in the api handler itself
will work, but to my understanding there is an underlying mismatch:

The API schema explicitly allows colons (for raw PCI IDs), but the 
ACL path normalizer explicitly rejects them.

Proposed solution
-----------------
IMHO providing explicit permissions on a raw PCI ID like 0000:01:00.0
instead of a mapping should not happen (i think) and should not be
checked. 

What is your opinion on simply not checking invalid ACL 
paths?

This would cause a more graceful failure. 

Caveats
-------
(I fear that such a check might causes problems in existing systems, 
that rely on "invalid" ACL paths. But i would love to hear your input
on this)

Side note
---------
What aboud allowing encoded PCI IDs instead? e.g.,
GET /nodes/lisa/hardware/pci/0000%3A01%3A00.0

Summary of changes
------------------

 src/PVE/RPCEnvironment.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index 7591aa9..61ef28b 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -487,7 +487,11 @@ sub exec_api2_perm_check {
         }
         my $path = PVE::Tools::template_replace($tmplpath, $param);
         my $normpath = PVE::AccessControl::normalize_path($path);
-        warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
+
+        if (!defined($normpath)) {
+            warn "Failed to normalize ACL path '$path'\n" if defined($path);
+            return 0;
+        }
 
         return $self->check_full($username, $normpath, $privs, $any, $noerr);
     } elsif ($test eq 'userid-group') {
-- 
2.47.3

Re: [PATCH access-control] rpc: do not check malformed acl path

[Fiona Ebner]

Missing rationale for why this is needed and why this is okay in the
commit message. Always good to have, but especially for changes to
permission checks.

Am 26.08.26 um 12:37 PM schrieb Elias Huhsovitz:
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> Thank you for the feedback. I looked into this a bit more and
> provided an experimental patch as a reference.
> 
> The Problem
> -----------
> The core problem is indeed the colon ':', which makes sense.
> 
> I did some further testing and the same error occurs when sending a
> request to the enpoint:
> 
> GET /cluster/mapping/pci/{id}
> 
> It outputs the errors:
> pveproxy[42711]: Failed to normalize '/mapping/pci/0000:01:00.0'
> pveproxy[42711]: internal error: ACL check called for undefined ACL path!
> pveproxy[42711]: Use of uninitialized value $path in concatenation (.) or string at /usr/share/perl5/PVE/RPCEnvironment.pm line 325.
> 
> Mismatch
> ---------
> I think explicitely checking the permissions in the api handler itself
> will work, but to my understanding there is an underlying mismatch:
> 
> The API schema explicitly allows colons (for raw PCI IDs), but the 
> ACL path normalizer explicitly rejects them.
> 
> Proposed solution
> -----------------
> IMHO providing explicit permissions on a raw PCI ID like 0000:01:00.0
> instead of a mapping should not happen (i think) and should not be
> checked. 
> 
> What is your opinion on simply not checking invalid ACL 
> paths?
> 
> This would cause a more graceful failure. 

Currently, the permission check for an invalid ACL path will still work
for root@pam. With your patch, it won't work anymore. See the beginning
of the permissions() sub.

Note also the history here:

> commit 37d3c16b25644f953647b512d9d231c866fa94e0
> Author: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Date:   Mon Jun 20 13:05:12 2022 +0200
> 
>     perm check: forbid undefined/empty ACL path
>     
>     to detect similar issues to that fixed in the previous commit early on
>     and without the potential for dangerous side-effects.
>     
>     root@pam is intentionally still allowed before the check in case such
>     situations can be triggered by misconfiguration - root@pam can then
>     still clean up the affected configs via the GUI/API, and not just via
>     manual editing.
>     
>     Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

So this is done intentionally.

What you can do is add a code comment, so people don't have to wonder
about it again in the future, as well as adapt the exception message in
case the path is undefined to avoid the ugly Perl warning.

I still finished my review below for completeness ;)

> 
> Caveats
> -------
> (I fear that such a check might causes problems in existing systems, 
> that rely on "invalid" ACL paths. But i would love to hear your input
> on this)
> 
> Side note
> ---------
> What aboud allowing encoded PCI IDs instead? e.g.,
> GET /nodes/lisa/hardware/pci/0000%3A01%3A00.0
> 
> Summary of changes
> ------------------
> 
>  src/PVE/RPCEnvironment.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 7591aa9..61ef28b 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -487,7 +487,11 @@ sub exec_api2_perm_check {
>          }
>          my $path = PVE::Tools::template_replace($tmplpath, $param);
>          my $normpath = PVE::AccessControl::normalize_path($path);
> -        warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
> +
> +        if (!defined($normpath)) {
> +            warn "Failed to normalize ACL path '$path'\n" if defined($path);
> +            return 0;

This should rather be:
return 0 if $noerr;
raise_perm_exc();

And I think we may assume that $path is defined and drop that post-if.
Because we have

die "missing parameters" if !($tmplpath && $privs);

so $tmplpath is set and then we call
my $path = PVE::Tools::template_replace($tmplpath, $param);
which should always produce a defined result if $tmplpath is set.

Even if that changes in the future for some reason, I'd rather get a
warning with an undefined value than no warning here, so the issue will
get noticed.

> +        }
>  
>          return $self->check_full($username, $normpath, $privs, $any, $noerr);
>      } elsif ($test eq 'userid-group') {

Re: [PATCH access-control] rpc: do not check malformed acl path

[Elias Huhsovitz]

Thanks for the quick response!

This was intended as a repsonse to this review:
https://lore.proxmox.com/pve-devel/8eff67e8-ac88-4b7d-a90d-de32644d1d95@proxmox.com/

I didn't mean for this to be a fully finished patch. This was meant
as a follow up.

I am unsure how to reply to a review while sharing code at
the same time. I relied on `--in-reply-to=` paramter too much.

Sorry for the trouble!

But your input resolves my confusion, so thanks a lot! :)


On Wed Aug 26, 2026 at 2:37 PM CEST, Fiona Ebner wrote:
> Missing rationale for why this is needed and why this is okay in the
> commit message. Always good to have, but especially for changes to
> permission checks.

[...]

>> Proposed solution
>> -----------------
>> IMHO providing explicit permissions on a raw PCI ID like 0000:01:00.0
>> instead of a mapping should not happen (i think) and should not be
>> checked. 
>> 
>> What is your opinion on simply not checking invalid ACL 
>> paths?
>> 
>> This would cause a more graceful failure. 
>
> Currently, the permission check for an invalid ACL path will still work
> for root@pam. With your patch, it won't work anymore. See the beginning
> of the permissions() sub.
>
> Note also the history here:
>
>> commit 37d3c16b25644f953647b512d9d231c866fa94e0
>> Author: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Date:   Mon Jun 20 13:05:12 2022 +0200
>> 
>>     perm check: forbid undefined/empty ACL path
>>     
>>     to detect similar issues to that fixed in the previous commit early on
>>     and without the potential for dangerous side-effects.
>>     
>>     root@pam is intentionally still allowed before the check in case such
>>     situations can be triggered by misconfiguration - root@pam can then
>>     still clean up the affected configs via the GUI/API, and not just via
>>     manual editing.
>>     
>>     Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>
> So this is done intentionally.
>
> What you can do is add a code comment, so people don't have to wonder
> about it again in the future, as well as adapt the exception message in
> case the path is undefined to avoid the ugly Perl warning.
>
> I still finished my review below for completeness ;)
>

[...]

>> 
>> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
>> index 7591aa9..61ef28b 100644
>> --- a/src/PVE/RPCEnvironment.pm
>> +++ b/src/PVE/RPCEnvironment.pm
>> @@ -487,7 +487,11 @@ sub exec_api2_perm_check {
>>          }
>>          my $path = PVE::Tools::template_replace($tmplpath, $param);
>>          my $normpath = PVE::AccessControl::normalize_path($path);
>> -        warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
>> +
>> +        if (!defined($normpath)) {
>> +            warn "Failed to normalize ACL path '$path'\n" if defined($path);
>> +            return 0;
>
> This should rather be:
> return 0 if $noerr;
> raise_perm_exc();
>
> And I think we may assume that $path is defined and drop that post-if.
> Because we have
>
> die "missing parameters" if !($tmplpath && $privs);
>
> so $tmplpath is set and then we call
> my $path = PVE::Tools::template_replace($tmplpath, $param);
> which should always produce a defined result if $tmplpath is set.
>
> Even if that changes in the future for some reason, I'd rather get a
> warning with an undefined value than no warning here, so the issue will
> get noticed.
>
>> +        }
>>  
>>          return $self->check_full($username, $normpath, $privs, $any, $noerr);
>>      } elsif ($test eq 'userid-group') {

Re: [PATCH access-control] rpc: do not check malformed acl path

[Fiona Ebner]

Am 26.08.26 um 2:53 PM schrieb Elias Huhsovitz:
> Thanks for the quick response!
> 
> This was intended as a repsonse to this review:
> https://lore.proxmox.com/pve-devel/8eff67e8-ac88-4b7d-a90d-de32644d1d95@proxmox.com/
> 
> I didn't mean for this to be a fully finished patch. This was meant
> as a follow up.

Yes, I saw. But ah okay, that's why there was no full commit message,
got it!

> I am unsure how to reply to a review while sharing code at
> the same time. I relied on `--in-reply-to=` paramter too much.
> 
> Sorry for the trouble!

No worries, this is fine :)

> But your input resolves my confusion, so thanks a lot! :)

Re: [PATCH access-control] rpc: do not check malformed acl path

[Dominik Csapak]

high level: I would not touch the acl checks here in anyway, but
as I already wrote would like to do it like we do elsewhere:

for example: in the 'startall' api call, we have:

permissions => { user => 'all' },

so every api user can call it, but then inside the api handler
we check the permission manually and do a `raise_perm_exc`


so in our case here, we first check if the parameter is a
pciid or a mapping, and check the permission accordingly.

then we don't need to adapt the ACL part, and have still
proper acl validation.

does that make sense?

On 8/26/26 12:37 PM, Elias Huhsovitz wrote:
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> Thank you for the feedback. I looked into this a bit more and
> provided an experimental patch as a reference.
> 
> The Problem
> -----------
> The core problem is indeed the colon ':', which makes sense.
> 
> I did some further testing and the same error occurs when sending a
> request to the enpoint:
> 
> GET /cluster/mapping/pci/{id}
> 
> It outputs the errors:
> pveproxy[42711]: Failed to normalize '/mapping/pci/0000:01:00.0'
> pveproxy[42711]: internal error: ACL check called for undefined ACL path!
> pveproxy[42711]: Use of uninitialized value $path in concatenation (.) or string at /usr/share/perl5/PVE/RPCEnvironment.pm line 325.
> 
> Mismatch
> ---------
> I think explicitely checking the permissions in the api handler itself
> will work, but to my understanding there is an underlying mismatch:
> 
> The API schema explicitly allows colons (for raw PCI IDs), but the
> ACL path normalizer explicitly rejects them.
> 
> Proposed solution
> -----------------
> IMHO providing explicit permissions on a raw PCI ID like 0000:01:00.0
> instead of a mapping should not happen (i think) and should not be
> checked.
> 
> What is your opinion on simply not checking invalid ACL
> paths?
> 
> This would cause a more graceful failure.
> 
> Caveats
> -------
> (I fear that such a check might causes problems in existing systems,
> that rely on "invalid" ACL paths. But i would love to hear your input
> on this)
> 
> Side note
> ---------
> What aboud allowing encoded PCI IDs instead? e.g.,
> GET /nodes/lisa/hardware/pci/0000%3A01%3A00.0
> 
> Summary of changes
> ------------------
> 
>   src/PVE/RPCEnvironment.pm | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 7591aa9..61ef28b 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -487,7 +487,11 @@ sub exec_api2_perm_check {
>           }
>           my $path = PVE::Tools::template_replace($tmplpath, $param);
>           my $normpath = PVE::AccessControl::normalize_path($path);
> -        warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
> +
> +        if (!defined($normpath)) {
> +            warn "Failed to normalize ACL path '$path'\n" if defined($path);
> +            return 0;
> +        }
>   
>           return $self->check_full($username, $normpath, $privs, $any, $noerr);
>       } elsif ($test eq 'userid-group') {

superseded: [PATCH manager] fix #6735: api: pci: extend mdevscan persmissions for mappings

[Elias Huhsovitz]

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