lkml.org 
[lkml]   [2016]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] prctl: remove one-shot limitation for changing exe link
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.
>
> 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

\
 
 \ /
  Last update: 2016-07-31 21:21    [W:0.116 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site