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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87vazlzlxm.fsf@x220.int.ebiederm.org>
Date:	Sun, 31 Jul 2016 13:45:09 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Mateusz Guzik <mguzik@...hat.com>
Cc:	Cyrill Gorcunov <gorcunov@...il.com>,
	Stanislav Kinsburskiy <skinsbursky@...tuozzo.com>,
	peterz@...radead.org, mingo@...hat.com, mhocko@...e.com,
	keescook@...omium.org, linux-kernel@...r.kernel.org,
	bsegall@...gle.com, john.stultz@...aro.org, oleg@...hat.com,
	matthltc@...ibm.com, akpm@...ux-foundation.org,
	luto@...capital.net, vbabka@...e.cz, xemul@...tuozzo.com,
	Richard Guy Briggs <rgb@...hat.com>
Subject: Re: [PATCH] prctl: remove one-shot limitation for changing exe link

Mateusz Guzik <mguzik@...hat.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