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  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]
Date:   Wed, 25 Nov 2020 11:35:46 +0000
From:   Will Deacon <will@...nel.org>
To:     Yang Yingliang <yangyingliang@...wei.com>
Cc:     Lu Baolu <baolu.lu@...ux.intel.com>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu: fix return error code in iommu_probe_device()

On Wed, Nov 25, 2020 at 09:54:34AM +0800, Yang Yingliang wrote:
> 
> On 2020/11/18 6:41, Will Deacon wrote:
> > On Tue, Nov 17, 2020 at 07:11:28PM +0800, Yang Yingliang wrote:
> > > On 2020/11/17 17:40, Lu Baolu wrote:
> > > > On 2020/11/17 10:52, Yang Yingliang wrote:
> > > > > If iommu_group_get() failed, it need return error code
> > > > > in iommu_probe_device().
> > > > > 
> > > > > Fixes: cf193888bfbd ("iommu: Move new probe_device path...")
> > > > > Reported-by: Hulk Robot <hulkci@...wei.com>
> > > > > Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> > > > > ---
> > > > >    drivers/iommu/iommu.c | 4 +++-
> > > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > index b53446bb8c6b..6f4a32df90f6 100644
> > > > > --- a/drivers/iommu/iommu.c
> > > > > +++ b/drivers/iommu/iommu.c
> > > > > @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev)
> > > > >            goto err_out;
> > > > >          group = iommu_group_get(dev);
> > > > > -    if (!group)
> > > > > +    if (!group) {
> > > > > +        ret = -ENODEV;
> > > > Can you please explain why you use -ENODEV here?
> > > Before 79659190ee97 ("iommu: Don't take group reference in
> > > iommu_alloc_default_domain()"), in
> > > 
> > > iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV.
> > Hmm. While I think the patch is ok, I'm not sure it qualifies as a fix.
> > Has iommu_probe_device() ever propagated this error? The commit you
> > identify in the 'Fixes:' tag doesn't seem to change this afaict.
> 
> I think after this commit 439945e74a4b ("iommu: Move default domain
> allocation to iommu_probe_device()"),

That SHA doesn't exist in my tree (maybe you mean 6e1aa2049154?). But even
then, I'm not sure 6e1aa2049154 is actually broken if you look at the
interaction with group creation in __iommu_probe_device().

In fact, isn't that the case in mainline too? If __iommu_probe_device()
returns 0, then we _know_ a group exists and so iommu_group_get() will
succeed. I'm still happy with the patch in case this changes in future,
but it doesn't appear to be fixing anything. Do you agree?

Will

Powered by blists - more mailing lists