[PATCH 19/23] xsm/dummy: Allow sysctls to both hardware and control

Jason Andryuk posted 23 patches 11 months, 1 week ago
There is a newer version of this series
[PATCH 19/23] xsm/dummy: Allow sysctls to both hardware and control
Posted by Jason Andryuk 11 months, 1 week ago
xl queries SYSCTL_physinfo for the physical cpus:
domU:~# xl list
libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
Name                    ID   Mem VCPUs        State   Time(s)
Domain-0                 0   800     1     r-----     130.0
dom0less-1               1   400     1     r-----     130.3
dom0less-2               2   800     1     r-----     130.3

Hardware and control are both privileged.  Allow them both access to
sysctls so they have insight into the running system.  This is coarse
grained permissions for the dummy policy.

Now the only sysctl denied to a control domain is readconsole.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
Could just allow physinfo to control as an alternative.  There could be
follow on sysctls needed in that case.
---
 xen/include/xsm/dummy.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index f4656bd179..ea8b2fd6ee 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -194,9 +194,10 @@ static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
     case XEN_SYSCTL_getdomaininfolist:
         return xsm_default_action(XSM_XS_PRIV, current->domain, NULL);
     case XEN_SYSCTL_readconsole:
-    case XEN_SYSCTL_physinfo:
         return xsm_default_action(XSM_HW_PRIV, current->domain, NULL);
     default:
+        if ( is_hardware_domain(current->domain) )
+            return xsm_default_action(XSM_HW_PRIV, current->domain, NULL);
         return xsm_default_action(XSM_PRIV, current->domain, NULL);
     }
 }
-- 
2.48.1
Re: [PATCH 19/23] xsm/dummy: Allow sysctls to both hardware and control
Posted by Jan Beulich 10 months, 4 weeks ago
On 06.03.2025 23:03, Jason Andryuk wrote:
> xl queries SYSCTL_physinfo for the physical cpus:
> domU:~# xl list
> libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
> libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
> libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
> Name                    ID   Mem VCPUs        State   Time(s)
> Domain-0                 0   800     1     r-----     130.0
> dom0less-1               1   400     1     r-----     130.3
> dom0less-2               2   800     1     r-----     130.3
> 
> Hardware and control are both privileged.  Allow them both access to
> sysctls so they have insight into the running system.  This is coarse
> grained permissions for the dummy policy.

In an earlier patch you alluded to the control domain being guarded against
the hardware one. Shouldn't hwdom be limited operations retrieving info,
but refused to make any changes to the system? Or maybe some kinds of
changes are to be done by hwdom, but then shouldn't be possible to be made
by the control domain?

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -194,9 +194,10 @@ static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>      case XEN_SYSCTL_getdomaininfolist:
>          return xsm_default_action(XSM_XS_PRIV, current->domain, NULL);
>      case XEN_SYSCTL_readconsole:
> -    case XEN_SYSCTL_physinfo:

Didn't you add this line just a single patch ago?

Jan

>          return xsm_default_action(XSM_HW_PRIV, current->domain, NULL);
>      default:
> +        if ( is_hardware_domain(current->domain) )
> +            return xsm_default_action(XSM_HW_PRIV, current->domain, NULL);
>          return xsm_default_action(XSM_PRIV, current->domain, NULL);
>      }
>  }
Re: [PATCH 19/23] xsm/dummy: Allow sysctls to both hardware and control
Posted by Jason Andryuk 10 months, 4 weeks ago
On 2025-03-17 10:36, Jan Beulich wrote:
> On 06.03.2025 23:03, Jason Andryuk wrote:
>> xl queries SYSCTL_physinfo for the physical cpus:
>> domU:~# xl list
>> libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
>> libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
>> libxl: error: libxl_utils.c:817:libxl_cpu_bitmap_alloc: failed to retrieve the maximum number of cpus
>> Name                    ID   Mem VCPUs        State   Time(s)
>> Domain-0                 0   800     1     r-----     130.0
>> dom0less-1               1   400     1     r-----     130.3
>> dom0less-2               2   800     1     r-----     130.3
>>
>> Hardware and control are both privileged.  Allow them both access to
>> sysctls so they have insight into the running system.  This is coarse
>> grained permissions for the dummy policy.
> 
> In an earlier patch you alluded to the control domain being guarded against
> the hardware one. Shouldn't hwdom be limited operations retrieving info,
> but refused to make any changes to the system? Or maybe some kinds of
> changes are to be done by hwdom, but then shouldn't be possible to be made
> by the control domain?

As an example, with ACPI living in the hardware_domain, it would be the 
domain to issue XEN_SYSCTL_pm_op to upload cpufreq data.  But then 
control domain should be in charge of controlling how the system is 
running by setting cpufreq parameters.

>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -194,9 +194,10 @@ static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>>       case XEN_SYSCTL_getdomaininfolist:
>>           return xsm_default_action(XSM_XS_PRIV, current->domain, NULL);
>>       case XEN_SYSCTL_readconsole:
>> -    case XEN_SYSCTL_physinfo:
> 
> Didn't you add this line just a single patch ago?

Yes.  The previous patch was a minimal change.  This patch is a maximal 
change.  I thought this could be rejected and didn't want to merge the 
two.  Though XEN_SYSCTL_physinfo should be handled better even for the 
minimal change.

This patch may go too far, hardware domain does have legitimate use to 
some sysctls.  For a coarse, base policy I went with allowing more to 
hardware domain.  Hardware domain can't be untrusted, so I erred on the 
side of more permissions rather than fewer.

Regards,
Jason