LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Aleksa Sarai <cyphar@cyphar.com>
To: Christian Brauner <christian@brauner.io>
Cc: ebiederm@xmission.com, linux-kernel@vger.kernel.org,
	serge@hallyn.com, jannh@google.com, luto@kernel.org,
	akpm@linux-foundation.org, oleg@redhat.com,
	viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
	linux-api@vger.kernel.org, dancol@google.com,
	timmurray@google.com, linux-man@vger.kernel.org,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
Date: Tue, 20 Nov 2018 08:23:43 +1100
Message-ID: <20181119212343.yikfoxob7f4hio7h@yavin> (raw)
In-Reply-To: <20181119211810.73ptfhnwdmkngfi4@yavin>


[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On 2018-11-20, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> > > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > > +	if (info) {
> > > > +		ret = __copy_siginfo_from_user(sig, &kinfo, info);
> > > > +		if (unlikely(ret))
> > > > +			goto err;
> > > > +		/*
> > > > +		 * Not even root can pretend to send signals from the kernel.
> > > > +		 * Nor can they impersonate a kill()/tgkill(), which adds
> > > > +		 * source info.
> > > > +		 */
> > > > +		ret = -EPERM;
> > > > +		if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
> > > > +		    (task_pid(current) != pid))
> > > > +			goto err;
> > > > +	} else {
> > > > +		prepare_kill_siginfo(sig, &kinfo);
> > > > +	}
> > > 
> > > I wonder whether we should also have a pidns restriction here, since
> > > currently it isn't possible for a container process using a pidns to
> > > signal processes outside its pidns. AFAICS, this isn't done through an
> > > explicit check -- it's a side-effect of processes in a pidns not being
> > > able to address non-descendant-pidns processes.
> > > 
> > > But maybe it's reasonable to allow sending a procfd to a different pidns
> > > and the same operations working on it? If we extend the procfd API to
> > 
> > No, I don't think so. I really don't want any fancy semantics in here.
> > Fancy doesn't get merged and fancy is hard to maintain. So we should do
> > something like:
> > 
> > if (proc_pid_ns() != current_pid_ns)
> > 	return EINVAL
> 
> This isn't quite sufficient. The key thing is that you have to be in an
> *ancestor* (or same) pidns, not the *same* pidns. Ideally you can re-use
> the check already in pidns_get_parent, and expose it. It would be
> something as trivial as:
> 
> bool pidns_is_descendant(struct pid_namespace *ns,
>                          struct pid_namespace *ancestor)
> {
>     for (;;) {
>         if (!ns)
>             return false;
>         if (ns == ancestor)
>             break;
>         ns = ns->parent;
>     }
>     return true;
> }
> 
> And you can rewrite pidns_get_parent to use it. So you would instead be
> doing:
> 
>     if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
>         return -EPERM;

Scratch the last bit, -EPERM is wrong here. I would argue that -EINVAL
is *somewhat* wrong because arguable the more semantically consistent
error (with kill(2)) would be -ESRCH -- but then you're mixing the "pid
is dead" and "pid is not visible to you" cases. I'm not sure what the
right errno would be here (I'm sure some of the LKML greybeards will
have a better clue.) :P

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-19 10:32 [PATCH v1 0/2] proc: allow signaling processes via file descriptors Christian Brauner
2018-11-19 10:32 ` [PATCH v1 1/2] proc: get process file descriptor from /proc/<pid> Christian Brauner
2018-11-19 15:32   ` Andy Lutomirski
2018-11-19 18:20     ` Christian Brauner
2018-11-19 10:32 ` [PATCH v1 2/2] signal: add procfd_signal() syscall Christian Brauner
2018-11-19 15:45   ` Andy Lutomirski
2018-11-19 15:57     ` Daniel Colascione
2018-11-19 18:39     ` Christian Brauner
2018-11-19 15:59   ` Daniel Colascione
2018-11-19 18:29     ` Christian Brauner
2018-11-19 19:02       ` Eric W. Biederman
2018-11-19 19:31         ` Christian Brauner
2018-11-19 19:39           ` Daniel Colascione
2018-11-19 17:10   ` Eugene Syromiatnikov
2018-11-19 18:23     ` Christian Brauner
2018-11-19 17:14   ` Eugene Syromiatnikov
2018-11-19 20:28   ` Aleksa Sarai
2018-11-19 20:55     ` Christian Brauner
2018-11-19 21:13       ` Christian Brauner
2018-11-19 21:18       ` Aleksa Sarai
2018-11-19 21:20         ` Christian Brauner
2018-11-19 21:21         ` Christian Brauner
2018-11-19 21:25           ` Aleksa Sarai
2018-11-19 21:26           ` Daniel Colascione
2018-11-19 21:36             ` Aleksa Sarai
2018-11-19 21:37             ` Christian Brauner
2018-11-19 21:41               ` Daniel Colascione
2018-11-20  4:59                 ` Eric W. Biederman
2018-11-20 10:31                   ` Christian Brauner
2018-11-21 21:39                     ` Serge E. Hallyn
2018-11-19 21:23         ` Aleksa Sarai [this message]
2018-11-22  7:41           ` Serge E. Hallyn
2018-11-19 22:39   ` Tycho Andersen
2018-11-19 22:49     ` Daniel Colascione
2018-11-19 23:07       ` Tycho Andersen
2018-11-20  0:27         ` Andy Lutomirski
2018-11-20  0:32           ` Christian Brauner
2018-11-20  0:34             ` Andy Lutomirski
2018-11-20  0:49           ` Daniel Colascione
2018-11-22  7:48     ` Serge E. Hallyn
2018-11-19 23:35   ` kbuild test robot
2018-11-19 23:37   ` kbuild test robot
2018-11-19 23:45     ` Christian Brauner
2018-11-28 21:45   ` Joey Pabalinas
2018-11-28 22:05     ` Christian Brauner
2018-11-28 23:02       ` Joey Pabalinas
2018-11-19 10:32 ` [PATCH] procfd_signal.2: document procfd_signal syscall Christian Brauner
2018-11-20 13:29   ` Michael Kerrisk (man-pages)
2018-11-28 20:59   ` Florian Weimer
2018-11-28 21:12     ` Christian Brauner

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=20181119212343.yikfoxob7f4hio7h@yavin \
    --to=cyphar@cyphar.com \
    --cc=akpm@linux-foundation.org \
    --cc=christian@brauner.io \
    --cc=dancol@google.com \
    --cc=ebiederm@xmission.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=linux-man@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 \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git