linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Daniel Colascione <dancol@google.com>
Cc: Andy Lutomirski <luto@kernel.org>,
	Christian Brauner <christian@brauner.io>,
	LKML <linux-kernel@vger.kernel.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Jann Horn <jannh@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>, Aleksa Sarai <cyphar@cyphar.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	Tim Murray <timmurray@google.com>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH] proc: allow killing processes via file descriptors
Date: Sun, 18 Nov 2018 11:43:14 -0600	[thread overview]
Message-ID: <875zwu46z1.fsf@xmission.com> (raw)
In-Reply-To: <CAKOZues3_uh1mnQr=VPCxoWiY0wfiUq0oFXcVuiHF7rPmUXsuw@mail.gmail.com> (Daniel Colascione's message of "Sun, 18 Nov 2018 09:17:31 -0800")

Daniel Colascione <dancol@google.com> writes:

> On Sun, Nov 18, 2018 at 9:13 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
>>>
>>> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>>> >>
>>> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> >> > I fully agree that a more comprehensive, less expensive API for
>>> >> > managing processes would be nice.  But I also think that this patch
>>> >> > (using the directory fd and ioctl) is better from a security
>>> >> > perspective than using a new file in /proc.
>>> >>
>>> >> That's an assertion, not an argument. And I'm not opposed to an
>>> >> operation on the directory FD, now that it's clear Linus has banned
>>> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
>>> >> with a system call instead of a less-reliable ioctl due to the
>>> >> inherent namespace collision issues in ioctl command names.
>>> >
>>> > Linus banned it because of bugs iike the ones in the patch.
>>>
>>> Maybe: he didn't provide a reason. What's your point?
>>
>> My point is that an API that involves a file like /proc/PID/kill is
>> very tricky to get right.  Here are some considerations:
>
> Moot. write(2) for this interface is off the table anyway. The right
> approach here is a system call that accepts a /proc/pid directory file
> descriptor, a signal number, and a signal information field (as in
> sigqueue(2)).

If we did not have the permission check challenges and could perform
the permission checks in open, write(2) would be on the table.
Performing write(2) would only be concrend about data.

Unfortunately we have setresuid and exec which make that infeasible
for the kill operations.

>> Now if we had an ioctlat() API, maybe it would make sense.  But we
>> don't, and I think it would be a bit crazy to add one.
>
> A process is not a driver. Why won't this idea of using an ioctl for
> the kill-process-by-dfd thing just won't die? An ioctl has *zero*
> advantage in this context.

An ioctl has an advantage in implementation complexity.  An ioctl is
very much easier to wire up that a system call.

I don't know if that outweighs ioctls disadvantages in long term
maintainability.

Eric


  reply	other threads:[~2018-11-18 17:43 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-18 11:17 Christian Brauner
2018-11-18 13:59 ` Daniel Colascione
2018-11-18 15:38   ` Andy Lutomirski
2018-11-18 15:53     ` Daniel Colascione
2018-11-18 16:17       ` Andy Lutomirski
2018-11-18 16:29         ` Daniel Colascione
2018-11-18 17:13           ` Andy Lutomirski
2018-11-18 17:17             ` Daniel Colascione
2018-11-18 17:43               ` Eric W. Biederman [this message]
2018-11-18 17:45                 ` Andy Lutomirski
2018-11-18 17:56                 ` Daniel Colascione
2018-11-18 16:33         ` Randy Dunlap
2018-11-18 16:48           ` Daniel Colascione
2018-11-18 17:09             ` Andy Lutomirski
2018-11-18 17:24               ` Daniel Colascione
2018-11-18 17:42                 ` Andy Lutomirski
2018-11-18 17:51                   ` Daniel Colascione
2018-11-18 18:28                     ` Andy Lutomirski
2018-11-18 18:43                       ` Daniel Colascione
2018-11-18 19:05                         ` Aleksa Sarai
2018-11-18 19:44                           ` Daniel Colascione
2018-11-18 20:15                             ` Christian Brauner
2018-11-18 20:21                               ` Daniel Colascione
2018-11-18 20:28                             ` Andy Lutomirski
2018-11-18 20:32                               ` Daniel Colascione
2018-11-19  1:43                                 ` Andy Lutomirski
2018-11-18 20:43                               ` Christian Brauner
2018-11-18 20:54                                 ` Daniel Colascione
2018-11-18 21:23                                   ` Christian Brauner
2018-11-18 21:30                                     ` Christian Brauner
2018-11-19  0:31                                       ` Daniel Colascione
2018-11-19  0:40                                         ` Christian Brauner
2018-11-19  0:09                             ` Aleksa Sarai
2018-11-19  0:53                               ` Daniel Colascione
2018-11-19  1:16                                 ` Daniel Colascione
2018-11-19 16:13                       ` Dmitry Safonov
2018-11-19 16:26                         ` [PATCH] proc: allow killing processes via file descriptors (Larger pids) Eric W. Biederman
2018-11-19 16:27                         ` [PATCH] proc: allow killing processes via file descriptors Daniel Colascione
2018-11-19 20:21                           ` Aleksa Sarai
2018-11-19  2:47                   ` Al Viro
2018-11-19  3:01                     ` Andy Lutomirski
2018-11-18 17:41     ` Christian Brauner
2018-11-18 17:44       ` Andy Lutomirski
2018-11-18 18:07       ` Daniel Colascione
2018-11-18 18:15         ` Andy Lutomirski
2018-11-18 18:31           ` Daniel Colascione
2018-11-18 19:24         ` Christian Brauner
2018-11-19  0:08         ` Aleksa Sarai
2018-11-19  1:14           ` Daniel Colascione
2018-11-18 16:03 ` Daniel Colascione
2018-11-19 10:56 ` kbuild test robot
2018-11-19 14:15 ` David Laight
2018-11-19 15:49 ` Dave Martin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875zwu46z1.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=akpm@linux-foundation.org \
    --cc=christian@brauner.io \
    --cc=cyphar@cyphar.com \
    --cc=dancol@google.com \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=oleg@redhat.com \
    --cc=serge@hallyn.com \
    --cc=timmurray@google.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH] proc: allow killing processes via file descriptors' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox