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]

Date:   Thu, 29 Jun 2017 21:39:20 +0200
From:   Salvatore Mesoraca <s.mesoraca16@...il.com>
To:     Kees Cook <keescook@...omium.org>
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

2017-06-28 1:04 GMT+02:00 Kees Cook <keescook@...omium.org>:
> 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");

I absolutely agree with all of the above. These issues will be addressed in v3.
Thank you for your contribution.

Salvatore