| 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
| ||
|
Message-ID: <08e0126f8075711e7bd59ca4110c2817c905d5a9.camel@linaro.org>
Date: Mon, 20 Nov 2023 11:26:39 +0000
From: André Draszik <andre.draszik@...aro.org>
To: Robin Murphy <robin.murphy@....com>, joro@...tes.org,
will@...nel.org
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org, rafael@...nel.org, lenb@...nel.org,
lpieralisi@...nel.org, quic_zhenhuah@...cinc.com, jgg@...dia.com
Subject: Re: [PATCH] iommu: Avoid more races around device probe
Hi Robin,
On Wed, 2023-11-15 at 18:25 +0000, Robin Murphy wrote:
> It turns out there are more subtle races beyond just the main part of
> __iommu_probe_device() itself running in parallel - the
> dev_iommu_free()
> on the way out of an unsuccessful probe can still manage to trip up
> concurrent accesses to a device's fwspec. Thus, extend the scope of
> iommu_probe_device_lock() to also serialise fwspec creation and
> initial
> retrieval.
>
> Reported-by: Zhenhua Huang <quic_zhenhuah@...cinc.com>
> Link:
> https://lore.kernel.org/linux-iommu/e2e20e1c-6450-4ac5-9804-b0000acdf7de@quicinc.com/
> Fixes: 01657bc14a39 ("iommu: Avoid races around device probe")
> Signed-off-by: Robin Murphy <robin.murphy@....com>
Reviewed-by: André Draszik <andre.draszik@...aro.org>
Tested-by: André Draszik <andre.draszik@...aro.org> (using continuous
boot test loop)
I like that this is easily back-portable to 6.1, thanks for this patch
:-)
Cheers,
André
> ---
>
> This is my idea of a viable fix, since it does not need a 700-line
> diffstat to make the code do what it was already *trying* to do
> anyway.
> This stuff should fundamentally not be hanging off driver probe in
> the
> first place, so I'd rather get on with removing the underlying
> brokenness than waste time and effort polishing it any further.
>
> drivers/acpi/scan.c | 7 ++++++-
> drivers/iommu/iommu.c | 20 ++++++++++----------
> drivers/iommu/of_iommu.c | 12 +++++++++---
> include/linux/iommu.h | 1 +
> 4 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fa5dd71a80fa..02bb2cce423f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1568,17 +1568,22 @@ static const struct iommu_ops
> *acpi_iommu_configure_id(struct device *dev,
> int err;
> const struct iommu_ops *ops;
>
> + /* Serialise to make dev->iommu stable under our potential
> fwspec */
> + mutex_lock(&iommu_probe_device_lock);
> /*
> * If we already translated the fwspec there is nothing left
> to do,
> * return the iommu_ops.
> */
> ops = acpi_iommu_fwspec_ops(dev);
> - if (ops)
> + if (ops) {
> + mutex_unlock(&iommu_probe_device_lock);
> return ops;
> + }
>
> err = iort_iommu_configure_id(dev, id_in);
> if (err && err != -EPROBE_DEFER)
> err = viot_iommu_configure(dev);
> + mutex_unlock(&iommu_probe_device_lock);
>
> /*
> * If we have reason to believe the IOMMU driver missed the
> initial
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f17a1113f3d6..e0c962648dde 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -485,11 +485,12 @@ static void iommu_deinit_device(struct device
> *dev)
> dev_iommu_free(dev);
> }
>
> +DEFINE_MUTEX(iommu_probe_device_lock);
> +
> static int __iommu_probe_device(struct device *dev, struct list_head
> *group_list)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> struct iommu_group *group;
> - static DEFINE_MUTEX(iommu_probe_device_lock);
> struct group_device *gdev;
> int ret;
>
> @@ -502,17 +503,15 @@ static int __iommu_probe_device(struct device
> *dev, struct list_head *group_list
> * probably be able to use device_lock() here to minimise
> the scope,
> * but for now enforcing a simple global ordering is fine.
> */
> - mutex_lock(&iommu_probe_device_lock);
> + lockdep_assert_held(&iommu_probe_device_lock);
>
> /* Device is probed already if in a group */
> - if (dev->iommu_group) {
> - ret = 0;
> - goto out_unlock;
> - }
> + if (dev->iommu_group)
> + return 0;
>
> ret = iommu_init_device(dev, ops);
> if (ret)
> - goto out_unlock;
> + return ret;
>
> group = dev->iommu_group;
> gdev = iommu_group_alloc_device(group, dev);
> @@ -548,7 +547,6 @@ static int __iommu_probe_device(struct device
> *dev, struct list_head *group_list
> list_add_tail(&group->entry, group_list);
> }
> mutex_unlock(&group->mutex);
> - mutex_unlock(&iommu_probe_device_lock);
>
> if (dev_is_pci(dev))
> iommu_dma_set_pci_32bit_workaround(dev);
> @@ -562,8 +560,6 @@ static int __iommu_probe_device(struct device
> *dev, struct list_head *group_list
> iommu_deinit_device(dev);
> mutex_unlock(&group->mutex);
> iommu_group_put(group);
> -out_unlock:
> - mutex_unlock(&iommu_probe_device_lock);
>
> return ret;
> }
> @@ -573,7 +569,9 @@ int iommu_probe_device(struct device *dev)
> const struct iommu_ops *ops;
> int ret;
>
> + mutex_lock(&iommu_probe_device_lock);
> ret = __iommu_probe_device(dev, NULL);
> + mutex_unlock(&iommu_probe_device_lock);
> if (ret)
> return ret;
>
> @@ -1822,7 +1820,9 @@ static int probe_iommu_group(struct device
> *dev, void *data)
> struct list_head *group_list = data;
> int ret;
>
> + mutex_lock(&iommu_probe_device_lock);
> ret = __iommu_probe_device(dev, group_list);
> + mutex_unlock(&iommu_probe_device_lock);
> if (ret == -ENODEV)
> ret = 0;
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 157b286e36bf..c25b4ae6aeee 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -112,16 +112,20 @@ const struct iommu_ops
> *of_iommu_configure(struct device *dev,
> const u32 *id)
> {
> const struct iommu_ops *ops = NULL;
> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> + struct iommu_fwspec *fwspec;
> int err = NO_IOMMU;
>
> if (!master_np)
> return NULL;
>
> + /* Serialise to make dev->iommu stable under our potential
> fwspec */
> + mutex_lock(&iommu_probe_device_lock);
> + fwspec = dev_iommu_fwspec_get(dev);
> if (fwspec) {
> - if (fwspec->ops)
> + if (fwspec->ops) {
> + mutex_unlock(&iommu_probe_device_lock);
> return fwspec->ops;
> -
> + }
> /* In the deferred case, start again from scratch */
> iommu_fwspec_free(dev);
> }
> @@ -155,6 +159,8 @@ const struct iommu_ops *of_iommu_configure(struct
> device *dev,
> fwspec = dev_iommu_fwspec_get(dev);
> ops = fwspec->ops;
> }
> + mutex_unlock(&iommu_probe_device_lock);
> +
> /*
> * If we have reason to believe the IOMMU driver missed the
> initial
> * probe for dev, replay it to get things in order.
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ec289c1016f5..6291aa7b079b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -845,6 +845,7 @@ static inline void dev_iommu_priv_set(struct
> device *dev, void *priv)
> dev->iommu->priv = priv;
> }
>
> +extern struct mutex iommu_probe_device_lock;
> int iommu_probe_device(struct device *dev);
>
> int iommu_dev_enable_feature(struct device *dev, enum
> iommu_dev_features f);
Powered by blists - more mailing lists