This is useful for a combined hardware/xenstore domain that will run
init-dom0less and xenstored.  init-dom0less calls xc_hvm_param_get() to
retrieve the xenstore event channel and pfn to configure xenstore for a
guest.  With a hypervisor-allocated event channel and page, the
set_hvm_param is not needed, and the normal domid permissions will allow
xenstored to connect.

Similarly, a hyperlaunch-ed xenstore stubdom needs to read a domain's
xenstore event channel out of hvm_param.

This allows reading but not modifying the guest, so allow the permission.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
 xen/arch/arm/hvm.c      |  2 +-
 xen/arch/x86/hvm/hvm.c  |  8 ++++----
 xen/include/xsm/dummy.h | 14 ++++++++++++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 86e49bf474..b50ca10cee 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -89,7 +89,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( d == NULL )
             return -ESRCH;
 
-        rc = xsm_hvm_param(XSM_TARGET, d, op);
+        rc = xsm_hvm_param(XSM_OTHER, d, op);
         if ( rc )
             goto param_fail;
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 969e43c2f2..f3f1002cc9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4177,7 +4177,7 @@ static int hvm_allow_set_param(struct domain *d,
     uint64_t value;
     int rc;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    rc = xsm_hvm_param(XSM_OTHER, d, HVMOP_set_param);
     if ( rc )
         return rc;
 
@@ -4458,7 +4458,7 @@ static int hvm_allow_get_param(struct domain *d,
 {
     int rc;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    rc = xsm_hvm_param(XSM_OTHER, d, HVMOP_get_param);
     if ( rc )
         return rc;
 
@@ -5055,7 +5055,7 @@ static int hvmop_get_mem_type(
     if ( d == NULL )
         return -ESRCH;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_mem_type);
+    rc = xsm_hvm_param(XSM_OTHER, d, HVMOP_get_mem_type);
     if ( rc )
         goto out;
 
@@ -5148,7 +5148,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( unlikely(d != current->domain) )
             rc = -EOPNOTSUPP;
         else if ( is_hvm_domain(d) && paging_mode_shadow(d) )
-            rc = xsm_hvm_param(XSM_TARGET, d, op);
+            rc = xsm_hvm_param(XSM_OTHER, d, op);
         if ( !rc )
             pagetable_dying(a.gpa);
 
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index f2d6662a9d..06f4eccf5f 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -614,8 +614,18 @@ static XSM_INLINE int cf_check xsm_map_gmfn_foreign(
 static XSM_INLINE int cf_check xsm_hvm_param(
     XSM_DEFAULT_ARG struct domain *d, unsigned long op)
 {
-    XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, current->domain, d);
+    XSM_ASSERT_ACTION(XSM_OTHER);
+    switch ( op )
+    {
+    case HVMOP_get_param:
+        /* A domain can query itself, or a DM can query its target. */
+        if ( !xsm_default_action(XSM_TARGET, current->domain, d) )
+            return 0;
+        /* Xenstore domain needs to be able to query for mapping. */
+        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
+    default:
+        return xsm_default_action(XSM_TARGET, current->domain, d);
+    }
 }
 
 static XSM_INLINE int cf_check xsm_hvm_param_altp2mhvm(
-- 
2.48.1

On 06.03.2025 23:03, Jason Andryuk wrote:
> This is useful for a combined hardware/xenstore domain that will run
> init-dom0less and xenstored.  init-dom0less calls xc_hvm_param_get() to
> retrieve the xenstore event channel and pfn to configure xenstore for a
> guest.  With a hypervisor-allocated event channel and page, the
> set_hvm_param is not needed, and the normal domid permissions will allow
> xenstored to connect.
> 
> Similarly, a hyperlaunch-ed xenstore stubdom needs to read a domain's
> xenstore event channel out of hvm_param.
> 
> This allows reading but not modifying the guest, so allow the permission.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

Since this is exposing the entire param space to Xenstore, what I'm missing
is a security discussion for existing as well as potential future params.
There could well be some that better wouldn't be available for Xenstrore to
fetch.

Jan

On 2025-03-17 10:18, Jan Beulich wrote:
> On 06.03.2025 23:03, Jason Andryuk wrote:
>> This is useful for a combined hardware/xenstore domain that will run
>> init-dom0less and xenstored.  init-dom0less calls xc_hvm_param_get() to
>> retrieve the xenstore event channel and pfn to configure xenstore for a
>> guest.  With a hypervisor-allocated event channel and page, the
>> set_hvm_param is not needed, and the normal domid permissions will allow
>> xenstored to connect.
>>
>> Similarly, a hyperlaunch-ed xenstore stubdom needs to read a domain's
>> xenstore event channel out of hvm_param.
>>
>> This allows reading but not modifying the guest, so allow the permission.
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> Since this is exposing the entire param space to Xenstore, what I'm missing
> is a security discussion for existing as well as potential future params.
> There could well be some that better wouldn't be available for Xenstrore to
> fetch.

I can't speak for future parameters, but existing HVM_PARAMs didn't seem 
sensitive to me.  The safest choice is to just pass the index to 
xsm_hvm_param() and allow just HVM_PARAM_STORE_EVTCHN (and 
HVM_PARAM_STORE_PFN) for the xenstore domain.

This works for ARM and x86 HVM/PVH.  PV doesn't have a way to determine 
a domain's event channel port, FWICT.

Regards,
Jason

On 17.03.25 15:50, Jason Andryuk wrote:
> On 2025-03-17 10:18, Jan Beulich wrote:
>> On 06.03.2025 23:03, Jason Andryuk wrote:
>>> This is useful for a combined hardware/xenstore domain that will run
>>> init-dom0less and xenstored.  init-dom0less calls xc_hvm_param_get() to
>>> retrieve the xenstore event channel and pfn to configure xenstore for a
>>> guest.  With a hypervisor-allocated event channel and page, the
>>> set_hvm_param is not needed, and the normal domid permissions will allow
>>> xenstored to connect.
>>>
>>> Similarly, a hyperlaunch-ed xenstore stubdom needs to read a domain's
>>> xenstore event channel out of hvm_param.
>>>
>>> This allows reading but not modifying the guest, so allow the permission.
>>>
>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>
>> Since this is exposing the entire param space to Xenstore, what I'm missing
>> is a security discussion for existing as well as potential future params.
>> There could well be some that better wouldn't be available for Xenstrore to
>> fetch.
> 
> I can't speak for future parameters, but existing HVM_PARAMs didn't seem 
> sensitive to me.  The safest choice is to just pass the index to xsm_hvm_param() 
> and allow just HVM_PARAM_STORE_EVTCHN (and HVM_PARAM_STORE_PFN) for the xenstore 
> domain.
> 
> This works for ARM and x86 HVM/PVH.  PV doesn't have a way to determine a 
> domain's event channel port, FWICT.

For what are you needing HVM_PARAM_STORE_PFN? The GNTTAB_RESERVED_XENSTORE
grant id should be enough to map the guest's Xenstore page?

And with that I'd rather suggest to expand struct xenstore_domain_interface
with the event channel port and let the component doing the seeding of the
grant table write the port into the struct.

This would enable Xenstore to just map the guest's Xenstore page and read
the event channel port from it. No additional hypercall permission needed.
And this would even work with PV domains. And as the Xenstore page is zeroed
initially, any event channel port != 0 could be regarded to be valid (a guest
choosing to write a bogus value there would just shoot itself in the foot by
harming its own Xenstore connection).


Juergen

On 2025-03-17 11:19, Juergen Gross wrote:
> On 17.03.25 15:50, Jason Andryuk wrote:
>> On 2025-03-17 10:18, Jan Beulich wrote:
>>> On 06.03.2025 23:03, Jason Andryuk wrote:
>>>> This is useful for a combined hardware/xenstore domain that will run
>>>> init-dom0less and xenstored.  init-dom0less calls xc_hvm_param_get() to
>>>> retrieve the xenstore event channel and pfn to configure xenstore for a
>>>> guest.  With a hypervisor-allocated event channel and page, the
>>>> set_hvm_param is not needed, and the normal domid permissions will 
>>>> allow
>>>> xenstored to connect.
>>>>
>>>> Similarly, a hyperlaunch-ed xenstore stubdom needs to read a domain's
>>>> xenstore event channel out of hvm_param.
>>>>
>>>> This allows reading but not modifying the guest, so allow the 
>>>> permission.
>>>>
>>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>>
>>> Since this is exposing the entire param space to Xenstore, what I'm 
>>> missing
>>> is a security discussion for existing as well as potential future 
>>> params.
>>> There could well be some that better wouldn't be available for 
>>> Xenstrore to
>>> fetch.
>>
>> I can't speak for future parameters, but existing HVM_PARAMs didn't 
>> seem sensitive to me.  The safest choice is to just pass the index to 
>> xsm_hvm_param() and allow just HVM_PARAM_STORE_EVTCHN (and 
>> HVM_PARAM_STORE_PFN) for the xenstore domain.
>>
>> This works for ARM and x86 HVM/PVH.  PV doesn't have a way to 
>> determine a domain's event channel port, FWICT.
> 
> For what are you needing HVM_PARAM_STORE_PFN? The GNTTAB_RESERVED_XENSTORE
> grant id should be enough to map the guest's Xenstore page?

Indeed, HVM_PARAM_STORE_PFN is not needed and I expect only using 
GNTTAB_RESERVED_XENSTORE to map the grant.  I just listed it as a 
compliment to the event channel.

> And with that I'd rather suggest to expand struct xenstore_domain_interface
> with the event channel port and let the component doing the seeding of the
> grant table write the port into the struct.
> 
> This would enable Xenstore to just map the guest's Xenstore page and read
> the event channel port from it. No additional hypercall permission needed.
> And this would even work with PV domains. And as the Xenstore page is 
> zeroed
> initially, any event channel port != 0 could be regarded to be valid (a 
> guest
> choosing to write a bogus value there would just shoot itself in the 
> foot by
> harming its own Xenstore connection).

Yes, that is a good idea.  I had also considered defining a reserved 
event channel for xenstore, but wasn't sure how that would be received. 
I like this better.

Thanks,
Jason