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 19:48:27 -0800
Message-Id: <2BD95E04-8DA9-40CA-B9B0-1548325B044D@amacapital.net>
References: <201911271852.4AA977DFD@keescook>
Cc:     Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
        x86 <x86@kernel.org>, linux-kernel@vger.kernel.org
In-Reply-To: <201911271852.4AA977DFD@keescook>
To:     Kees Cook <keescook@chromium.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Nov 27, 2019, at 6:54 PM, Kees Cook <keescook@chromium.org> wrote:
>=20
> =EF=BB=BFOn Wed, Nov 27, 2019 at 06:15:17PM -0800, Andy Lutomirski wrote:
>>=20
>>>> 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 o=
n
>>>>> 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 unf=
ortunate. 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=
 makes 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
>>=20
>>=20
>> So how is the test harness supposed
>> to distinguish success from failure?  If it printed UNSUPPORTED instead o=
f FAIL, it would make more sense to me, but I=E2=80=99m not sure why that=E2=
=80=99s better than just not exposing it at all.
>=20
> If kernelci or similar ever mentions this as a problem for them, I'm
> happy to change it. I think it's an error to request this test in the
> wrong environment (because that implies userspace doesn't know how to
> evaluate the results). As I like it _available_ because having it
> missing makes the code ugly (lots of ifdefs) and provides to signal to
> userspace about it (EINVAL on the write to DIRECT) doesn't tell me if I
> have the wrong kernel version or the wrong architecture, etc. Since the
> tester needs to be parsing dmesg and system state (did it panic, etc), I
> much prefer keeping the signals there.
>=20
>=20

Could we perhaps standardized on some particular error code to say =E2=80=9C=
I know about this test, but it=E2=80=99s not implemented on this kernel?=E2=80=
=9D=