Permitting writes when the P2M type says "read-only" can't be correct.

Fixes: 1661158723a ("xen/arm: Extend copy_to_guest to support copying from/to guest physical address")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
What exactly p2m_ram_ro means on Arm is unclear: The comment next to its
definition says one thing, its use in get_page_from_gfn() says another.
(I remember raising this point before, i.e. it feels a little odd that the
ambiguity still exists.) The patch here assumes the comment is what is
wrong.

--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -44,7 +44,7 @@ static struct page_info *translate_get_p
     if ( !page )
         return NULL;
 
-    if ( !p2m_is_ram(p2mt) )
+    if ( write ? p2mt != p2m_ram_rw : !p2m_is_ram(p2mt) )
     {
         put_page(page);
         return NULL;

On 16/02/2026 16:20, Jan Beulich wrote:
> Permitting writes when the P2M type says "read-only" can't be correct.
> 
> Fixes: 1661158723a ("xen/arm: Extend copy_to_guest to support copying from/to guest physical address")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

> ---
> What exactly p2m_ram_ro means on Arm is unclear: The comment next to its
> definition says one thing, its use in get_page_from_gfn() says another.
> (I remember raising this point before, i.e. it feels a little odd that the
> ambiguity still exists.) The patch here assumes the comment is what is
> wrong.
> 
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -44,7 +44,7 @@ static struct page_info *translate_get_p
>      if ( !page )
>          return NULL;
>  
> -    if ( !p2m_is_ram(p2mt) )
> +    if ( write ? p2mt != p2m_ram_rw : !p2m_is_ram(p2mt) )
>      {
>          put_page(page);
>          return NULL;

The ambiguity you mention is indeed problematic. This mixes page type with p2m
type. The comment "The p2m_type is based on the type of the page" admits this
conflation for DOMID_XEN.

AFAICT, p2m_ram_ro is not used on Arm for normal domains. The only use is in
get_page_from_gfn() for DOMID_XEN. Maybe we could change get_page_from_gfn() to
always return p2m_ram_rw since DOMID_XEN has direct 1:1 access anyway?

~Michal

On 17.02.2026 16:28, Orzel, Michal wrote:
> 
> 
> On 16/02/2026 16:20, Jan Beulich wrote:
>> Permitting writes when the P2M type says "read-only" can't be correct.
>>
>> Fixes: 1661158723a ("xen/arm: Extend copy_to_guest to support copying from/to guest physical address")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks.

>> ---
>> What exactly p2m_ram_ro means on Arm is unclear: The comment next to its
>> definition says one thing, its use in get_page_from_gfn() says another.
>> (I remember raising this point before, i.e. it feels a little odd that the
>> ambiguity still exists.) The patch here assumes the comment is what is
>> wrong.
>>
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -44,7 +44,7 @@ static struct page_info *translate_get_p
>>      if ( !page )
>>          return NULL;
>>  
>> -    if ( !p2m_is_ram(p2mt) )
>> +    if ( write ? p2mt != p2m_ram_rw : !p2m_is_ram(p2mt) )
>>      {
>>          put_page(page);
>>          return NULL;
> 
> The ambiguity you mention is indeed problematic. This mixes page type with p2m
> type. The comment "The p2m_type is based on the type of the page" admits this
> conflation for DOMID_XEN.
> 
> AFAICT, p2m_ram_ro is not used on Arm for normal domains. The only use is in
> get_page_from_gfn() for DOMID_XEN. Maybe we could change get_page_from_gfn() to
> always return p2m_ram_rw since DOMID_XEN has direct 1:1 access anyway?

But that's not correct for cases where share_xen_page_with_privileged_guest()
is passed SHARE_ro. XENMAPSPACE_gmfn_foreign requests have to result in r/o
mappings in that case.

Jan

On 17/02/2026 16:54, Jan Beulich wrote:
> On 17.02.2026 16:28, Orzel, Michal wrote:
>>
>>
>> On 16/02/2026 16:20, Jan Beulich wrote:
>>> Permitting writes when the P2M type says "read-only" can't be correct.
>>>
>>> Fixes: 1661158723a ("xen/arm: Extend copy_to_guest to support copying from/to guest physical address")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> Thanks.
> 
>>> ---
>>> What exactly p2m_ram_ro means on Arm is unclear: The comment next to its
>>> definition says one thing, its use in get_page_from_gfn() says another.
>>> (I remember raising this point before, i.e. it feels a little odd that the
>>> ambiguity still exists.) The patch here assumes the comment is what is
>>> wrong.
>>>
>>> --- a/xen/arch/arm/guestcopy.c
>>> +++ b/xen/arch/arm/guestcopy.c
>>> @@ -44,7 +44,7 @@ static struct page_info *translate_get_p
>>>      if ( !page )
>>>          return NULL;
>>>  
>>> -    if ( !p2m_is_ram(p2mt) )
>>> +    if ( write ? p2mt != p2m_ram_rw : !p2m_is_ram(p2mt) )
>>>      {
>>>          put_page(page);
>>>          return NULL;
>>
>> The ambiguity you mention is indeed problematic. This mixes page type with p2m
>> type. The comment "The p2m_type is based on the type of the page" admits this
>> conflation for DOMID_XEN.
>>
>> AFAICT, p2m_ram_ro is not used on Arm for normal domains. The only use is in
>> get_page_from_gfn() for DOMID_XEN. Maybe we could change get_page_from_gfn() to
>> always return p2m_ram_rw since DOMID_XEN has direct 1:1 access anyway?
> 
> But that's not correct for cases where share_xen_page_with_privileged_guest()
> is passed SHARE_ro. XENMAPSPACE_gmfn_foreign requests have to result in r/o
> mappings in that case.
Yes. Therefore, on Arm:
- p2m_ram_ro is never stored in P2M tables for normal domains
- it's only used by get_page_from_gfn() for DOMID_XEN pages
- it's used as a signal to install p2m_map_foreign_ro mappings

The code should stay as is then and we could modify the comment to say:
/* Read-only RAM; only used for DOMID_XEN */

~Michal

Hi Michal,

On 18/02/2026 08:36, Orzel, Michal wrote:
>> But that's not correct for cases where share_xen_page_with_privileged_guest()
>> is passed SHARE_ro. XENMAPSPACE_gmfn_foreign requests have to result in r/o
>> mappings in that case.
> Yes. Therefore, on Arm:
> - p2m_ram_ro is never stored in P2M tables for normal domains

p2m_set_permission() is able to deal with p2m_ram_ro. So this could in 
theory happen.

> - it's only used by get_page_from_gfn() for DOMID_XEN pages
> - it's used as a signal to install p2m_map_foreign_ro mappings
> 
> The code should stay as is then and we could modify the comment to say:
 > /* Read-only RAM; only used for DOMID_XEN */

With what I wrote above, I don't think we should add such comment.

Cheers,

-- 
Julien Grall

Hi Julien,

On 21/02/2026 12:26, Julien Grall wrote:
> Hi Michal,
> 
> On 18/02/2026 08:36, Orzel, Michal wrote:
>>> But that's not correct for cases where share_xen_page_with_privileged_guest()
>>> is passed SHARE_ro. XENMAPSPACE_gmfn_foreign requests have to result in r/o
>>> mappings in that case.
>> Yes. Therefore, on Arm:
>> - p2m_ram_ro is never stored in P2M tables for normal domains
> 
> p2m_set_permission() is able to deal with p2m_ram_ro. So this could in 
> theory happen.
Only in theory. As of today, this is the unreachable code. In fact I checked our
coverage reports for safety and indeed it shows up as an unreachable code. There
is no path in Xen that can lead to that point.

> 
>> - it's only used by get_page_from_gfn() for DOMID_XEN pages
>> - it's used as a signal to install p2m_map_foreign_ro mappings
>>
>> The code should stay as is then and we could modify the comment to say:
>  > /* Read-only RAM; only used for DOMID_XEN */
> 
> With what I wrote above, I don't think we should add such comment.
I think the goal here is to make the comment reflect the current situation (and
as of now it's only used for DOMID_XEN). Taking what I wrote above, do you still
think we should not update it? Once we have a use for RO for normal domains, we
could then update the comment to reflect a new reality.

~Michal