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

[Eric W. Biederman]

Mateusz Guzik <mguzik@xxxxxxxxxx> 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.
>
> 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
> 2. rcu does not protect f_inode
>
> 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.
>
> 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.

Eric