Re: [PATCH] prctl: remove one-shot limitation for changing exe link

[Richard Guy Briggs <rgb@redhat.com>]

On 2016-07-31 13:45, Eric W. Biederman wrote:
> Mateusz Guzik <mguzik@redhat.com> writes:
> 
> > On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
> >> So what I am requesting is very simple.  That the checks in
> >> prctl_set_mm_exe_file be tightened up to more closely approach what
> >> execve requires.  Thus preserving the value of the /proc/[pid]/exe for
> >> the applications that want to use the exe link.
> >> 
> >> Once the checks in prctl_set_mm_exe_file are tightened up please feel
> >> free to remove the one shot test.
> >
> > This is more fishy.
> >
> > First of all exe_file is used by the audit subsystem. So someone has to
> > ask audit people what is the significance (if any) of the field.

This was added as part of the ability to audit execution by filename
rather than by inode, the latter of which must exist at the time of the
rule instantiation and can be renamed on disk.  The former allows a rule
to be instantiated before the path exists and to follow the path even if
the original inode of the path is replaced.

> > All exe_file users but one use get_mm_exe_file and handle NULL
> > gracefully.
> >
> > Even with the current limit of changing the field once, the user can
> > cause a transient failure of get_mm_exe_file which can fail to increment
> > the refcount before it drops to 0.
> >
> > This transient failure can be used to get a NULL value stored in
> > ->exe_file during fork (in dup_mmap):
> > RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm));
> >
> > The one place which is not using get_mm_exe_file to get to the pointer
> > is audit_exe_compare:
> >         rcu_read_lock();
> >         exe_file = rcu_dereference(tsk->mm->exe_file);
> >         ino = exe_file->f_inode->i_ino;
> >         dev = exe_file->f_inode->i_sb->s_dev;
> >         rcu_read_unlock();
> >
> > This is buggy on 2 accounts:
> > 1. exe_file can be NULL

Agreed, this is a bug.

> > 2. rcu does not protect f_inode

Thank you for pointing this out too.

I'll send a patch to fix this.

> > The issue is made worse with allowing arbitrary number changes.
> >
> > Modifying get_mm_exe_file to retry is trivial and in effect never return
> > NULL is trivial. With arbitrary number of changes allowed this may
> > require some cond_resched() or something.

I agree this sounds like a wise idea.

> > For comments I cc'ed Richard Guy Briggs, who is both an audit person and
> > the author of audit_exe_compare.
> 
> That is fair.  Keeping the existing users working is what needs to
> happen.
> 
> At the same time we have an arbitrary number of possible changes with
> exec, but I guess that works differently because the mm is changed as
> well.
> 
> So yes let's bug fix this piece of code and then we can see about
> relaxing constraints.

Ok, please comment on the subsequent patch and I'll get Paul Moore to
push this through the audit tree and also to stable.

> Eric

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635