Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Andy Lutomirski <luto@amacapital.net>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH] lkdtm/bugs: Avoid ifdefs for DOUBLE_FAULT
Date:   Wed, 27 Nov 2019 18:15:17 -0800
Message-Id: <C0659748-A5B3-45D3-B752-88A643C66E46@amacapital.net>
References: <201911271748.A7C361E28@keescook>
Cc:     Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
        x86 <x86@kernel.org>, linux-kernel@vger.kernel.org
In-Reply-To: <201911271748.A7C361E28@keescook>
To:     Kees Cook <keescook@chromium.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Nov 27, 2019, at 5:50 PM, Kees Cook <keescook@chromium.org> wrote:
>=20
> =EF=BB=BFOn Wed, Nov 27, 2019 at 01:01:40PM -0800, Andy Lutomirski wrote:
>>=20
>>=20
>>>> On Nov 27, 2019, at 11:19 AM, Kees Cook <keescook@chromium.org> wrote:
>>>=20
>>> =EF=BB=BFLKDTM test visibility shouldn't change, so remove the ifdefs on=

>>> DOUBLE_FAULT and make sure test failure doesn't crash the system.
>>>=20
>>> 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@chromium.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(-)
>>>=20
>>> 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
>>> }
>>>=20
>>> -#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)));
>>>=20
>>> -    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
>>> +}
>>=20
>> I=E2=80=99m not familiar with the userspace tooling, but this seems unfor=
tunate. The first FAIL is =E2=80=9Cthe test case screwed up, and it=E2=80=99=
s a bug.=E2=80=9D  The second FAIL is =E2=80=9Cnot applicable to this system=
.=E2=80=9D
>>=20
>>=20
>> ISTM simply not exposing the test on systems that don=E2=80=99t support m=
akes sense. Can you clarify?
>=20
> 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.)
>=20


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