Message-ID: <55C9D768.7060409@imgtec.com>
Date:	Tue, 11 Aug 2015 12:07:20 +0100
From:	James Hogan <james.hogan@...tec.com>
To:	David Miller <davem@...hat.com>
CC:	<linux-kernel@...r.kernel.org>, <linux-arch@...r.kernel.org>,
	<keescook@...omium.org>, <akpm@...ux-foundation.org>,
	<monstr@...str.eu>, <lftan@...era.com>, <jonas@...thpole.se>,
	<chris@...kel.net>, <jcmvbkbc@...il.com>,
	<nios2-dev@...ts.rocketboards.org>, <linux@...ts.openrisc.net>,
	<linux-xtensa@...ux-xtensa.org>
Subject: Re: [PATCH v2 00/11] test_user_copy improvements

Hi David,

On 10/08/15 23:29, David Miller wrote:
> From: James Hogan <james.hogan@...tec.com>
> Date: Fri, 7 Aug 2015 16:21:53 +0100
> 
>> These patches extend the test_user_copy test module to handle lots more
>> cases of user accessors which architectures can override separately, and
>> in particular those which are important for checking the MIPS Enhanced
>> Virtual Addressing (EVA) implementations, which need to handle
>> overlapping user and kernel address spaces, with special instructions
>> for accessing user address space from kernel mode.
>>
>> - Checking that kernel pointers are accepted when user address limit is
>>   set to KERNEL_DS, as done by the kernel when it internally invokes
>>   system calls with kernel pointers.
>> - Checking of the unchecked accessors (which don't call access_ok()).
>>   Some of the tests are special cased for EVA at the moment which has
>>   stricter hardware guarantees for bad user accesses than other
>>   configurations.
>> - Checking of other sets of user accessors, including the inatomic user
>>   copies, clear_user, compatibility accessors (copy_in_user and
>>   _unaligned), the user string accessors, and the user checksum
>>   functions, all of which need special handling in arch code with EVA.
>>
>> Tested on MIPS with and without EVA, and on x86_64.
>>
>> Only build tested for arm, blackfin, metag, microblaze, openrisc,
>> parisc, powerpc, sh, sparc, tile, i386 & xtensa.
>>
>> All arches were audited for the appropriate exports, only score is known
>> to still be missing some.
> 
> James, thanks for doing this work.
> 
> If I understand the MIPS EVA facility correctly, it operates exactly like
> how sparc64 does.  Wherein user and kernel virtual addresses are fully
> segregated, and one must use a specially tagged load or store to access
> user addresses.

Yes, sort of. Roughly speaking, 6 segments in the MIPS virtual address
space become configurable such that each one may be:
* TLB mapped and accessible to user and kernel (both modes must share
TLB mappings in that segment)
* TLB mapped in user mode, but not-TLB-mapped to kernel mode (a window
into physical memory).
* (and various other combinations)

This allows the kernel virtual address space to be extended down to
overlap the user address space and make more physical memory directly
accessible to the kernel, and potentially for the user virtual address
space to extend upwards too.

So if there is any overlap, the EVA load/store instructions must be used
whenever user memory is accessed by kernel.

> This actually creates problems for the tests as currently coded on
> such systems (this problem existed before your changes).  You might
> not be triggering this problem on MIPS EV but it certainly is there.
> 
> For example, consider this test:
> 
> 	ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
> 				    PAGE_SIZE),
> 		    "illegal reversed copy_from_user passed");
> 
> If the 'kmem' access faults, we will try to zero out PAGE_SIZE bytes
> at 'bad_usermem'.  But this is not necessarily going to fail.

Out of interest, is the zeroing a strict requirement for correct use, or
a safety precaution to prevent data leakage in case of bad error checking?

(A quick look reveals that for copy_from_user() when access_ok() fails,
only arm, arm64, frv, m32r, m68k, sparc, tile, x86, and xtensa do this).

> 
> The user address 'bad_usermem', on MIPS EV and sparc64, could just as
> equally happen to be a legitimate kernel address.  So this clear will
> succeed and we will end up clearing memory at an arbitrary kernel
> address.

That's a good point. The reversed tests aren't really safe in that case.
With MIPS EVA the user address is very likely to be a valid
non-TLB-mapped address to kernel mode, and will zero arbitrary memory.
They could also potentially crash the kernel if user memory isn't
normally kernel accessible and the arch doesn't fix up faults for the
kernel accesses (not EVA, but maybe sparc64?).

It is also possible (though less likely) that the kernel address will
have a valid user mapping at the same address, so the reversed
copy_to_user test may well leak arbitrary kernel memory to user memory
without faulting.

> There is no real way to trap this situation as a native load/store
> will work just fine on these addresses.
> 
> I don't have a good suggestion other than to say that these tests
> seem to only be valid in a combined kernel/user address space, ie.
> for systems other than MIPS EV and sparc64.

Yes, although I think the all-kernel ones are still valuable for testing
where it can be assumed that kernel addresses are unlikely to be valid
user addresses (otherwise they may also succeed where they should fail
for similar reasons, but are otherwise harmless).

> Also, I think the tests you added and protected with MIPS ifdefs could
> equally be enabled on sparc64.

Yes, it sounds like it. I'll try the ARCH_SPLIT_VA_SPACE idea.

I have since tested these ones on a different (out of tree) EVA
configuration (legacy segment layout, but EVA instructions enabled) and
it ends up accessing kernel only addresses (not in the overlapping area)
with EVA instructions, which would normally get caught by access_ok() in
the other illegal tests, but are not handled properly by the arch (they
generate address error exception instead of TLB invalid exception). It
sounds like they should just work out of the box on sparc64 though if
they literally use a different ASI (aside from the risk of false positives).

Thanks for the feedback!

Cheers
James


Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists