RE: [PATCH v5 5/6] prctl: Allow checkpoint/restore capable processes to change exe link

From: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
To: Christian Brauner <christian.brauner@ubuntu.com>,
	Adrian Reber <areber@redhat.com>
Cc: "Eric Biederman" <ebiederm@xmission.com>,
	"Pavel Emelyanov" <ovzxemul@gmail.com>,
	"Oleg Nesterov" <oleg@redhat.com>,
	"Dmitry Safonov" <0x7f454c46@gmail.com>,
	"Andrei Vagin" <avagin@gmail.com>,
	"Michał Cłapiński" <mclapinski@google.com>,
	"Kamil Yurtsever" <kyurtsever@google.com>,
	"Dirk Petersen" <dipeit@gmail.com>,
	"Christine Flood" <chf@redhat.com>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"Mike Rapoport" <rppt@linux.ibm.com>,
	"Radostin Stoyanov" <rstoyanov1@gmail.com>,
	"Cyrill Gorcunov" <gorcunov@openvz.org>,
	"Serge Hallyn" <serge@hallyn.com>,
	"Stephen Smalley" <stephen.smalley.work@gmail.com>,
	"Sargun Dhillon" <sargun@sargun.me>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"linux-security-module@vger.kernel.org"
	<linux-security-module@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"selinux@vger.kernel.org" <selinux@vger.kernel.org>,
	"Eric Paris" <eparis@parisplace.org>,
	"Jann Horn" <jannh@google.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: RE: [PATCH v5 5/6] prctl: Allow checkpoint/restore capable processes to change exe link
Date: Wed, 15 Jul 2020 15:49:57 +0000	[thread overview]
Message-ID: <ba55d8b160e541429dc0c823d3240eb3@EXMBDFT11.ad.twosigma.com> (raw)
In-Reply-To: <20200715152011.whdeysy3ztqrnocn@wittgenstein>

> On Wed, Jul 15, 2020 at 04:49:53PM +0200, Adrian Reber wrote:
> > From: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
> > 
> > Allow CAP_CHECKPOINT_RESTORE capable users to change /proc/self/exe.
> > 
> > This commit also changes the permission error code from -EINVAL to 
> > -EPERM for consistency with the rest of the prctl() syscall when 
> > checking capabilities.
> I agree that EINVAL seems weird here but this is a potentially user visible change. Might be nice to have the EINVAL->EPERM change be an additional patch on top after this one so we can revert it in case it breaks someone (unlikely though). I can split this out myself though so no need to resend for that alone.
> What I would also prefer is to have some history in the commit message tbh. The reason is that when we started discussing that specific change I had to hunt down the history of changing /proc/self/exe and had to dig up and read through ancient threads on lore to come up with the explanation why this is placed under a capability. The commit message should then also mention that there are other ways to change the /proc/self/exe link that don't require capabilities and that /proc/self/exe itself is not something userspace should rely on for security. Mainly so that in a few months/years we can read through that commit message and go "Weird, but ok.". :)
> But maybe I can just rewrite this myself so you don't have to go through the trouble. This is really not pedantry it's just that it's a lot of work digging up the reasons for a piece of code existing when it's really not obvious. :)

Hello Christian,

I agree.
Thank you for suggesting doing the work, but you've done plenty already. So we'll come back to you with:
1) A separate commit for EINVAL->EPERM
2) A full history of discussions in the commit message related to /proc/self/exe capability check

Thanks,
Nico