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

[Jann Horn <jann@thejh.net>]

[-- Attachment #1: Type: text/plain, Size: 3028 bytes --]

On Fri, Sep 23, 2016 at 02:04:30PM -0700, Andy Lutomirski wrote:
> 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.

Ah, I didn't realize that either of those was possible. Yes, I guess I'll
change it to use local64_t.

> > +
> > +/*
> > + * 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().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]