| Message ID | 20170323203419.GA62859@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 23, 2017 at 01:34:19PM -0700, Kees Cook wrote: > This adds CORRUPT_USER_DS to check that the get_fs() test on syscall return > still sees USER_DS during the new VERIFY_PRE_USERMODE_STATE checks. > > Signed-off-by: Kees Cook <keescook@chromium.org> ... > +void lkdtm_CORRUPT_USER_DS(void) > +{ > + /* > + * Test that USER_DS has been set correctly on exiting a syscall. > + * Since setting this higher than USER_DS (TASK_SIZE) would introduce > + * an exploitable condition, we lower it instead, since that should > + * not create as large a problem on an unprotected system. > + */ > + mm_segment_t lowfs; > +#ifdef MAKE_MM_SEG > + lowfs = MAKE_MM_SEG(TASK_SIZE - PAGE_SIZE); > +#else > + lowfs = TASK_SIZE - PAGE_SIZE; > +#endif > + > + pr_info("setting bad task size limit\n"); > + set_fs(lowfs); > +} This won't work on architectures where the set_fs() argument does not contain an address but an address space identifier. This is true e.g. for s390 and as far as I know also for sparc. On s390 we have complete distinct address spaces for kernel and user space that each start at address zero.
On Fri, Mar 24, 2017 at 1:14 AM, Heiko Carstens <heiko.carstens@de.ibm.com> wrote: > On Thu, Mar 23, 2017 at 01:34:19PM -0700, Kees Cook wrote: >> This adds CORRUPT_USER_DS to check that the get_fs() test on syscall return >> still sees USER_DS during the new VERIFY_PRE_USERMODE_STATE checks. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> > > ... > >> +void lkdtm_CORRUPT_USER_DS(void) >> +{ >> + /* >> + * Test that USER_DS has been set correctly on exiting a syscall. >> + * Since setting this higher than USER_DS (TASK_SIZE) would introduce >> + * an exploitable condition, we lower it instead, since that should >> + * not create as large a problem on an unprotected system. >> + */ >> + mm_segment_t lowfs; >> +#ifdef MAKE_MM_SEG >> + lowfs = MAKE_MM_SEG(TASK_SIZE - PAGE_SIZE); >> +#else >> + lowfs = TASK_SIZE - PAGE_SIZE; >> +#endif >> + >> + pr_info("setting bad task size limit\n"); >> + set_fs(lowfs); >> +} > > This won't work on architectures where the set_fs() argument does not > contain an address but an address space identifier. This is true e.g. for > s390 and as far as I know also for sparc. > On s390 we have complete distinct address spaces for kernel and user space > that each start at address zero. > The patch that enforce USER_DS is disabled on s390 anyway. I guess, we can just do a set_fs(KERNEL_DS) for the others.
On 03/24/2017 04:17 PM, Thomas Garnier wrote: > On Fri, Mar 24, 2017 at 1:14 AM, Heiko Carstens > <heiko.carstens@de.ibm.com> wrote: >> On Thu, Mar 23, 2017 at 01:34:19PM -0700, Kees Cook wrote: >>> This adds CORRUPT_USER_DS to check that the get_fs() test on syscall return >>> still sees USER_DS during the new VERIFY_PRE_USERMODE_STATE checks. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >> >> ... >> >>> +void lkdtm_CORRUPT_USER_DS(void) >>> +{ >>> + /* >>> + * Test that USER_DS has been set correctly on exiting a syscall. >>> + * Since setting this higher than USER_DS (TASK_SIZE) would introduce >>> + * an exploitable condition, we lower it instead, since that should >>> + * not create as large a problem on an unprotected system. >>> + */ >>> + mm_segment_t lowfs; >>> +#ifdef MAKE_MM_SEG >>> + lowfs = MAKE_MM_SEG(TASK_SIZE - PAGE_SIZE); >>> +#else >>> + lowfs = TASK_SIZE - PAGE_SIZE; >>> +#endif >>> + >>> + pr_info("setting bad task size limit\n"); >>> + set_fs(lowfs); >>> +} >> >> This won't work on architectures where the set_fs() argument does not >> contain an address but an address space identifier. This is true e.g. for >> s390 and as far as I know also for sparc. >> On s390 we have complete distinct address spaces for kernel and user space >> that each start at address zero. >> > > The patch that enforce USER_DS is disabled on s390 anyway. I guess, we > can just do a set_fs(KERNEL_DS) for the others. that would enable the test, but it would also mean that lkdtm can be used by a program to escalate its rights. I think that is the reason why Kees did this lowfs things.
On Fri, Mar 24, 2017 at 8:24 AM, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 03/24/2017 04:17 PM, Thomas Garnier wrote: >> On Fri, Mar 24, 2017 at 1:14 AM, Heiko Carstens >> <heiko.carstens@de.ibm.com> wrote: >>> On Thu, Mar 23, 2017 at 01:34:19PM -0700, Kees Cook wrote: >>>> This adds CORRUPT_USER_DS to check that the get_fs() test on syscall return >>>> still sees USER_DS during the new VERIFY_PRE_USERMODE_STATE checks. >>>> >>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> >>> ... >>> >>>> +void lkdtm_CORRUPT_USER_DS(void) >>>> +{ >>>> + /* >>>> + * Test that USER_DS has been set correctly on exiting a syscall. >>>> + * Since setting this higher than USER_DS (TASK_SIZE) would introduce >>>> + * an exploitable condition, we lower it instead, since that should >>>> + * not create as large a problem on an unprotected system. >>>> + */ >>>> + mm_segment_t lowfs; >>>> +#ifdef MAKE_MM_SEG >>>> + lowfs = MAKE_MM_SEG(TASK_SIZE - PAGE_SIZE); >>>> +#else >>>> + lowfs = TASK_SIZE - PAGE_SIZE; >>>> +#endif >>>> + >>>> + pr_info("setting bad task size limit\n"); >>>> + set_fs(lowfs); >>>> +} >>> >>> This won't work on architectures where the set_fs() argument does not >>> contain an address but an address space identifier. This is true e.g. for >>> s390 and as far as I know also for sparc. >>> On s390 we have complete distinct address spaces for kernel and user space >>> that each start at address zero. >>> >> >> The patch that enforce USER_DS is disabled on s390 anyway. I guess, we >> can just do a set_fs(KERNEL_DS) for the others. > > that would enable the test, but it would also mean that lkdtm can be used by > a program to escalate its rights. I think that is the reason why Kees did this > lowfs things. > I see, I need to look at how lkdtm works exactly.
On Fri, Mar 24, 2017 at 8:24 AM, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 03/24/2017 04:17 PM, Thomas Garnier wrote: >> On Fri, Mar 24, 2017 at 1:14 AM, Heiko Carstens >> <heiko.carstens@de.ibm.com> wrote: >>> On Thu, Mar 23, 2017 at 01:34:19PM -0700, Kees Cook wrote: >>>> This adds CORRUPT_USER_DS to check that the get_fs() test on syscall return >>>> still sees USER_DS during the new VERIFY_PRE_USERMODE_STATE checks. >>>> >>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> >>> ... >>> >>>> +void lkdtm_CORRUPT_USER_DS(void) >>>> +{ >>>> + /* >>>> + * Test that USER_DS has been set correctly on exiting a syscall. >>>> + * Since setting this higher than USER_DS (TASK_SIZE) would introduce >>>> + * an exploitable condition, we lower it instead, since that should >>>> + * not create as large a problem on an unprotected system. >>>> + */ >>>> + mm_segment_t lowfs; >>>> +#ifdef MAKE_MM_SEG >>>> + lowfs = MAKE_MM_SEG(TASK_SIZE - PAGE_SIZE); >>>> +#else >>>> + lowfs = TASK_SIZE - PAGE_SIZE; >>>> +#endif >>>> + >>>> + pr_info("setting bad task size limit\n"); >>>> + set_fs(lowfs); >>>> +} >>> >>> This won't work on architectures where the set_fs() argument does not >>> contain an address but an address space identifier. This is true e.g. for >>> s390 and as far as I know also for sparc. >>> On s390 we have complete distinct address spaces for kernel and user space >>> that each start at address zero. >>> >> >> The patch that enforce USER_DS is disabled on s390 anyway. I guess, we >> can just do a set_fs(KERNEL_DS) for the others. > > that would enable the test, but it would also mean that lkdtm can be used by > a program to escalate its rights. I think that is the reason why Kees did this > lowfs things. Yeah, but it seems like getting this right for all architectures isn't sane. I'm going to change it to use KERNEL_DS but also post a SIGKILL to the process. That way it'll still trigger the syscall return checking, but will be unable to continue running with a potentially uncaught KERNEL_DS. -Kees
diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h index 67d27be60405..3b4976396ec4 100644 --- a/drivers/misc/lkdtm.h +++ b/drivers/misc/lkdtm.h @@ -27,6 +27,7 @@ void lkdtm_REFCOUNT_ZERO_SUB(void); void lkdtm_REFCOUNT_ZERO_ADD(void); void lkdtm_CORRUPT_LIST_ADD(void); void lkdtm_CORRUPT_LIST_DEL(void); +void lkdtm_CORRUPT_USER_DS(void); /* lkdtm_heap.c */ void lkdtm_OVERWRITE_ALLOCATION(void); diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c index e3f4cd8876b5..4906e53a6df3 100644 --- a/drivers/misc/lkdtm_bugs.c +++ b/drivers/misc/lkdtm_bugs.c @@ -8,6 +8,7 @@ #include <linux/list.h> #include <linux/refcount.h> #include <linux/sched.h> +#include <linux/uaccess.h> struct lkdtm_list { struct list_head node; @@ -279,3 +280,22 @@ void lkdtm_CORRUPT_LIST_DEL(void) else pr_err("list_del() corruption not detected!\n"); } + +void lkdtm_CORRUPT_USER_DS(void) +{ + /* + * Test that USER_DS has been set correctly on exiting a syscall. + * Since setting this higher than USER_DS (TASK_SIZE) would introduce + * an exploitable condition, we lower it instead, since that should + * not create as large a problem on an unprotected system. + */ + mm_segment_t lowfs; +#ifdef MAKE_MM_SEG + lowfs = MAKE_MM_SEG(TASK_SIZE - PAGE_SIZE); +#else + lowfs = TASK_SIZE - PAGE_SIZE; +#endif + + pr_info("setting bad task size limit\n"); + set_fs(lowfs); +} diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c index b9a4cd4a9b68..42d2b8e31e6b 100644 --- a/drivers/misc/lkdtm_core.c +++ b/drivers/misc/lkdtm_core.c @@ -199,6 +199,7 @@ struct crashtype crashtypes[] = { CRASHTYPE(OVERFLOW), CRASHTYPE(CORRUPT_LIST_ADD), CRASHTYPE(CORRUPT_LIST_DEL), + CRASHTYPE(CORRUPT_USER_DS), CRASHTYPE(CORRUPT_STACK), CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE), CRASHTYPE(OVERWRITE_ALLOCATION),
This adds CORRUPT_USER_DS to check that the get_fs() test on syscall return still sees USER_DS during the new VERIFY_PRE_USERMODE_STATE checks. Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/misc/lkdtm.h | 1 + drivers/misc/lkdtm_bugs.c | 20 ++++++++++++++++++++ drivers/misc/lkdtm_core.c | 1 + 3 files changed, 22 insertions(+)