Re: [PATCH 1/2] metag/uaccess: Fix access_ok()

[James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4761 bytes --]

Hi Stephen,

On Tue, May 02, 2017 at 10:11:54PM +0100, James Hogan wrote:
> The __user_bad() macro used by access_ok() has a few corner cases
> noticed by Al Viro where it doesn't behave correctly:
> 
>  - The kernel range check has off by 1 errors which permit access to the
>    first and last byte of the kernel mapped range.
> 
>  - The kernel range check ends at LINCORE_BASE rather than
>    META_MEMORY_LIMIT, which is ineffective when the kernel is in global
>    space (an extremely uncommon configuration).
> 
> There are a couple of other shortcomings here too:
> 
>  - Access to the whole of the other address space is permitted (i.e. the
>    global half of the address space when the kernel is in local space).
>    This isn't ideal as it could theoretically still contain privileged
>    mappings set up by the bootloader.
> 
>  - The size argument is unused, permitting user copies which start on
>    valid pages at the end of the user address range and cross the
>    boundary into the kernel address space (e.g. addr = 0x3ffffff0, size
>    > 0x10).
> 
> It isn't very convenient to add size checks when disallowing certain
> regions, and it seems far safer to be sure and explicit about what
> userland is able to access, so invert the logic to allow certain regions
> instead, and fix the off by 1 errors and missing size checks. This also
> allows the get_fs() == KERNEL_DS check to be more easily optimised into
> the user address range case.
> 
> We now have 3 such allowed regions:
> 
>  - The user address range (incorporating the get_fs() == KERNEL_DS
>    check).
> 
>  - NULL (some kernel code expects this to work, and we'll always catch
>    the fault anyway).
> 
>  - The core code memory region.
> 
> Fixes: 373cd784d0fc ("metag: Memory handling")
> Reported-by: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
> Signed-off-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/metag/include/asm/uaccess.h | 40 +++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h
> index 469a2f1393d3..1e5f26d2dce8 100644
> --- a/arch/metag/include/asm/uaccess.h
> +++ b/arch/metag/include/asm/uaccess.h
> @@ -28,24 +28,32 @@
>  
>  #define segment_eq(a, b)	((a).seg == (b).seg)
>  
> -#define __kernel_ok (segment_eq(get_fs(), KERNEL_DS))

FYI this patch, commit 8a8b56638bca ("metag/uaccess: Fix access_ok()"),
just pushed to metag/for-next, will conflict with Al's commit
db68ce10c4f0 ("new helper: uaccess_kernel()") from his vfs/for-next
branch.

This change should supersede Al's metag change, i.e. __kernel_ok should
be removed from this file.

Cheers
James

> -/*
> - * Explicitly allow NULL pointers here. Parts of the kernel such
> - * as readv/writev use access_ok to validate pointers, but want
> - * to allow NULL pointers for various reasons. NULL pointers are
> - * safe to allow through because the first page is not mappable on
> - * Meta.
> - *
> - * We also wish to avoid letting user code access the system area
> - * and the kernel half of the address space.
> - */
> -#define __user_bad(addr, size) (((addr) > 0 && (addr) < META_MEMORY_BASE) || \
> -				((addr) > PAGE_OFFSET &&		\
> -				 (addr) < LINCORE_BASE))
> -
>  static inline int __access_ok(unsigned long addr, unsigned long size)
>  {
> -	return __kernel_ok || !__user_bad(addr, size);
> +	/*
> +	 * Allow access to the user mapped memory area, but not the system area
> +	 * before it. The check extends to the top of the address space when
> +	 * kernel access is allowed (there's no real reason to user copy to the
> +	 * system area in any case).
> +	 */
> +	if (likely(addr >= META_MEMORY_BASE && addr < get_fs().seg &&
> +		   size <= get_fs().seg - addr))
> +		return true;
> +	/*
> +	 * Explicitly allow NULL pointers here. Parts of the kernel such
> +	 * as readv/writev use access_ok to validate pointers, but want
> +	 * to allow NULL pointers for various reasons. NULL pointers are
> +	 * safe to allow through because the first page is not mappable on
> +	 * Meta.
> +	 */
> +	if (!addr)
> +		return true;
> +	/* Allow access to core code memory area... */
> +	if (addr >= LINCORE_CODE_BASE && addr <= LINCORE_CODE_LIMIT &&
> +	    size <= LINCORE_CODE_LIMIT + 1 - addr)
> +		return true;
> +	/* ... but no other areas. */
> +	return false;
>  }
>  
>  #define access_ok(type, addr, size) __access_ok((unsigned long)(addr),	\
> -- 
> git-series 0.8.10

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]