From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH] prctl: remove one-shot limitation for changing exe link Date: Sun, 31 Jul 2016 13:45:09 -0500 Message-ID: <87vazlzlxm.fsf__6650.60100514001$1470063081$gmane$org@x220.int.ebiederm.org> References: <20160712152940.24895.61315.stgit@localhost.localdomain> <8a863273-c571-63d6-c0c3-637dff5645a3@virtuozzo.com> <87y44pbmtc.fsf@x220.int.ebiederm.org> <20160725192242.GA26208@uranus> <87a8h58pac.fsf@x220.int.ebiederm.org> <20160726083445.GB26208@uranus> <87y44j6nib.fsf@x220.int.ebiederm.org> <20160730202821.7ojhciviocjfnw7p@mguzik> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from madcap2.tricolour.ca (vpn-48-183.rdu2.redhat.com [10.10.48.183]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u716D91Y010299 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 1 Aug 2016 02:13:11 -0400 Resent-Message-ID: <20160801061309.GW10734@madcap2.tricolour.ca> Resent-To: linux-audit@redhat.com In-Reply-To: <20160730202821.7ojhciviocjfnw7p@mguzik> (Mateusz Guzik's message of "Sat, 30 Jul 2016 22:28:22 +0200") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Mateusz Guzik Cc: bsegall@google.com, mhocko@suse.com, peterz@infradead.org, Stanislav Kinsburskiy , linux-kernel@vger.kernel.org, oleg@redhat.com, Cyrill Gorcunov , Richard Guy Briggs , mingo@redhat.com, john.stultz@linaro.org, matthltc@us.ibm.com, akpm@linux-foundation.org, luto@amacapital.net, vbabka@suse.cz, xemul@virtuozzo.com List-Id: linux-audit@redhat.com Mateusz Guzik 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