* [PATCH 0/2] metag/uaccess: Some more user access fixes @ 2017-05-02 21:11 ` James Hogan 0 siblings, 0 replies; 8+ messages in thread From: James Hogan @ 2017-05-02 21:11 UTC (permalink / raw) To: linux-metag; +Cc: Al Viro, James Hogan, stable Here are a couple more user access fixes for metag. The first fixes access_ok() which was flawed in a number of ways (including ignoring the size of the copy and so allowing a copy crossing the user/kernel boundary). The second adds missing access_ok() checking to strncpy_from_user(). Thanks to Al for spotting some of the issues. Both are tagged for stable. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org James Hogan (2): metag/uaccess: Fix access_ok() metag/uaccess: Check access_ok in strncpy_from_user arch/metag/include/asm/uaccess.h | 49 ++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 18 deletions(-) -- git-series 0.8.10 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 0/2] metag/uaccess: Some more user access fixes @ 2017-05-02 21:11 ` James Hogan 0 siblings, 0 replies; 8+ messages in thread From: James Hogan @ 2017-05-02 21:11 UTC (permalink / raw) To: linux-metag; +Cc: Al Viro, James Hogan, stable Here are a couple more user access fixes for metag. The first fixes access_ok() which was flawed in a number of ways (including ignoring the size of the copy and so allowing a copy crossing the user/kernel boundary). The second adds missing access_ok() checking to strncpy_from_user(). Thanks to Al for spotting some of the issues. Both are tagged for stable. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org James Hogan (2): metag/uaccess: Fix access_ok() metag/uaccess: Check access_ok in strncpy_from_user arch/metag/include/asm/uaccess.h | 49 ++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 18 deletions(-) -- git-series 0.8.10 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] metag/uaccess: Fix access_ok() @ 2017-05-02 21:11 ` James Hogan 0 siblings, 0 replies; 8+ messages in thread From: James Hogan @ 2017-05-02 21:11 UTC (permalink / raw) To: linux-metag; +Cc: Al Viro, James Hogan, stable 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@zeniv.linux.org.uk> Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.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)) -/* - * 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/2] metag/uaccess: Fix access_ok() @ 2017-05-02 21:11 ` James Hogan 0 siblings, 0 replies; 8+ messages in thread From: James Hogan @ 2017-05-02 21:11 UTC (permalink / raw) To: linux-metag-u79uwXL29TY76Z2rM5mHXA Cc: Al Viro, James Hogan, stable-u79uwXL29TY76Z2rM5mHXA 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)) -/* - * 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <8a8b56638bcac4e64cccc88bf95a0f9f4b19a2fb.1493759289.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 1/2] metag/uaccess: Fix access_ok() [not found] ` <8a8b56638bcac4e64cccc88bf95a0f9f4b19a2fb.1493759289.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2017-05-02 21:26 ` James Hogan [not found] ` <20170502212605.GO1105-4bYivNCBEGTR3KXKvIWQxtm+Uo4AYnCiHZ5vskTnxNA@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: James Hogan @ 2017-05-02 21:26 UTC (permalink / raw) To: Stephen Rothwell; +Cc: Al Viro, linux-metag-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20170502212605.GO1105-4bYivNCBEGTR3KXKvIWQxtm+Uo4AYnCiHZ5vskTnxNA@public.gmane.org>]
* Re: [PATCH 1/2] metag/uaccess: Fix access_ok() [not found] ` <20170502212605.GO1105-4bYivNCBEGTR3KXKvIWQxtm+Uo4AYnCiHZ5vskTnxNA@public.gmane.org> @ 2017-05-02 21:36 ` Stephen Rothwell 0 siblings, 0 replies; 8+ messages in thread From: Stephen Rothwell @ 2017-05-02 21:36 UTC (permalink / raw) To: James Hogan; +Cc: Al Viro, linux-metag-u79uwXL29TY76Z2rM5mHXA Hi James, On Tue, 2 May 2017 22:26:05 +0100 James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote: > > > 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. Thanks for letting me know. -- Cheers, Stephen Rothwell ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] metag/uaccess: Check access_ok in strncpy_from_user @ 2017-05-02 21:11 ` James Hogan 0 siblings, 0 replies; 8+ messages in thread From: James Hogan @ 2017-05-02 21:11 UTC (permalink / raw) To: linux-metag; +Cc: Al Viro, James Hogan, stable The metag implementation of strncpy_from_user() doesn't validate the src pointer, which could allow reading of arbitrary kernel memory. Add a short access_ok() check to prevent that. Its still possible for it to read across the user/kernel boundary, but it will invariably reach a NUL character after only 9 bytes, leaking only a static kernel address being loaded into D0Re0 at the beginning of __start, which is acceptable for the immediate fix. Reported-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org --- arch/metag/include/asm/uaccess.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h index 1e5f26d2dce8..500f1be6e0fe 100644 --- a/arch/metag/include/asm/uaccess.h +++ b/arch/metag/include/asm/uaccess.h @@ -199,8 +199,13 @@ do { \ extern long __must_check __strncpy_from_user(char *dst, const char __user *src, long count); -#define strncpy_from_user(dst, src, count) __strncpy_from_user(dst, src, count) - +static inline long +strncpy_from_user(char *dst, const char __user *src, long count) +{ + if (!access_ok(VERIFY_READ, src, 1)) + return -EFAULT; + return __strncpy_from_user(dst, src, count); +} /* * Return the size of a string (including the ending 0) * -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] metag/uaccess: Check access_ok in strncpy_from_user @ 2017-05-02 21:11 ` James Hogan 0 siblings, 0 replies; 8+ messages in thread From: James Hogan @ 2017-05-02 21:11 UTC (permalink / raw) To: linux-metag-u79uwXL29TY76Z2rM5mHXA Cc: Al Viro, James Hogan, stable-u79uwXL29TY76Z2rM5mHXA The metag implementation of strncpy_from_user() doesn't validate the src pointer, which could allow reading of arbitrary kernel memory. Add a short access_ok() check to prevent that. Its still possible for it to read across the user/kernel boundary, but it will invariably reach a NUL character after only 9 bytes, leaking only a static kernel address being loaded into D0Re0 at the beginning of __start, which is acceptable for the immediate fix. 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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h index 1e5f26d2dce8..500f1be6e0fe 100644 --- a/arch/metag/include/asm/uaccess.h +++ b/arch/metag/include/asm/uaccess.h @@ -199,8 +199,13 @@ do { \ extern long __must_check __strncpy_from_user(char *dst, const char __user *src, long count); -#define strncpy_from_user(dst, src, count) __strncpy_from_user(dst, src, count) - +static inline long +strncpy_from_user(char *dst, const char __user *src, long count) +{ + if (!access_ok(VERIFY_READ, src, 1)) + return -EFAULT; + return __strncpy_from_user(dst, src, count); +} /* * Return the size of a string (including the ending 0) * -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-05-02 21:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-02 21:11 [PATCH 0/2] metag/uaccess: Some more user access fixes James Hogan
2017-05-02 21:11 ` James Hogan
2017-05-02 21:11 ` [PATCH 1/2] metag/uaccess: Fix access_ok() James Hogan
2017-05-02 21:11 ` James Hogan
[not found] ` <8a8b56638bcac4e64cccc88bf95a0f9f4b19a2fb.1493759289.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2017-05-02 21:26 ` James Hogan
[not found] ` <20170502212605.GO1105-4bYivNCBEGTR3KXKvIWQxtm+Uo4AYnCiHZ5vskTnxNA@public.gmane.org>
2017-05-02 21:36 ` Stephen Rothwell
2017-05-02 21:11 ` [PATCH 2/2] metag/uaccess: Check access_ok in strncpy_from_user James Hogan
2017-05-02 21:11 ` James Hogan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.