linux-kernel - Re: [PATCH] iommufd: Enforce IOMMU_RESV_SW_MSI upon hwpt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zqf2HFUf/75hRcUU@Asurada-Nvidia>
Date: Mon, 29 Jul 2024 13:05:48 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <kevin.tian@...el.com>, <yi.l.liu@...el.com>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iommufd: Enforce IOMMU_RESV_SW_MSI upon hwpt_paging
 allocation

On Mon, Jul 29, 2024 at 03:24:46PM -0300, Jason Gunthorpe wrote:
> On Sun, Jul 28, 2024 at 04:51:06PM -0700, Nicolin Chen wrote:
> > IOMMU_RESV_SW_MSI is a unique region defined by an IOMMU driver. Though it
> > is eventually used by a device for address translation to an MSI location
> > (including nested cases), practically it is a universal region across all
> > domains allocated for the IOMMU that defines it.
> > 
> > Currently IOMMUFD core fetches and reserves the region during an attach to
> > an hwpt_paging. It works with a hwpt_paging-only case, but might not work
> > with a nested case where a device could directly attach to a hwpt_nested,
> > bypassing the hwpt_paging attachment.
> 
> Well, it does this because the attach is the only place where we have
> *all* the devices available.
> 
> Doing it doing allocation means you get only one device.
>
> So, I'd imagine more like we allocate the MSI region during allocation
> for the device specific during allocation
> 
> But continue to enforce that every attached device also has its MSI
> region allocated.. Which probably just means checking that the
> driver's reported MSI address is the same as the address during
> allocation?

The idea here is to treat IOMMU_RESV_SW_MSI as a per-IOMMU thing
v.s. a per-device thing, because it's defined by SW for an IOMMU
that doesn't have a HW MSI window. In another word, devices don't
really matter, so long as we know which IOMMU that can be decided
by any "one device".

This is based on an enforcement that there won't be a case mixing
IOMMU_RESV_SW_MSI and IOMMU_RESV_MSI on a single IOMMU instance.

> > @@ -364,7 +305,8 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> >  	}
> >  
> >  	if (hwpt_is_paging(hwpt)) {
> > -		rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt), idev);
> > +		rc = iopt_table_enforce_dev_resv_regions(
> > +				&to_hwpt_paging(hwpt)->ioas->iopt, idev->dev);
> >  		if (rc)
> >  			goto err_unlock;
> 
> And this seems kind of weird, shouldn't any change the ioas regions
> happen when the domain is joined to the ioas with the MSI mapping, not
> during attach?? Like we don't want any change of the IOAS to blow away
> the MSI region.

Hmm, not quite getting this part..

This doesn't change the current attach flow except moving that
IOMMU_RESV_SW_MSI from iopt_table_enforce_dev_resv_regions() to
the allocation routine, since other types of reserved regions
are still per-device. The unwra of iommufd_hwpt_paging_attach()
is a cleanup since we moved iommu_get_msi_cookie() away too.

> This should probably get selftest coverage as well, it seems easy
> enough to add to the mock iommu driver?

OK. I will try adding one. I assume we only need a reserved MSI
region in mock device and another testing ioctl verifying if it
is reserved in ioas/iopt upon an allocation?

Thanks
Nicolin