Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676249 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC v2 5/9] S.A.R.A. WX Protection Kees Cook <keescook@chromium.org> - 2017-06-28 01:10 +0200
    Re: [RFC v2 5/9] S.A.R.A. WX Protection Salvatore Mesoraca <s.mesoraca16@gmail.com> - 2017-06-29 21:40 +0200

#1676249 — Re: [RFC v2 5/9] S.A.R.A. WX Protection

On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
<s.mesoraca16@gmail.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

[toc] | [next] | [standalone]

#1678128

2017-06-28 1:04 GMT+02:00 Kees Cook <keescook@chromium.org>:
> On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
> <s.mesoraca16@gmail.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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web