linux-kernel - Re: [PATCH] lkdtm/bugs: Avoid ifdefs for DOUBLE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Nov 2019 18:15:17 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Kees Cook <keescook@...omium.org>
Cc:     Ingo Molnar <mingo@...nel.org>, Andy Lutomirski <luto@...nel.org>,
        x86 <x86@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lkdtm/bugs: Avoid ifdefs for DOUBLE_FAULT


> On Nov 27, 2019, at 5:50 PM, Kees Cook <keescook@...omium.org> wrote:
> 
> On Wed, Nov 27, 2019 at 01:01:40PM -0800, Andy Lutomirski wrote:
>> 
>> 
>>>> On Nov 27, 2019, at 11:19 AM, Kees Cook <keescook@...omium.org> wrote:
>>> 
>>> LKDTM test visibility shouldn't change, so remove the ifdefs on
>>> DOUBLE_FAULT and make sure test failure doesn't crash the system.
>>> 
>>> Link: https://lore.kernel.org/lkml/20191127184837.GA35982@gmail.com
>>> Fixes: b09511c253e5 ("lkdtm: Add a DOUBLE_FAULT crash type on x86")
>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>> ---
>>> applies on top of tip/x86/urgent
>>> ---
>>> drivers/misc/lkdtm/bugs.c  | 8 +++++---
>>> drivers/misc/lkdtm/core.c  | 4 +---
>>> drivers/misc/lkdtm/lkdtm.h | 2 --
>>> 3 files changed, 6 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
>>> index a4fdad04809a..22f5293414cc 100644
>>> --- a/drivers/misc/lkdtm/bugs.c
>>> +++ b/drivers/misc/lkdtm/bugs.c
>>> @@ -342,9 +342,9 @@ void lkdtm_UNSET_SMEP(void)
>>> #endif
>>> }
>>> 
>>> -#ifdef CONFIG_X86_32
>>> void lkdtm_DOUBLE_FAULT(void)
>>> {
>>> +#ifdef CONFIG_X86_32
>>>   /*
>>>    * Trigger #DF by setting the stack limit to zero.  This clobbers
>>>    * a GDT TLS slot, which is okay because the current task will die
>>> @@ -373,6 +373,8 @@ void lkdtm_DOUBLE_FAULT(void)
>>>   asm volatile ("movw %0, %%ss; addl $0, (%%esp)" ::
>>>             "r" ((unsigned short)(GDT_ENTRY_TLS_MIN << 3)));
>>> 
>>> -    panic("tried to double fault but didn't die\n");
>>> -}
>>> +    pr_err("FAIL: tried to double fault but didn't die!\n");
>>> +#else
>>> +    pr_err("FAIL: this test is only available on 32-bit x86.\n");
>>> #endif
>>> +}
>> 
>> I’m not familiar with the userspace tooling, but this seems unfortunate. The first FAIL is “the test case screwed up, and it’s a bug.”  The second FAIL is “not applicable to this system.”
>> 
>> 
>> ISTM simply not exposing the test on systems that don’t support makes sense. Can you clarify?
> 
> I don't like the tests liked in the DIRECT file to change from build to
> build (it should be stable per kernel version). Userspace needs to know
> how to evaluate the results of running each test, so in both cases, I
> consider it a failure: double fault didn't work or you tried to test
> double fault on an unsupported architecture. (The SMEP test works
> similarly.)
> 


So how is the test harness supposed
to distinguish success from failure?  If it printed UNSUPPORTED instead of FAIL, it would make more sense to me, but I’m not sure why that’s better than just not exposing it at all.