[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
