linux-kernel - Re: [PATCH] lkdtm: Add a tests for NULL pointer dereference

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]

Message-ID: <CAGXu5jKUArQAkirjTTrPM-xayT1aYEGzmyveDZwrLuqmvRqD+A@mail.gmail.com>
Date:   Wed, 9 Jan 2019 07:16:55 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        PowerPC <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH] lkdtm: Add a tests for NULL pointer dereference

On Tue, Jan 8, 2019 at 10:31 PM Christophe Leroy
<christophe.leroy@....fr> wrote:
>
>
>
> Le 09/01/2019 à 02:14, Kees Cook a écrit :
> > On Fri, Dec 14, 2018 at 7:26 AM Christophe Leroy
> > <christophe.leroy@....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 ?

Okay, gotcha. You're getting more complete reporting coverage. Sounds
good to me. Thanks!

Acked-by: Kees Cook <keescook@...omium.org>

-Kees

>
> Christophe
>
> >
> > -Kees
> >
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@....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