linux-kernel - Re: [PATCH v4 5/6] iommu/mediatek: Add mt8173 IOMMU driver

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: <20150811153947.GF14980@8bytes.org>
Date:	Tue, 11 Aug 2015 17:39:47 +0200
From:	Joerg Roedel <joro@...tes.org>
To:	Yong Wu <yong.wu@...iatek.com>
Cc:	Thierry Reding <treding@...dia.com>,
	Mark Rutland <mark.rutland@....com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Robin Murphy <robin.murphy@....com>,
	Will Deacon <will.deacon@....com>,
	Daniel Kurtz <djkurtz@...gle.com>,
	Tomasz Figa <tfiga@...gle.com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Rob Herring <robh+dt@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	linux-mediatek@...ts.infradead.org,
	Sasha Hauer <kernel@...gutronix.de>,
	srv_heupstream@...iatek.com, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	iommu@...ts.linux-foundation.org, pebolle@...cali.nl,
	arnd@...db.de, mitchelh@...eaurora.org, youhua.li@...iatek.com,
	k.zhang@...iatek.com, frederic.chen@...iatek.com
Subject: Re: [PATCH v4 5/6] iommu/mediatek: Add mt8173 IOMMU driver

On Mon, Aug 03, 2015 at 06:21:18PM +0800, Yong Wu wrote:
> +/*
> + * There is only one iommu domain called the m4u domain that
> + * all Multimedia modules share.
> + */
> +static struct mtk_iommu_domain	*m4udom;

What is the reason you only implement one domain? Can't the IOMMU
isolate different devices from each other?


> +static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
> +{
> +	struct mtk_iommu_domain *priv;
> +
> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> +		return NULL;
> +
> +	if (m4udom)/* The m4u domain exist. */
> +		return &m4udom->domain;

This is not going to work. If you always return the same domain the
iommu core might re-initialize domain state (and overwrite changed
state). At the moment this is only the domain-type which will change
every time this function is called, but there might be more.

> +static int mtk_iommu_add_device(struct device *dev)
> +{
> +	struct iommu_group *group;
> +	int ret;
> +
> +	if (!dev->archdata.iommu) /* Not a iommu client device */
> +		return -ENODEV;
> +
> +	group = iommu_group_get(dev);
> +	if (!group) {
> +		group = iommu_group_alloc();
> +		if (IS_ERR(group)) {
> +			dev_err(dev, "Failed to allocate IOMMU group\n");
> +			return PTR_ERR(group);
> +		}
> +	}
> +
> +	ret = iommu_group_add_device(group, dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to add IOMMU group\n");
> +		goto err_group_put;
> +	}
> +
> +	ret = iommu_attach_group(&m4udom->domain, group);
> +	if (ret)
> +		dev_err(dev, "Failed to attach IOMMU group\n");
> +
> +err_group_put:
> +	iommu_group_put(group);
> +	return ret;
> +}

Putting every device into its own group indicates that the IOMMU can
isolate between single devices on the bus, which makes it even more
questionable that you only allow one domain for the whole driver.


	Joerg

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/