linux-kernel - Re: [Patch v3 06/13] mm: Pass task instead of task->mm as argument to set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Oct 2018 15:22:09 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
cc:     Jiri Kosina <jikos@...nel.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Casey Schaufler <casey.schaufler@...el.com>,
        Asit Mallick <asit.k.mallick@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Jon Masters <jcm@...hat.com>, linux-kernel@...r.kernel.org,
        x86@...nel.org
Subject: Re: [Patch v3 06/13] mm: Pass task instead of task->mm as argument
 to set_dumpable

On Wed, 17 Oct 2018, Tim Chen wrote:

> Change the argument to set_dumpable from task->mm to task.  This allows us
> to later add hooks to modify a task's property according to whether it is
> a non-dumpable task. Non dumpable tasks demand a higher level of security.
> Changes the dumpable value from in to unsigned int as negative number is
> not allowed.

Please use paragraphs and do not write everything in one big lump. Also
please start with the context and the rationale for a change before
explaining what. Suggestion:

  set_dumpable() takes a struct mm pointer as argument, but for finer
  grained security control of hardware vulnerabilites a architecture
  specific set_dumpable() needs to be added which needs access to the task
  struct and not only to the tasks mm struct.

  Replace the mm pointer with a task pointer and fix up implementation and
  call sites.

  While at it change the type of the value argument from int to unsigned
  int as the valid dumpable mode values are greater equal zero.

Hmm?

> diff --git a/fs/exec.c b/fs/exec.c
> index 1ebf6e5..e204830 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1362,9 +1362,9 @@ void setup_new_exec(struct linux_binprm * bprm)
>  	if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
>  	    !(uid_eq(current_euid(), current_uid()) &&
>  	      gid_eq(current_egid(), current_gid())))
> -		set_dumpable(current->mm, suid_dumpable);
> +		set_dumpable(current, (unsigned int) suid_dumpable);

Yuck. For one the type cast is pointless, but can we please fix the whole
thing and make suid_dumpable unsigned int?

Thanks,

	tglx