linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andy Lutomirski <luto@amacapital.net>
To: Daniel Colascione <dancol@google.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Christian Brauner <christian@brauner.io>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	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>,
	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>,
	Jan Engelhardt <jengelh@inai.de>
Subject: Re: [PATCH] proc: allow killing processes via file descriptors
Date: Sun, 18 Nov 2018 17:43:34 -0800	[thread overview]
Message-ID: <CALCETrUAX72MNB8yPmzuwjW=yvMB=CHSSdowuZNperv3aDHFcQ@mail.gmail.com> (raw)
In-Reply-To: <CAKOZuet4uzYjvNznfUvid2RH8kAuxteWWc26vLhJHKSfS6MjAA@mail.gmail.com>

On Sun, Nov 18, 2018 at 12:32 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Sun, Nov 18, 2018 at 12:28 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >> That is, I'm proposing an API that looks like this:
> >>
> >> int process_kill(int procfs_dfd, int signo, const union sigval value)
> >>
> >> If, later, process_kill were to *also* accept process-capability FDs,
> >> nothing would break.
> >
> > Except that this makes it ambiguous to the caller as to whether their current creds are considered.  So it would need to be a different syscall or at least a flag.  Otherwise a lot of those nice theoretical properties go away.
>
> Sure. A flag might make for better ergonomics.
>
> >> Yes, that's what I have in mind. A siginfo_t is small enough that we
> >> could just store it as a blob allocated off the procfs inode or
> >> something like that without bothering with a shmfs file. You'd be able
> >> to read(2) the exit status as many times as you wanted.
> >
> > I think that, if the syscall in question is read(2), then it should work *once* per struct file.  Otherwise running cat on the file would behave very oddly.
>
> Why? The file pointer would work normally.

Can you explain the exact semantics?  If I have an fd where read(2)
returns the same 4-byte value every time read(2) is called, then cat
will just return an infinite sequence of the same value.  This is not
a complete disaster, but it's not really a good thing.

>
> > Read and poll have the same problem as write: we can’t check caps in read or poll either.
>
> Why not? Reading /proc/pid/stat does an access check today and
> conditionally replaces the exit status with zero.

And that's probably a bug.  It's at least a giant kludge that we shouldn't copy.

Here is the general rule: the basic operations that are expected to
treat file descriptors as capabilities *must* treat file descriptors
as capabilities, at least for new APIs.  This includes read(2),
write(2), and poll(2).  We should have an exceedingly good reason to
check current's creds, mm, or anything else about current in those
syscalls.

There is a good reason for this: consider what happens if you type:

sudo >/proc/PID/whatever

or

sudo </proc/PID/whatever

  reply	other threads:[~2018-11-19  1: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
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 [this message]
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='CALCETrUAX72MNB8yPmzuwjW=yvMB=CHSSdowuZNperv3aDHFcQ@mail.gmail.com' \
    --to=luto@amacapital.net \
    --cc=akpm@linux-foundation.org \
    --cc=christian@brauner.io \
    --cc=cyphar@cyphar.com \
    --cc=dancol@google.com \
    --cc=ebiederm@xmission.com \
    --cc=jannh@google.com \
    --cc=jengelh@inai.de \
    --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=rdunlap@infradead.org \
    --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