linux-kernel - Re: [RFC v2 5/9] S.A.R.A. WX Protection

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: <CAGXu5jJuQx2qOt_aDqDQDcqGOZ5kmr5rQ9Zjv=MRRCJ65ERfGw@mail.gmail.com>
Date:   Tue, 27 Jun 2017 16:04:17 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Salvatore Mesoraca <s.mesoraca16@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        Brad Spengler <spender@...ecurity.net>,
        PaX Team <pageexec@...email.hu>,
        Casey Schaufler <casey@...aufler-ca.com>,
        James Morris <james.l.morris@...cle.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Linux-MM <linux-mm@...ck.org>, "x86@...nel.org" <x86@...nel.org>,
        Jann Horn <jannh@...gle.com>,
        Christoph Hellwig <hch@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC v2 5/9] S.A.R.A. WX Protection

On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
<s.mesoraca16@...il.com> wrote:
> +static int sara_check_vmflags(vm_flags_t vm_flags)
> +{
> +       u16 sara_wxp_flags = get_current_sara_wxp_flags();
> +
> +       if (sara_enabled && wxprot_enabled) {
> +               if (sara_wxp_flags & SARA_WXP_WXORX &&
> +                   vm_flags & VM_WRITE &&
> +                   vm_flags & VM_EXEC) {
> +                       if ((sara_wxp_flags & SARA_WXP_VERBOSE))
> +                               pr_wxp("W^X");
> +                       if (!(sara_wxp_flags & SARA_WXP_COMPLAIN))
> +                               return -EPERM;
> +               }
> +               if (sara_wxp_flags & SARA_WXP_MMAP &&
> +                   (vm_flags & VM_EXEC ||
> +                    (!(vm_flags & VM_MAYWRITE) && (vm_flags & VM_MAYEXEC))) &&
> +                   get_current_sara_mmap_blocked()) {
> +                       if ((sara_wxp_flags & SARA_WXP_VERBOSE))
> +                               pr_wxp("executable mmap");
> +                       if (!(sara_wxp_flags & SARA_WXP_COMPLAIN))
> +                               return -EPERM;
> +               }
> +       }

Given the subtle differences between these various if blocks (here and
in the other hook), I think it would be nice to have some beefy
comments here to describe specifically what's being checked (and why).
It'll help others review this code, and help validate code against
intent.

I would also try to minimize the written code by creating a macro for
a repeated pattern here:

> +                               if ((sara_wxp_flags & SARA_WXP_VERBOSE))
> +                                       pr_wxp("mprotect on file mmap");
> +                               if (!(sara_wxp_flags & SARA_WXP_COMPLAIN))
> +                                       return -EACCES;

These four lines are repeated several times with only the const char *
and return value changing. Perhaps something like:

#define sara_return(err, msg) do { \
                               if ((sara_wxp_flags & SARA_WXP_VERBOSE)) \
                                       pr_wxp(err); \
                               if (!(sara_wxp_flags & SARA_WXP_COMPLAIN)) \
                                       return -err; \
} while (0)

Then each if block turns into something quite easier to parse:

               if (sara_wxp_flags & SARA_WXP_WXORX &&
                   vm_flags & VM_WRITE &&
                   vm_flags & VM_EXEC)
                       sara_return(EPERM, "W^X");


-Kees

-- 
Kees Cook
Pixel Security