linux-kernel - Re: [PATCH] iommu: Check dev->iommu in iommu_dev

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 26 Jan 2021 17:24:45 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "jean-philippe@...aro.org" <jean-philippe@...aro.org>,
        "will@...nel.org" <will@...nel.org>,
        "linuxarm@...neuler.org" <linuxarm@...neuler.org>,
        "Zengtao (B)" <prime.zeng@...ilicon.com>
Subject: Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions

On 2021-01-26 16:40, Shameerali Kolothum Thodi wrote:
> Hi Robin,
> 
>> -----Original Message-----
>> From: Robin Murphy [mailto:robin.murphy@....com]
>> Sent: 26 January 2021 13:51
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
>> Cc: linux-kernel@...r.kernel.org; iommu@...ts.linux-foundation.org;
>> jean-philippe@...aro.org; will@...nel.org; linuxarm@...neuler.org; Zengtao
>> (B) <prime.zeng@...ilicon.com>
>> Subject: Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
>>
>> On Tue, 26 Jan 2021 13:06:29 +0000
>> Shameer Kolothum <shameerali.kolothum.thodi@...wei.com> wrote:
>>
>>> The device iommu probe/attach might have failed leaving dev->iommu to
>>> NULL and device drivers may still invoke these functions resulting a
>>> crash in iommu vendor driver code. Hence make sure we check that.
>>>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>
>>> ---
>>>   drivers/iommu/iommu.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
>>> ffeebda8d6de..cb68153c5cc0 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev,
>>> enum iommu_dev_features feat) {
>>>   	const struct iommu_ops *ops = dev->bus->iommu_ops;
>>>
>>> -	if (ops && ops->dev_has_feat)
>>> +	if (dev->iommu && ops && ops->dev_has_feat)
>>>   		return ops->dev_has_feat(dev, feat);
>>
>> Might make sense to make these more self-contained, e.g.:
>>
>> 	if (dev->iommu && dev->iommu->ops->foo)
>> 		dev->iommu->ops->foo()
> 
> Right. Does that mean adding ops to "struct dev_iommu" or retrieve ops like
> below,
> 
> if (dev->iommu && dev->iommu->iommu_dev->ops->foo)
>   		dev->iommu->iommu_dev->ops->foo()
>   
> Sorry, not clear to me.

Bleh, I was thinking that dev->iommu pointed directly to a struct 
iommu_device there, sorry. There are too many things and not enough 
distinct names for the things.

But yeah, basically that if the device's "I am associated with an IOMMU" 
data is set, then by construction it must lead to a set of ops which are 
definitely valid. Conceptually it's cleaner than combining two different 
data sources (the per-device info plus the bus ops which may or may not 
be relevant to a given device), even if cosmetically we have to juggle 
through practically every possible permutation of the names "iommu" and 
"device" to get there :/

Robin.