linux-kernel - Re: [PATCH v5 3/8] arm: fixmap: implement __set

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: <20140908191634.GV5598@outflux.net>
Date:	Mon, 8 Sep 2014 12:16:34 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Will Deacon <will.deacon@....com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rabin Vincent <rabin@....in>,
	Laura Abbott <lauraa@...eaurora.org>,
	Rob Herring <robh@...nel.org>,
	Leif Lindholm <leif.lindholm@...aro.org>,
	"msalter@...hat.com" <msalter@...hat.com>,
	Liu hua <sdu.liu@...wei.com>,
	Nikolay Borisov <Nikolay.Borisov@....com>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Doug Anderson <dianders@...gle.com>,
	Jason Wessel <jason.wessel@...driver.com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 3/8] arm: fixmap: implement __set_fixmap()

On Thu, Sep 04, 2014 at 06:27:48PM +0100, Will Deacon wrote:
> On Thu, Sep 04, 2014 at 06:23:42PM +0100, Kees Cook wrote:
> > On Thu, Sep 4, 2014 at 10:03 AM, Will Deacon <will.deacon@....com> wrote:
> > > Hi Kees,
> > >
> > > On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote:
> > >> This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h.
> > >> Also makes sure that the fixmap allocation fits into the expected range.
> > >>
> > >> Based on patch by Rabin Vincent.
> > >
> > > [...]
> > >
> > >> +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
> > >> +{
> > >> +     unsigned long vaddr = __fix_to_virt(idx);
> > >> +     pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr);
> > >> +
> > >> +     /* Make sure fixmap region does not exceed available allocation. */
> > >> +     BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) >
> > >> +                  FIXADDR_END);
> > >> +     BUG_ON(idx >= __end_of_fixed_addresses);
> > >> +
> > >> +     if (pgprot_val(prot))
> > >> +             set_pte_at(NULL, vaddr, pte,
> > >> +                     pfn_pte(phys >> PAGE_SHIFT, prot));
> > >> +     else
> > >> +             pte_clear(NULL, vaddr, pte);
> > >> +
> > >> +     /*
> > >> +      * Given the potential a15 tlbi errata, we can only do tlb flushes
> > >> +      * with interrupts disabled. Callers must have taken care of this.
> > >> +      */
> > >> +     WARN_ON_ONCE(!irqs_disabled());
> > >> +     flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> > >
> > > Aha, this explains why we were confusing each other! The issue is that
> > > interrupts must be *enabled*, so this code does the exact opposite of
> > > what we need.
> > >
> > > I think this got lost in a sea of double negatives during the last round
> > > of review.
> > 
> > Ah! If this is the case, perhaps we can get away with
> > local_flush_tlb_kernel_range() then?
> 
> That's a bit tricky, since you need to ensure that preemption is disabled
> until the mapping is put back like it was.

Okay, under both real hardware with the errata, and under QEMU, things seem
to work with this change to the series. What do you think?

Thanks!

-Kees

diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
index b050ed7..8558d6b 100644
--- a/arch/arm/kernel/patch.c
+++ b/arch/arm/kernel/patch.c
@@ -37,6 +37,7 @@ void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
 	else
 		__acquire(&patch_lock);
 
+	preempt_disable();
 	set_fixmap(fixmap, page_to_phys(page));
 
 	return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
@@ -46,6 +47,7 @@ void __kprobes patch_unmap(int fixmap, unsigned long *flags)
 	__releases(&patch_lock)
 {
 	clear_fixmap(fixmap);
+	preempt_enable();
 
 	if (flags)
 		spin_unlock_irqrestore(&patch_lock, *flags);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index fab8583..efaf74c 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -368,7 +368,7 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
 			pfn_pte(phys >> PAGE_SHIFT, prot));
 	else
 		pte_clear(NULL, vaddr, pte);
-	flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
+	local_flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
 }
 
 /*

-- 
Kees Cook
Chrome OS Security
--
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/