Re: [PATCH v2 2/8] exec: turn self_exec_id into self_privunit

From: Andy Lutomirski <luto@amacapital.net>
To: Jann Horn <jann@thejh.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Roland McGrath <roland@hack.frob.com>,
	Oleg Nesterov <oleg@redhat.com>,
	John Johansen <john.johansen@canonical.com>,
	James Morris <james.l.morris@oracle.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Paul Moore <aul@paul-moore.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Eric Paris <eparis@parisplace.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Janis Danisevskis <jdanis@google.com>,
	Seth Forshee <seth.forshee@canonical.com>,
	"Eric . Biederman" <ebiederm@xmission.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Benjamin LaHaise <bcrl@kvack.org>,
	Ben Hutchings <ben@decadent.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	"security@kernel.org" <security@kernel.org>
Subject: Re: [PATCH v2 2/8] exec: turn self_exec_id into self_privunit
Date: Fri, 23 Sep 2016 14:04:30 -0700	[thread overview]
Message-ID: <CALCETrVT3MofhX56jJBeWJbucOrfnw8RyLXa0X=64gtpGDUrPg@mail.gmail.com> (raw)
In-Reply-To: <1474663238-22134-3-git-send-email-jann@thejh.net>

On Fri, Sep 23, 2016 at 1:40 PM, Jann Horn <jann@thejh.net> wrote:
> This ensures that self_privunit ("privilege unit locally unique ID")
> is only shared by processes that share the mm_struct and the signal_struct;
> not just spatially, but also temporally. In other words, if you do execve()
> or clone() without CLONE_THREAD, you get a new privunit that has never
> been used before.
>
> One reason for doing this is that it prevents an attacker from sending an
> arbitrary signal to a parent process after performing 2^32-1 execve()
> calls.
>
> The second reason for this is that it permits using the self_exec_luid in
> a later patch to check during a ptrace access whether subject and object
> are temporally and spatially equal for privilege checking purposes.
>
> The implementation of locally unique IDs is in sched.h and exec.c for now
> because those are the only users so far - if anything else wants to use
> them in the future, they can be moved elsewhere.
>
> changed in v2:
>  - have 2^64 IDs per CPU instead of 2^64 shared ones (luid scheme,
>    suggested by Andy Lutomirski)
>  - take task_lock for reading in setup_new_exec() while bumping the LUID
>
> Signed-off-by: Jann Horn <jann@thejh.net>
> ---
>  fs/exec.c             | 41 +++++++++++++++++++++++++++++++++++++++--
>  include/linux/sched.h | 17 +++++++++++++++--
>  kernel/fork.c         |  5 +++--
>  kernel/signal.c       |  5 ++++-
>  4 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 84430ee..fcc11f0 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1281,6 +1281,34 @@ void would_dump(struct linux_binprm *bprm, struct file *file)
>  }
>  EXPORT_SYMBOL(would_dump);
>
> +static DEFINE_PER_CPU(u64, luid_counters);
> +
> +static int __init init_luid_counters(void)
> +{
> +       unsigned int cpu;
> +
> +       for_each_possible_cpu(cpu) {
> +               /* value 0 is reserved for init */
> +               per_cpu(luid_counters, cpu) = 1;
> +       }
> +
> +       return 0;
> +}
> +early_initcall(init_luid_counters);

How about static DEFINE_PER_CPU(u64, luid_counters) = 1?  You could
optionally use local64_t instead, which would let you avoid needing to
think about preemption.

> +
> +/*
> + * Allocates a new LUID and writes the allocated LUID to @out.
> + * This function must not be called from IRQ context.
> + */
> +void fill_luid(struct luid *out)
> +{
> +       preempt_disable();
> +       out->count = raw_cpu_read(luid_counters);
> +       raw_cpu_add(luid_counters, 1);
> +       out->cpu = smp_processor_id();
> +       preempt_enable();
> +}
> +

I would call this alloc_luid().