| 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
| ||
|
Date: Thu, 16 Apr 2015 16:00:36 +1000
From: David Gibson <david@...son.dropbear.id.au>
To: Alexey Kardashevskiy <aik@...abs.ru>
Cc: linuxppc-dev@...ts.ozlabs.org,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Alex Williamson <alex.williamson@...hat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH kernel v8 13/31] vfio: powerpc/spapr: powerpc/iommu:
Rework IOMMU ownership control
On Fri, Apr 10, 2015 at 04:30:55PM +1000, Alexey Kardashevskiy wrote:
> This replaces iommu_take_ownership()/iommu_release_ownership() calls
> with the callback calls and it is up to the platform code to call
> iommu_take_ownership()/iommu_release_ownership() if needed.
I think this commit message is out of date - I don't see any callbacks
here.
> Signed-off-by: Alexey Kardashevskiy <aik@...abs.ru>
> ---
> arch/powerpc/include/asm/iommu.h | 4 +--
> arch/powerpc/kernel/iommu.c | 50 ++++++++++++++++++++++++++++---------
> drivers/vfio/vfio_iommu_spapr_tce.c | 4 +--
> 3 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index 667aa1a..b9e50d3 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -225,8 +225,8 @@ extern unsigned long iommu_clear_tce(struct iommu_table *tbl,
> unsigned long entry);
>
> extern void iommu_flush_tce(struct iommu_table *tbl);
> -extern int iommu_take_ownership(struct iommu_table *tbl);
> -extern void iommu_release_ownership(struct iommu_table *tbl);
> +extern int iommu_take_ownership(struct iommu_table_group *table_group);
> +extern void iommu_release_ownership(struct iommu_table_group *table_group);
>
> extern enum dma_data_direction iommu_tce_direction(unsigned long tce);
> extern unsigned long iommu_direction_to_tce_perm(enum dma_data_direction dir);
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index fd49c8e..7d6089b 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1050,7 +1050,7 @@ int iommu_tce_build(struct iommu_table *tbl, unsigned long entry,
> }
> EXPORT_SYMBOL_GPL(iommu_tce_build);
>
> -int iommu_take_ownership(struct iommu_table *tbl)
> +static int iommu_table_take_ownership(struct iommu_table *tbl)
> {
> unsigned long sz = (tbl->it_size + 7) >> 3;
>
> @@ -1064,19 +1064,36 @@ int iommu_take_ownership(struct iommu_table *tbl)
>
> memset(tbl->it_map, 0xff, sz);
>
> - /*
> - * Disable iommu bypass, otherwise the user can DMA to all of
> - * our physical memory via the bypass window instead of just
> - * the pages that has been explicitly mapped into the iommu
> - */
> - if (tbl->set_bypass)
> - tbl->set_bypass(tbl, false);
The code to disable bypass is removed, and doesn't seem to be replaced
with anything. That doesn't look safe.
> + return 0;
> +}
> +
> +static void iommu_table_release_ownership(struct iommu_table *tbl);
> +
> +int iommu_take_ownership(struct iommu_table_group *table_group)
> +{
> + int i, j, rc = 0;
> +
> + for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
> + struct iommu_table *tbl = &table_group->tables[i];
> +
> + if (!tbl->it_map)
> + continue;
> +
> + rc = iommu_table_take_ownership(tbl);
> + if (rc) {
> + for (j = 0; j < i; ++j)
> + iommu_table_release_ownership(
> + &table_group->tables[j]);
> +
> + return rc;
> + }
> + }
>
> return 0;
> }
> EXPORT_SYMBOL_GPL(iommu_take_ownership);
>
> -void iommu_release_ownership(struct iommu_table *tbl)
> +static void iommu_table_release_ownership(struct iommu_table *tbl)
> {
> unsigned long sz = (tbl->it_size + 7) >> 3;
>
> @@ -1086,9 +1103,18 @@ void iommu_release_ownership(struct iommu_table *tbl)
> if (tbl->it_offset == 0)
> set_bit(0, tbl->it_map);
>
> - /* The kernel owns the device now, we can restore the iommu bypass */
> - if (tbl->set_bypass)
> - tbl->set_bypass(tbl, true);
> +}
> +
> +extern void iommu_release_ownership(struct iommu_table_group *table_group)
> +{
> + int i;
> +
> + for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
> + struct iommu_table *tbl = &table_group->tables[i];
> +
> + if (tbl->it_map)
> + iommu_table_release_ownership(tbl);
> + }
> }
> EXPORT_SYMBOL_GPL(iommu_release_ownership);
>
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index d61aad2..9f38351 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -535,7 +535,7 @@ static int tce_iommu_attach_group(void *iommu_data,
> goto unlock_exit;
> }
>
> - ret = iommu_take_ownership(&table_group->tables[0]);
> + ret = iommu_take_ownership(table_group);
> if (!ret)
> container->grp = iommu_group;
>
> @@ -572,7 +572,7 @@ static void tce_iommu_detach_group(void *iommu_data,
> table_group = iommu_group_get_iommudata(iommu_group);
> BUG_ON(!table_group);
>
> - iommu_release_ownership(&table_group->tables[0]);
> + iommu_release_ownership(table_group);
>
> unlock_exit:
> mutex_unlock(&container->lock);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists