* [PATCH] lkdtm: add bad USER_DS test
@ 2017-03-23 20:34 Kees Cook
2017-03-24 8:14 ` Heiko Carstens
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2017-03-23 20:34 UTC (permalink / raw)
To: Thomas Garnier
Cc: Martin Schwidefsky, Heiko Carstens, David Howells, Arnd Bergmann,
Dave Hansen, Al Viro, Thomas Gleixner, René Nyffenegger,
Andrew Morton, Paul E . McKenney, Ingo Molnar, Oleg Nesterov,
Pavel Tikhomirov, Stephen Smalley, Ingo Molnar, H . Peter Anvin,
Andy Lutomirski, Paolo Bonzini, Rik van Riel
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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
drivers/misc/lkdtm.h | 1 +
drivers/misc/lkdtm_bugs.c | 20 ++++++++++++++++++++
drivers/misc/lkdtm_core.c | 1 +
3 files changed, 22 insertions(+)
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),
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] lkdtm: add bad USER_DS test
2017-03-23 20:34 [PATCH] lkdtm: add bad USER_DS test Kees Cook
@ 2017-03-24 8:14 ` Heiko Carstens
2017-03-24 15:17 ` Thomas Garnier
0 siblings, 1 reply; 6+ messages in thread
From: Heiko Carstens @ 2017-03-24 8:14 UTC (permalink / raw)
To: Kees Cook
Cc: Thomas Garnier, Martin Schwidefsky, David Howells, Arnd Bergmann,
Dave Hansen, Al Viro, Thomas Gleixner, René Nyffenegger,
Andrew Morton, Paul E . McKenney, Ingo Molnar, Oleg Nesterov,
Pavel Tikhomirov, Stephen Smalley, Ingo Molnar, H . Peter Anvin,
Andy Lutomirski, Paolo Bonzini, Rik van Riel, Josh Poimboeuf
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lkdtm: add bad USER_DS test
2017-03-24 8:14 ` Heiko Carstens
@ 2017-03-24 15:17 ` Thomas Garnier
2017-03-24 15:24 ` Christian Borntraeger
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Garnier @ 2017-03-24 15:17 UTC (permalink / raw)
To: Heiko Carstens
Cc: Kees Cook, Martin Schwidefsky, David Howells, Arnd Bergmann,
Dave Hansen, Al Viro, Thomas Gleixner, René Nyffenegger,
Andrew Morton, Paul E . McKenney, Ingo Molnar, Oleg Nesterov,
Pavel Tikhomirov, Stephen Smalley, Ingo Molnar, H . Peter Anvin,
Andy Lutomirski, Paolo Bonzini, Rik van Riel
On Fri, Mar 24, 2017 at 1:14 AM, Heiko Carstens
<heiko.carstens-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org> 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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.
--
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lkdtm: add bad USER_DS test
2017-03-24 15:17 ` Thomas Garnier
@ 2017-03-24 15:24 ` Christian Borntraeger
2017-03-24 16:11 ` Thomas Garnier
2017-03-24 17:46 ` Kees Cook
0 siblings, 2 replies; 6+ messages in thread
From: Christian Borntraeger @ 2017-03-24 15:24 UTC (permalink / raw)
To: Thomas Garnier, Heiko Carstens
Cc: Kees Cook, Martin Schwidefsky, David Howells, Arnd Bergmann,
Dave Hansen, Al Viro, Thomas Gleixner, René Nyffenegger,
Andrew Morton, Paul E . McKenney, Ingo Molnar, Oleg Nesterov,
Pavel Tikhomirov, Stephen Smalley, Ingo Molnar, H . Peter Anvin,
Andy Lutomirski, Paolo Bonzini, Rik van Riel, Josh Poimboeuf,
Borislav Petkov
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lkdtm: add bad USER_DS test
2017-03-24 15:24 ` Christian Borntraeger
@ 2017-03-24 16:11 ` Thomas Garnier
2017-03-24 17:46 ` Kees Cook
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Garnier @ 2017-03-24 16:11 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Heiko Carstens, Kees Cook, Martin Schwidefsky, David Howells,
Arnd Bergmann, Dave Hansen, Al Viro, Thomas Gleixner,
René Nyffenegger, Andrew Morton, Paul E . McKenney,
Ingo Molnar, Oleg Nesterov, Pavel Tikhomirov, Stephen Smalley,
Ingo Molnar, H . Peter Anvin, Andy Lutomirski, Paolo Bonzini,
Rik van Riel, Josh Poimboeuf, Borislav Petkov, Brian Gerst
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.
--
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lkdtm: add bad USER_DS test
2017-03-24 15:24 ` Christian Borntraeger
2017-03-24 16:11 ` Thomas Garnier
@ 2017-03-24 17:46 ` Kees Cook
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2017-03-24 17:46 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Thomas Garnier, Heiko Carstens, Martin Schwidefsky, David Howells,
Arnd Bergmann, Dave Hansen, Al Viro, Thomas Gleixner,
René Nyffenegger, Andrew Morton, Paul E . McKenney,
Ingo Molnar, Oleg Nesterov, Pavel Tikhomirov, Stephen Smalley,
Ingo Molnar, H . Peter Anvin, Andy Lutomirski, Paolo Bonzini,
Rik van Riel, Josh Poimboeuf, Borislav Petkov, Brian Gerst
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
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-24 17:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-23 20:34 [PATCH] lkdtm: add bad USER_DS test Kees Cook
2017-03-24 8:14 ` Heiko Carstens
2017-03-24 15:17 ` Thomas Garnier
2017-03-24 15:24 ` Christian Borntraeger
2017-03-24 16:11 ` Thomas Garnier
2017-03-24 17:46 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).