LKML Archive on
help / color / mirror / Atom feed
From: Oleg Nesterov <>
To: "Eric W. Biederman" <>,
	Harald Welte <>
Cc: Andrew Morton <>,
	Pavel Emelyanov <>,
Subject: Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
Date: Sun, 24 Feb 2008 21:09:31 +0300	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 02/23, Eric W. Biederman wrote:
> Andrew Morton <> writes:
> > um, is that code namespace-clean?
> Choke, gag.
> There are uid namespace issues but since no one has finished the
> uid namespace that I am aware of that is minor.
> However the code does not appear clean/maintainable.  The normal linux
> signal sending policy has already been enforce before we get to this
> point.
> So unless I am totally mistaken the code should read:
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid)
> {
>         if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
>                 return 0;
>         if (!cap_issubset(p->cap_permitted, current->cap_permitted))
>                 return -EPERM;
> 	return 0;
> }
> Although doing it that way violates:
>        /*
>          * Running a setuid root program raises your capabilities.
>          * Killing your own setuid root processes was previously
>          * allowed.
>          * We must preserve legacy signal behavior in this case.
>          */
> Which says to me the code should really read:
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid)
> {
> 	return 0;
> }
> The entire point of defining cap_task_kill under
> CONFIG_SECURITY_FILE_CAPABLITIES appears to be deny killing processes
> with more caps.  Killing processes that we could ordinarily kill 
> which have more caps appears to be precisely the case we have decided
> to allow.  So the patched version of cap_task_kill appears to be an
> expensive way of doing nothing, just waiting for someone to complain
> about the last couple of cases it denies until it is truly a noop.

(Can't comment, I never understood this security magic, but Eric's
 explanation looks very reasonable to me).

I just have an almost off-topic (sorry ;) question. Do we really need
kill_pid_info_as_uid() ? Harald Welte cc'ed.

>From "[PATCH] Fix signal sending in usbdevio on async URB completion"
commit 46113830a18847cff8da73005e57bc49c2f95a56

	> If a process issues an URB from userspace and (starts to) terminate
	> before the URB comes back, we run into the issue described above.  This
	> is because the urb saves a pointer to "current" when it is posted to the
	> device, but there's no guarantee that this pointer is still valid
	> afterwards.
	> In fact, there are three separate issues:
	> 1) the pointer to "current" can become invalid, since the task could be
	>    completely gone when the URB completion comes back from the device.
	> 2) Even if the saved task pointer is still pointing to a valid task_struct,
	>    task_struct->sighand could have gone meanwhile.
	> 3) Even if the process is perfectly fine, permissions may have changed,
	>    and we can no longer send it a signal.

The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real
problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send
the signal to it, should we deny the signal even if it changes its credentials
in some way?

Just curious. Thanks,


  reply	other threads:[~2008-02-24 18:05 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
2008-02-24  6:50 ` Eric W. Biederman
2008-02-24 18:09   ` Oleg Nesterov [this message]
2008-02-24 21:08     ` Harald Welte
2008-02-25 18:23       ` Oleg Nesterov
2008-02-27  4:18   ` serge
2008-02-27  4:33   ` serge
2008-02-28 20:25     ` Eric W. Biederman
2008-02-28 21:35       ` serge

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:

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

  git send-email \ \ \ \ \ \ \ \
    --subject='Re: Fw: [PATCH 1/1] file capabilities: simplify signal check' \

* 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
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).