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;
Subject: Re: [PATCH] lkdtm: Add a tests for NULL pointer dereference
To:     Kees Cook <keescook@chromium.org>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        PowerPC <linuxppc-dev@lists.ozlabs.org>
References: <8593f7faf89812a9987d44d9ae615d64dca4d77f.1544800744.git.christophe.leroy@c-s.fr>
 <CAGXu5jKeGp_z_69SybtkSaE381Ui7aXCxp9yBxv=TS0=hSsm-A@mail.gmail.com>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <2d2e8cef-dd12-75e8-4779-fe4437e2169c@c-s.fr>
Date:   Wed, 9 Jan 2019 07:31:17 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.3
MIME-Version: 1.0
In-Reply-To: <CAGXu5jKeGp_z_69SybtkSaE381Ui7aXCxp9yBxv=TS0=hSsm-A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


Le 09/01/2019 à 02:14, Kees Cook a écrit :
> On Fri, Dec 14, 2018 at 7:26 AM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>
>> Introduce lkdtm tests for NULL pointer dereference: check
>> access or exec at NULL address.
> 
> Why is this not already covered by the existing tests? (Is there
> something special about NULL that is being missed?) I'd expect SMAP
> and SMEP to cover NULL as well.

Most arches print a different message whether the faulty address is 
above or under PAGE_SIZE. Below is exemple from x86:

	pr_alert("BUG: unable to handle kernel %s at %px\n",
		 address < PAGE_SIZE ? "NULL pointer dereference" : "paging request",
		 (void *)address);


Until recently, the powerpc arch didn't do it. When I implemented it 
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49a502ea23bf9dec47f8f3c3960909ff409cd1bb), 
I needed a way to test it and couldn't find an existing one, hence this 
new LKDTM test.

But maybe I missed something ?

Christophe

> 
> -Kees
> 
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   drivers/misc/lkdtm/core.c  |  2 ++
>>   drivers/misc/lkdtm/lkdtm.h |  2 ++
>>   drivers/misc/lkdtm/perms.c | 18 ++++++++++++++++++
>>   3 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
>> index bc76756b7eda..36910e1d5c09 100644
>> --- a/drivers/misc/lkdtm/core.c
>> +++ b/drivers/misc/lkdtm/core.c
>> @@ -157,7 +157,9 @@ static const struct crashtype crashtypes[] = {
>>          CRASHTYPE(EXEC_VMALLOC),
>>          CRASHTYPE(EXEC_RODATA),
>>          CRASHTYPE(EXEC_USERSPACE),
>> +       CRASHTYPE(EXEC_NULL),
>>          CRASHTYPE(ACCESS_USERSPACE),
>> +       CRASHTYPE(ACCESS_NULL),
>>          CRASHTYPE(WRITE_RO),
>>          CRASHTYPE(WRITE_RO_AFTER_INIT),
>>          CRASHTYPE(WRITE_KERN),
>> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
>> index 3c6fd327e166..b69ee004a3f7 100644
>> --- a/drivers/misc/lkdtm/lkdtm.h
>> +++ b/drivers/misc/lkdtm/lkdtm.h
>> @@ -45,7 +45,9 @@ void lkdtm_EXEC_KMALLOC(void);
>>   void lkdtm_EXEC_VMALLOC(void);
>>   void lkdtm_EXEC_RODATA(void);
>>   void lkdtm_EXEC_USERSPACE(void);
>> +void lkdtm_EXEC_NULL(void);
>>   void lkdtm_ACCESS_USERSPACE(void);
>> +void lkdtm_ACCESS_NULL(void);
>>
>>   /* lkdtm_refcount.c */
>>   void lkdtm_REFCOUNT_INC_OVERFLOW(void);
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index fa54add6375a..62f76d506f04 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -164,6 +164,11 @@ void lkdtm_EXEC_USERSPACE(void)
>>          vm_munmap(user_addr, PAGE_SIZE);
>>   }
>>
>> +void lkdtm_EXEC_NULL(void)
>> +{
>> +       execute_location(NULL, CODE_AS_IS);
>> +}
>> +
>>   void lkdtm_ACCESS_USERSPACE(void)
>>   {
>>          unsigned long user_addr, tmp = 0;
>> @@ -195,6 +200,19 @@ void lkdtm_ACCESS_USERSPACE(void)
>>          vm_munmap(user_addr, PAGE_SIZE);
>>   }
>>
>> +void lkdtm_ACCESS_NULL(void)
>> +{
>> +       unsigned long tmp;
>> +       unsigned long *ptr = (unsigned long *)NULL;
>> +
>> +       pr_info("attempting bad read at %px\n", ptr);
>> +       tmp = *ptr;
>> +       tmp += 0xc0dec0de;
>> +
>> +       pr_info("attempting bad write at %px\n", ptr);
>> +       *ptr = tmp;
>> +}
>> +
>>   void __init lkdtm_perms_init(void)
>>   {
>>          /* Make sure we can write to __ro_after_init values during __init */
>> --
>> 2.13.3
>>
> 
>