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.
--
Mateusz Guzik
