Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Sep 13, 2023 at 07:46:45PM +0100, Robin Murphy wrote:
> On 2023-09-13 15:39, Jason Gunthorpe wrote:
> > On Tue, Sep 12, 2023 at 05:18:44PM +0100, Robin Murphy wrote:
> > > The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
> > > bit stifled, since it is quite likely now that a non-paging domain
> > > won't have a pgsize_bitmap and/or mapping ops, and thus get caught
> > > by the earlier condition anyway. Swap them around to test the more
> > > fundamental condition first, then we can reasonably also upgrade
> > > the other to a WARN_ON, since if a driver does ever expose a paging
> > > domain without the means to actually page, it's clearly very broken.
> > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/iommu/iommu.c | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index c83f2e4c56f5..391bcae4d02d 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> > >   	phys_addr_t orig_paddr = paddr;
> > >   	int ret = 0;
> > > -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> > > -		return -ENODEV;
> > > -
> > >   	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
> > >   		return -EINVAL;
> > 
> > Why isn't this a WARN_ON? The caller is clearly lost its mind if it
> > is calling map on a non paging domain..
> 
> Sure it's a dumb thing to do, however I don't think we can reasonably say
> without question that an external caller being dumb represents an unexpected
> and serious loss of internal consistency.

WARN_ON is not just for "serious loss of internal consistency" it
should be use in all places where invariant are violated. We can't
guess why the caller is using this wrong (most likely it is UAF or
memory corruption), but if this fires something definately has gone
wrong with the kernel.

eg if syzkaller somehow hits this we want the WARN_ON so it reports
it.

> > > +	if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> > > +		return -ENODEV;
> > > +
> > 
> > And these could be moved to after attach so they are not in any fast
> > path, and eventually to after alloc..
> 
> Perhaps, although TBH I can't imagine it making any appreciable difference -
> they're both values we need to load and use soon anyway, so on a sensible
> CPU, the additional overhead should only really be a couple of not-taken
> branch-if-zero instructions, which is nothing at all compared to how much
> goes on in the main loop and the driver op call itself.

IMHO it is a cleaner pattern to check the objects the driver creates
when it creates them, not when we go to try to use them..

Jason