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

Groups > linux.kernel > #1678128

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

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

csiph-web