2017-06-28 1:04 GMT+02:00 Kees Cook <keescook@xxxxxxxxxxxx>:
> On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
> <s.mesoraca16@xxxxxxxxx> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
