linux-kernel - Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

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]

Date:   Wed, 7 Sep 2022 09:46:28 +0100
From:   John Garry <john.garry@...wei.com>
To:     Robin Murphy <robin.murphy@....com>,
        Ethan Zhao <haifeng.zhao@...ux.intel.com>, <joro@...tes.org>,
        <will@...nel.org>
CC:     <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
        <linuxarm@...wei.com>
Subject: Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

On 06/09/2022 19:25, Robin Murphy wrote:
>>
>> Caveat: on the chance that the IOVA domain init fails due to the 
>> rcache init failing, then, if there were another device in the group 
>> which probes later, its probe would be ok as the start_pfn is set. Not 
>> Good.
> 
> Yeah, there's a lot not to like about iommu_dma_init_domain() - I've 
> been banking on it all getting cleaned up when I get to refactoring that 
> area of probing (remember the issue you reported years ago with PCI 
> groups being built in the wrong order? All related...), but in fact 
> since the cookie management got pulled into core code, we can probably 
> tie the IOVA domain setup to that right now without much other 
> involvement. That could be a cheap win, so I'll give it a go soon.

ok, great.

On a related topic, another thing to consider is that errors in IOVA 
domain init are not handled gracefully in terms of how we deal with the 
device probe and setting dma mapping ops, ref iommu_setup_dma_ops(). I 
assume you know all this.

> 
>> - vdpa just fails to create the domain in vduse_domain_create()
>>
>>> That makes a fair amount of sense, but does mean that we're missing 
>>> the equivalent in iova_rcache_insert() for it to actually work. Or we 
>>> just remove it and tighten up the documentation to say that's not valid 
>>
>> I'd be more inclined to remove it. I would rather remove fathpath 
>> checks as much as possible and have robust error handling in the 
>> domain init.
>>
>> Afterall I do have the "remove check" craze going.
> 
> Sure, like I say I'm happy to be consistent either way. If I do end up 
> reinstating such a check I think I'd prefer to have it explicit in 
> {alloc,free}_iova_fast() anyway, rather than buried in internal 
> implementation details.

I'm not sure what you would like to see now, if anything.

I could just remove the iovad->rcache check in iova_rcache_get().  It's 
pretty useless (on its own) since we don't have the same check on the 
"insert" path.

Or also add this:

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 0d6d8edf782d..e8f0b8f47f45 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -578,6 +578,12 @@ static int iommu_dma_init_domain(struct 
iommu_domain *domain, dma_addr_t base,
  			goto done_unlock;
  		}

+		if (!iovad->rcaches) {
+			pr_warn("IOVA domain rcache not properly initialised\n");
+			ret = -EFAULT;
+			goto done_unlock;
+		}
+
  		ret = 0;
  		goto done_unlock;


But I figure that you don't want more crud there now, considering the 
work you mention above.

Thanks,
John