Message-ID: <3647508c-aa94-8540-6fbc-f781aeea77ce@virtuozzo.com>
Date:	Wed, 10 Aug 2016 12:48:08 +0200
From:	Stanislav Kinsburskiy <skinsbursky@...tuozzo.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Cyrill Gorcunov <gorcunov@...il.com>
CC:	<peterz@...radead.org>, <mingo@...hat.com>, <mhocko@...e.com>,
	<keescook@...omium.org>, <linux-kernel@...r.kernel.org>,
	<mguzik@...hat.com>, <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>
Subject: Re: [PATCH] prctl: remove one-shot limitation for changing exe link



30.07.2016 19:31, Eric W. Biederman пишет:
> Cyrill Gorcunov <gorcunov@...il.com> writes:
>
>> On Mon, Jul 25, 2016 at 02:56:43PM -0500, Eric W. Biederman wrote:
>> ...
>>>>> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
>>>>> validate that the new file is a actually mmaped executable.  We would
>>>>> definitely need that to be fixed before even considering removing the
>>>>> limit.
>>>> Could you please elaborate? We check for inode being executable,
>>>> what else needed?
>>> That the inode is mmaped into the process with executable mappings.
>>>
>>> Effectively what we check the old mapping for and refuse to remove the old
>>> mm_exe_file if it exists.
>>>
>>> I think a reasonable argument can be made that if the file is
>>> executable, and it is mmaped with executable pages that exe_file is not
>>> a complete lie.
>> I might be missing something obvious, so sorry for the question --
>> when criu setups old exe link the inode we obtain from file open
>> is not mapped into memory, the old exe not read by anyone because
>> it's not even executed anyhow. So I don't really understand which
>> mapping we should check here. Mind to point me?
> That sounds like an out and out bug that should not be preserved.
> Of course we should mmap the executable and set it up so that it can be
> executed (at least as much as the executable was previously mapped).
> Anything else is a buggy restart, and lying to userspace.
>
>>> Which is the important part.  At the end of the day how much can
>>> userspace trust /proc/pid/exe?  If we are too lax it is just a random
>>> file descriptor we can not trust at all.  At which point there is
>>> exactly no point in preserving it in checkpoint/restart, because nothing
>>> will trust or look at it.
>> You know, I think we should not trust exe link much, and in real we
>> never could: this link is rather a hint pointing which executable a
>> process has been using on execve call, once the process start working
>> one can't be sure if the code currently running is exactly from the
>> file pointed by exe link. It just a hint suitable for debuggin and
>> obtain clean view of which processes are running on noncompromised
>> system. Monitoring exe link change won't help much if there are
>> malicious software running on the system.
> But it is not just a hint.  It is a record of which executable we called
> execve on.  Knowing which file was executed doesn't guarantee what is
> running now but it provides a very strong hint.
>
> At then end of a restart the state of a process should be (by
> definition) exactly the state the process was before a checkpoint
> and thus a state the original executable could have gotten into.
>
> I admit it is possible for an application to unmap itself.  I honestly
> have not met that application (except perhaps criu).
>
>>> If the only user is checkpoint/restart perhaps it should be only ptrace
>>> that can set this and not the process itself with a prctl.  I don't
>>> know.  All I know is that we should work on making it a very trustable
>>> value even though in some specific instances we can set it.
>> Since as I said I suppose nobody except us using this feature, we can
>> setup some sysctl trigger for it (I personally think this is an
>> overkill, but OTOH if people rely on the exe link and not going
>> to use criu at all, this trigger will help).
> Some clarity of thought came to me, and I apologise for not replying
> sooner with it sooner.
>
> My problem with the original patch submission is that it was
> justifying changing prctl_set_mm_exe_file based on what
> prctl_set_mm_exe_file does today.  As prctl_set_mm_exe_file was added
> for the checkpoint/restart case that is justifying changing code based
> on a buggy implementation.
>
> It is necessary to look at the ordinary situation.  Without
> prctl_set_mm_exe /proc/[pid]/exe can be counted on as a record
> of which executable was last passed to execve.  Furthermore the state of
> a process can be counted on to be a state reachable from calling
> execve on /proc/[pid]/exe.
>
> Which means to preserve those expectations prctl_set_mm_exe_file should
> in practice just be a nicer less cumbersome interface to things you can
> already achieve with execve.
>
> Justifying removale of the one-short nature for prctl_set_mm_exe_file
> is as straight forward as noting that a process can call execve on
> any executable file.
>
> However when I compare the invariants that execve has on a file (such as
> the executable being mmaped) I see some noticable disparities between
> what prctl_set_mm_exe_file allows and what execve allows.  With
> prctl_set_mm_exe being less strict.
>
> 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.

Eric, could you elaborate on the checks, you mentioned?
I don't see any significant checks, which are applicable to
prctl_set_mm_exe_file.
Say, there is a check for PF_NPROC_EXCEEDED in execve, but this is not 
applicable.
Some of the checks are related to file open, but this is not done in 
prctl_set_mm_exe_file.
There is some logic, related to "close on exec", but this is something 
which has to be avoided.
Would be nice to have an example of a check, which is missing.

> Once the checks in prctl_set_mm_exe_file are tightened up please feel
> free to remove the one shot test.
>
> Eric

Powered by blists - more mailing lists