linux-kernel - Re: [PATCH v8 03/12] iommu/exynos: fix page table maintenance

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] [day] [month] [year] [list]

Date:	Mon, 29 Jul 2013 09:22:01 -0700
From:	Grant Grundler <grundler@...omium.org>
To:	Cho KyongHo <pullip.cho@...sung.com>
Cc:	Grant Grundler <grundler@...omium.org>,
	Linux ARM Kernel <linux-arm-kernel@...ts.infradead.org>,
	Linux IOMMU <iommu@...ts.linux-foundation.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>,
	Hyunwoong Kim <khw0178.kim@...sung.com>,
	Joerg Roedel <joro@...tes.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Prathyush <prathyush.k@...sung.com>,
	Rahul Sharma <rahul.sharma@...sung.com>,
	Subash Patel <supash.ramaswamy@...aro.org>,
	Keyyoung Park <keyyoung.park@...sung.com>,
	Antonios Motakis <a.motakis@...tualopensystems.com>,
	kvmarm@...ts.cs.columbia.edu,
	Sachin Kamat <sachin.kamat@...aro.org>
Subject: Re: [PATCH v8 03/12] iommu/exynos: fix page table maintenance

On Mon, Jul 29, 2013 at 2:18 AM, Cho KyongHo <pullip.cho@...sung.com> wrote:
...
>> > @@ -908,7 +916,7 @@ static int lv2set_page(unsigned long *pent, phys_addr_t paddr, size_t size,
>> >                 int i;
>> >                 for (i = 0; i < SPAGES_PER_LPAGE; i++, pent++) {
>> >                         if (!lv2ent_fault(pent)) {
>> > -                               memset(pent, 0, sizeof(*pent) * i);
>> > +                               clear_page_table(pent - i, i);
>> >                                 return -EADDRINUSE;
>>
>> I am wondering about two issues with this error handling:
>> 1) we don't call pgtable_flush() in this case - I think just for
>> consistency we should - don't rush to add since my next comment is to
>> change this error handling completely.
>>
> clear_page_table() is called for the page table entries that are already
> fault pages. That is why it does not contain cache flush.
>
>> 2) If  -EADDRINUSE is correct, why does the code clear the IO Page
>> table entries?
>>
>>    I think this error path should either
>>    (a) BUG_ON (ie panic) since this is an inconsistency between
>> generic IOMMU page allocation and chip specific IOMMU mapping code OR
>>    (b) WARN_ON, not clear the entries, and hope whoever was using it
>> can finish using the system before crashing or gracefully shutting
>> down.
>>
>> In any case, I'm pretty sure this code needs to change and it should
>> be in a follow up to this series.
>
> Yes, you're right. But I worried the case that a kernel module calls IOMMU API
> functions directly and does not want to make kernel panic when it tries to map
> a region that is already in use.

Using a DMA address for a different physical page while the current
mapping is still active can only be a bug. I can confidently say there
is no way to map the same DMA address twice (at least not for a single
page table.)  We can try to fail the mapping somehow and WARN_ON to
indicate we had a "Re-Use before free" type bug.

> I also wonder if the such kernel module exists.

I believe the kernel will never do this.

> WARN_ON is also a good idea.

After this series goes in, post another patch and I'd be happy to
review that as well.

After thinking about it more, I'm also ok with removing this code.
It's a very "defensive" code to catch errors in the generic IOMMU code
that probably no longer exist. Or maybe just make it
"CONFIG_DEBUG_IOMMU_ALLOC" or something like that.

cheers,
grant
--
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/