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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> Cc: linux-metag@xxxxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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:
signature.asc
Description: Digital signature
