Re: [PATCH v3 1/4] arm64: compat: Alloc separate pages for vectors and sigpage

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Fri, Apr 12, 2019 at 12:55:07PM +0100, Will Deacon wrote:
> On Fri, Apr 12, 2019 at 12:08:30PM +0100, Vincenzo Frascino wrote:
> > On 10/04/2019 19:10, Will Deacon wrote:
> > > On Tue, Apr 02, 2019 at 05:27:54PM +0100, Vincenzo Frascino wrote:
> > >> +static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
> > >>  {
> > >> -	struct mm_struct *mm = current->mm;
> > >> -	unsigned long addr = AARCH32_VECTORS_BASE;
> > >> -	static const struct vm_special_mapping spec = {
> > >> -		.name	= "[vectors]",
> > >> -		.pages	= vectors_page,
> > >> +	void *ret;
> > >> +
> > >> +	/* The kuser helpers must be mapped at the ABI-defined high address */
> > >> +	ret = _install_special_mapping(mm, AARCH32_KUSER_BASE, PAGE_SIZE,
> > >> +				       VM_READ | VM_EXEC |
> > >> +				       VM_MAYREAD | VM_MAYEXEC,
> > > 
> > > How come you don't need VM_MAYWRITE here...
> > > 
> > 
> > This is to keep it consistent with what the arm (32 bit) implementation does.
> > 
> > My understanding is that the kuser code is executed in user mode for efficiency
> > reasons but it is too close to the kernel to be implemented in user libraries
> > and that the kernel can change its internal implementation from version to
> > version as far as it guarantees the "interface" (entry points and results).
> > Based on this gdb should not need to put a breakpoint inside the kuser helpers code.

That is not the point at all.  The kuser code is to give userspace
independence of two things:

1. The CPU architecture it is running on (whether it be SMP or UP.)
2. The configuration of the kernel.
3. The method with which the per-thread value is obtained.
4. Atomic compare-exchange.

If it was only (1), then maybe it would be possible to have had the
userspace dynamic linker figure out which libraries to load, but (2),
(3) and (4) make that extremely complex to manage.

For (3), there are two ways to get the thread value:

1. Reading it from the userspace visible thread-private CPU register.
   This is fine for CPUs that implement it, but not all ARM CPUs
   implement this register.

2. Reading it from 0xffff0ffc, but only when the kernel is configured
   to write it there.

Which one works is a function of the kernel configuration (hence 2)
and the CPU capabilities.

For (4), there is a need to provide modern libraries with a way to
implement the cmpxchg() semantics.  This is easy in ARMv6+, as there
are the load-exclusive and store-exclusive instructions, but on
earlier architectures, there is only one atomic instruction which
is essentially an exchange operation - and that can't be used to
provide cmpxchg() semantics.

To work around that, there is code in the kuser page which provides
cmpxchg() semantics with the help of the kernel fixing things up if
an exception happens while userspace is executing that code.

Pushing that code into a library means that the kernel has to be aware,
whenever _any_ exception occurs, whether the PC is in that code, and
dealing with that efficiently if it isn't at a fixed address becomes
problematical - it's overhead incurred on almost every exception.
What's more is that the kernel has to be aware of the exact code so
it knows which range of PC values are required to be fixed up - since
the fixup involves changing the userland state, inappropriately changing
it will corrupt the program execution.

> Hmm, but couldn't you apply the same reasoning to the sigpage?

The sigpage is mapped in the userspace address range, and gdb has the
expectation that it can set breakpoints (which involves writing via
ptrace) to pages.  If we can't set breakpoints in the sigpage or vdso,
we end up losing control of the executable while trying to single step
it if the executable branches into such places - or gdb refuses to
step.

> [also, talking to Russell, he makes the very good point that you can't CoW
>  the page containing the vectors because that would give userspace control
>  over the vectors themselves!]

Yes, the vectors page is very special and gdb knows about it - it can't
be CoW'd because that would give userspace a way to modify the machine
vectors, thereby taking control of the machine in a privileged execution
state.

> > And if we consider as well that the fixed address nature of the helpers could be
> > used from ROP authors during the creation of exploits probably we want to
> > prevent gdb to set a breakpoint there hence the proposed patch does not contain
> > VM_MAYWRITE.
> 
> I'm not sure I buy the ROP angle... you need to GUP the thing to get the
> write to happen. Maybe you could do it with a futex or something, but I'm
> also not sure that's really a viable attack.

This came up on 32-bit.  Yes, ROP is a concern, that's why we changed
things around a bit a few years ago, and also made it possible to
disable the kuser helpers page entirely for maximum security - but
doing so has been biting people.  Last merge window, I merged a patch
which made the kernel state when such an executable was run, because
people were complaining that it was difficult to work out what was
happening.

ROP here is not about changing the data in the page, but exploiting the
instructions already there.  In the old says, most of the page was
filled with zeros, which meant you could branch before a kuser helper
and we'd execute "andeq r0, r0, r0" instructions until we hit one.
One of the changes to improve security was to fill the page with
instructions guaranteed to fault in either ARM or Thumb mode, thereby
reducing the possibility of ROP based attack on systems where the kuser
page was still present.

Another change that was made was to move the vector stubs out of the
vectors page into a page at 0xffff1000 which was inaccessible to the
user, so the only instructions userspace can see are the branches in
each of the vectors to the inaccessible code at 0xffff1000.  This also
has the effect of hiding the address of the kernel entry points from
userspace.

> > I had a look to arm implementation and it seems the it defines the vector page
> > as READONLY_EXEC and there is no VM_MAYWRITE in the vm_flags.
> > 
> > I could extend the comment accordingly.
> 
> I initially thought that the gate VMA would mean that VM_MAYWRITE was
> implicit for GUP, but actually that's handled explicitly in get_gate_page():
> 
> 	/* user gate pages are read-only */
> 	if (gup_flags & FOLL_WRITE)
> 		return -EFAULT;
> 
> so yes, I agree with you that this is consistent with 32-bit. What I'm not
> sure about is why we need to CoW the sigpage. But I suppose being compatible
> with 32-bit is the aim of the game, so this is all moot.

On 32-bit ARM, the "gate VMA" describes the vectors page mapping, not
the sigpage.  The sigpage is CoW-able to support gdb.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel