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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 16, 2026 at 11:51:54AM +0200, Jan Beulich wrote:
> 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".

Yeah, I've noticed after looking at the next patch.

> 
> > 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.

Since you have to adjust the commit message, I wouldn't mind if you
also want to adjust the logic to remove the extra branch.