Re: [PATCH for-4.22? 7/9] domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2026 11:08, Roger Pau Monné wrote:
> On Mon, Jun 15, 2026 at 04:15:36PM +0200, Jan Beulich wrote:
>> Like is already done for I/O ports on x86 and for IRQ unbinding, check
>> only the requesting domain's permissions (for it to not interfere with
>> MMIO backed by another stubdom DM), but not the target domain's: Removal
>> should be okay even (perhaps: especially) when permissions were already
>> revoked.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -436,11 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>              goto domctl_out_unlock_rcuonly;
>>  #endif
>>  
>> +        /*
>> +         * NB: The double lock isn't really needed when !add, but is used 
>> anyway
>> +         * to keep things simple.
>> +         */
>>          iocaps_double_lock(d, false);
>>  
>>          ret = -EPERM;
>>          if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
>> -             !iomem_access_permitted(d, mfn, mfn_end) )
>> +             (add && !iomem_access_permitted(d, mfn, mfn_end)) )
> 
> You seem to be doing the opposite of what the commit message states
> here, and checking for permissions on the target domain, not
> permissions of the requesting domain?

I'm always checking permissions of the requesting domain, while the
target's are now checked only for "add". That's what the description
also says.

What's wrong with the description is ...

> XEN_DOMCTL_ioport_mapping does check against current->domain, and not
> against d.

... that it suggests this to be the behavior at the point of this patch,
when it really is moved to that only in patch 8. The patches used to be
ordered differently earlier on. I guess I should change the wording to
be closer to what's used in "x86/domctl: don't imply I/O port permissions
from I/O port mapping".

> FWIW, we could also remove one branch here by doing:
> 
> ret = -EPERM
> if ( add && iomem_access_permitted(current->domain, mfn, mfn_end) )
> {
>     /* add logic. */
> }
> else if ( !add )
> {
>     /* remove logic. */
> }

Indeed I was wondering whether something like this would be worthwhile,
but I opted for the variant with less overall churn.

Jan