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;
MIME-Version: 1.0
References: <8593f7faf89812a9987d44d9ae615d64dca4d77f.1544800744.git.christophe.leroy@c-s.fr>
In-Reply-To: <8593f7faf89812a9987d44d9ae615d64dca4d77f.1544800744.git.christophe.leroy@c-s.fr>
From:   Kees Cook <keescook@chromium.org>
Date:   Tue, 8 Jan 2019 17:14:36 -0800
Message-ID: <CAGXu5jKeGp_z_69SybtkSaE381Ui7aXCxp9yBxv=TS0=hSsm-A@mail.gmail.com>
Subject: Re: [PATCH] lkdtm: Add a tests for NULL pointer dereference
To:     Christophe Leroy <christophe.leroy@c-s.fr>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        PowerPC <linuxppc-dev@lists.ozlabs.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

-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
>


-- 
Kees Cook