linux-kernel - Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

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: <20191018111603.GD27759@lakrids.cambridge.arm.com>
Date:   Fri, 18 Oct 2019 12:16:03 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Dave Martin <Dave.Martin@....com>
Cc:     Paul Elliott <paul.elliott@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>,
        Amit Kachhap <amit.kachhap@....com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        linux-arch@...r.kernel.org, Eugene Syromiatnikov <esyr@...hat.com>,
        Szabolcs Nagy <szabolcs.nagy@....com>,
        "H.J. Lu" <hjl.tools@...il.com>, Andrew Jones <drjones@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>, Jann Horn <jannh@...gle.com>,
        Richard Henderson <richard.henderson@...aro.org>,
        Kristina Martšenko <kristina.martsenko@....com>,
        Mark Brown <broonie@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-arm-kernel@...ts.infradead.org,
        Florian Weimer <fweimer@...hat.com>,
        linux-kernel@...r.kernel.org, Sudakshina Das <sudi.das@....com>,
        Dave Kleikamp <shaggy@...ux.vnet.ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification
 support

[adding mm folk]

On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> > > +{
> > > +	unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> > > +
> > > +	if (system_supports_bti())
> > > +		supported |= PROT_BTI;
> > > +
> > > +	return (prot & ~supported) == 0;
> > > +}
> > 
> > If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> > with PROT_BIT but !system_supports_bti()?
> > 
> > ... or can that become:
> > 
> > 	return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
> 
> We can reach this via mmap() and friends IIUC.
> 
> Since this function only gets called once-ish per vma I have a weak
> preference for keeping the check here to avoid code fragility.
> 
> 
> It does feel like arch_validate_prot() is supposed to be a generic gate
> for prot flags coming into the kernel via any route though, but only the
> mprotect() path actually uses it.
> 
> This function originally landed in v2.6.27 as part of the powerpc strong
> access ordering support (PROT_SAO):
> 
> b845f313d78e ("mm: Allow architectures to define additional protection bits")
> ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")
> 
> where the mmap() path uses arch_calc_vm_prot_bits() without
> arch_validate_prot(), just as in the current code.  powerpc's original
> arch_calc_vm_prot_bits() does no obvious policing.
> 
> This might be a bug.  I can draft a patch to add it for the mmap() path
> for people to comment on ... I can't figure out yet whether or not the
> difference is intentional or there's some subtlety that I'm missed.

>From reading those two commit messages, it looks like this was an
oversight. I'd expect that we should apply this check for any
user-provided prot (i.e. it should apply to both mprotect and mmap).

Ben, Andrew, does that make sense to you?

... or was there some reason to only do this for mprotect?

Thanks,
Mark.

> mmap( ... prot = -1 ... ) succeeds with effective rwx permissions and no
> apparent ill effects on my random x86 box, but mprotect(..., -1) fails
> with -EINVAL.
> 
> This is at least strange.
> 
> Theoretically, tightening this would be an ABI break, though I'd say
> this behaviour is not intentional.
> 
> Thoughts?
> 
> [...]
> 
> Cheers
> ---Dave