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