kill_pid_info_as_uid() is solely used by drivers/usb/core/. The original
"[PATCH] Fix signal sending in usbdevio on async URB completion" commit
46113830a18847cff8da73005e57bc49c2f95a56 was right, but nowadays we use
struct pid and this solves most of the addressed problems.
It would be nice to use kill_pid_info() instead, but we can't because USB
uses .si_code = SI_ASYNCIO which fools SI_FROMUSER() and thus security checks.
I think we should omit the permission checks completely, the task which does
ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we should
not deny the signal even if the task changes its credentials in any way.
For now, we can remove security_task_kill(). It is bogus, the signal has come
from kernel.
Signed-off-by: Oleg Nesterov <[email protected]>
include/linux/sched.h | 2 +-
kernel/signal.c | 5 +----
drivers/usb/core/usb.h | 1 -
drivers/usb/core/devio.c | 7 ++-----
drivers/usb/core/inode.c | 3 ++-
5 files changed, 6 insertions(+), 12 deletions(-)
--- 25/include/linux/sched.h~2_KPIAU_NO_LSM 2008-02-15 16:59:17.000000000 +0300
+++ 25/include/linux/sched.h 2008-02-25 19:13:05.000000000 +0300
@@ -1690,7 +1690,7 @@ extern int force_sigsegv(int, struct tas
extern int force_sig_info(int, struct siginfo *, struct task_struct *);
extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
-extern int kill_pid_info_as_uid(int, struct siginfo *, struct pid *, uid_t, uid_t, u32);
+extern int kill_pid_info_as_uid(int, struct siginfo *, struct pid *, uid_t, uid_t);
extern int kill_pgrp(struct pid *pid, int sig, int priv);
extern int kill_pid(struct pid *pid, int sig, int priv);
extern int kill_proc_info(int, struct siginfo *, pid_t);
--- 25/kernel/signal.c~2_KPIAU_NO_LSM 2008-02-25 18:15:38.000000000 +0300
+++ 25/kernel/signal.c 2008-02-25 19:13:56.000000000 +0300
@@ -1078,7 +1078,7 @@ kill_proc_info(int sig, struct siginfo *
/* like kill_pid_info(), but doesn't use uid/euid of "current" */
int kill_pid_info_as_uid(int sig, struct siginfo *info, struct pid *pid,
- uid_t uid, uid_t euid, u32 secid)
+ uid_t uid, uid_t euid)
{
int ret = -EINVAL;
struct task_struct *p;
@@ -1098,9 +1098,6 @@ int kill_pid_info_as_uid(int sig, struct
ret = -EPERM;
goto out_unlock;
}
- ret = security_task_kill(p, info, sig, secid);
- if (ret)
- goto out_unlock;
if (sig && p->sighand) {
unsigned long flags;
spin_lock_irqsave(&p->sighand->siglock, flags);
--- 25/drivers/usb/core/usb.h~2_KPIAU_NO_LSM 2008-02-15 16:59:10.000000000 +0300
+++ 25/drivers/usb/core/usb.h 2008-02-25 19:16:37.000000000 +0300
@@ -155,7 +155,6 @@ struct dev_state {
uid_t disc_uid, disc_euid;
void __user *disccontext;
unsigned long ifclaimed;
- u32 secid;
};
/* internal notify stuff */
--- 25/drivers/usb/core/devio.c~2_KPIAU_NO_LSM 2008-02-15 16:59:09.000000000 +0300
+++ 25/drivers/usb/core/devio.c 2008-02-25 19:19:55.000000000 +0300
@@ -72,7 +72,6 @@ struct async {
void __user *userurb;
struct urb *urb;
int status;
- u32 secid;
};
static int usbfs_snoop;
@@ -321,8 +320,8 @@ static void async_completed(struct urb *
sinfo.si_errno = as->status;
sinfo.si_code = SI_ASYNCIO;
sinfo.si_addr = as->userurb;
- kill_pid_info_as_uid(as->signr, &sinfo, as->pid, as->uid,
- as->euid, as->secid);
+ kill_pid_info_as_uid(as->signr, &sinfo, as->pid,
+ as->uid, as->euid);
}
snoop(&urb->dev->dev, "urb complete\n");
snoop_urb(urb, as->userurb);
@@ -603,7 +602,6 @@ static int usbdev_open(struct inode *ino
ps->disc_euid = current->euid;
ps->disccontext = NULL;
ps->ifclaimed = 0;
- security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
file->private_data = ps;
@@ -1132,7 +1130,6 @@ static int proc_do_submiturb(struct dev_
as->pid = get_pid(task_pid(current));
as->uid = current->uid;
as->euid = current->euid;
- security_task_getsecid(current, &as->secid);
if (!is_in) {
if (copy_from_user(as->urb->transfer_buffer, uurb->buffer,
as->urb->transfer_buffer_length)) {
--- 25/drivers/usb/core/inode.c~2_KPIAU_NO_LSM 2008-02-15 16:59:09.000000000 +0300
+++ 25/drivers/usb/core/inode.c 2008-02-25 19:21:09.000000000 +0300
@@ -728,7 +728,8 @@ static void usbfs_remove_device(struct u
sinfo.si_errno = EPIPE;
sinfo.si_code = SI_ASYNCIO;
sinfo.si_addr = ds->disccontext;
- kill_pid_info_as_uid(ds->discsignr, &sinfo, ds->disc_pid, ds->disc_uid, ds->disc_euid, ds->secid);
+ kill_pid_info_as_uid(ds->discsignr, &sinfo, ds->disc_pid,
+ ds->disc_uid, ds->disc_euid);
}
}
}
Quoting Oleg Nesterov ([email protected]):
> kill_pid_info_as_uid() is solely used by drivers/usb/core/. The original
> "[PATCH] Fix signal sending in usbdevio on async URB completion" commit
> 46113830a18847cff8da73005e57bc49c2f95a56 was right, but nowadays we use
> struct pid and this solves most of the addressed problems.
>
> It would be nice to use kill_pid_info() instead, but we can't because USB
> uses .si_code = SI_ASYNCIO which fools SI_FROMUSER() and thus security checks.
>
> I think we should omit the permission checks completely, the task which does
> ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we should
> not deny the signal even if the task changes its credentials in any way.
>
> For now, we can remove security_task_kill(). It is bogus, the signal has come
> from kernel.
Ok, could you augment the comment above
kernel/signal.c:kill_pid_info_as_uid() to say that it is only called
from within the usb subsystem?
It also would be nice if some sparse magic could check that that remains
the case, but that's 'vali' territory and the comment should suffice.
thanks,
-serge
> Signed-off-by: Oleg Nesterov <[email protected]>
>
> include/linux/sched.h | 2 +-
> kernel/signal.c | 5 +----
> drivers/usb/core/usb.h | 1 -
> drivers/usb/core/devio.c | 7 ++-----
> drivers/usb/core/inode.c | 3 ++-
> 5 files changed, 6 insertions(+), 12 deletions(-)
>
> --- 25/include/linux/sched.h~2_KPIAU_NO_LSM 2008-02-15 16:59:17.000000000 +0300
> +++ 25/include/linux/sched.h 2008-02-25 19:13:05.000000000 +0300
> @@ -1690,7 +1690,7 @@ extern int force_sigsegv(int, struct tas
> extern int force_sig_info(int, struct siginfo *, struct task_struct *);
> extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
> extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
> -extern int kill_pid_info_as_uid(int, struct siginfo *, struct pid *, uid_t, uid_t, u32);
> +extern int kill_pid_info_as_uid(int, struct siginfo *, struct pid *, uid_t, uid_t);
> extern int kill_pgrp(struct pid *pid, int sig, int priv);
> extern int kill_pid(struct pid *pid, int sig, int priv);
> extern int kill_proc_info(int, struct siginfo *, pid_t);
> --- 25/kernel/signal.c~2_KPIAU_NO_LSM 2008-02-25 18:15:38.000000000 +0300
> +++ 25/kernel/signal.c 2008-02-25 19:13:56.000000000 +0300
> @@ -1078,7 +1078,7 @@ kill_proc_info(int sig, struct siginfo *
>
> /* like kill_pid_info(), but doesn't use uid/euid of "current" */
> int kill_pid_info_as_uid(int sig, struct siginfo *info, struct pid *pid,
> - uid_t uid, uid_t euid, u32 secid)
> + uid_t uid, uid_t euid)
> {
> int ret = -EINVAL;
> struct task_struct *p;
> @@ -1098,9 +1098,6 @@ int kill_pid_info_as_uid(int sig, struct
> ret = -EPERM;
> goto out_unlock;
> }
> - ret = security_task_kill(p, info, sig, secid);
> - if (ret)
> - goto out_unlock;
> if (sig && p->sighand) {
> unsigned long flags;
> spin_lock_irqsave(&p->sighand->siglock, flags);
> --- 25/drivers/usb/core/usb.h~2_KPIAU_NO_LSM 2008-02-15 16:59:10.000000000 +0300
> +++ 25/drivers/usb/core/usb.h 2008-02-25 19:16:37.000000000 +0300
> @@ -155,7 +155,6 @@ struct dev_state {
> uid_t disc_uid, disc_euid;
> void __user *disccontext;
> unsigned long ifclaimed;
> - u32 secid;
> };
>
> /* internal notify stuff */
> --- 25/drivers/usb/core/devio.c~2_KPIAU_NO_LSM 2008-02-15 16:59:09.000000000 +0300
> +++ 25/drivers/usb/core/devio.c 2008-02-25 19:19:55.000000000 +0300
> @@ -72,7 +72,6 @@ struct async {
> void __user *userurb;
> struct urb *urb;
> int status;
> - u32 secid;
> };
>
> static int usbfs_snoop;
> @@ -321,8 +320,8 @@ static void async_completed(struct urb *
> sinfo.si_errno = as->status;
> sinfo.si_code = SI_ASYNCIO;
> sinfo.si_addr = as->userurb;
> - kill_pid_info_as_uid(as->signr, &sinfo, as->pid, as->uid,
> - as->euid, as->secid);
> + kill_pid_info_as_uid(as->signr, &sinfo, as->pid,
> + as->uid, as->euid);
> }
> snoop(&urb->dev->dev, "urb complete\n");
> snoop_urb(urb, as->userurb);
> @@ -603,7 +602,6 @@ static int usbdev_open(struct inode *ino
> ps->disc_euid = current->euid;
> ps->disccontext = NULL;
> ps->ifclaimed = 0;
> - security_task_getsecid(current, &ps->secid);
> smp_wmb();
> list_add_tail(&ps->list, &dev->filelist);
> file->private_data = ps;
> @@ -1132,7 +1130,6 @@ static int proc_do_submiturb(struct dev_
> as->pid = get_pid(task_pid(current));
> as->uid = current->uid;
> as->euid = current->euid;
> - security_task_getsecid(current, &as->secid);
> if (!is_in) {
> if (copy_from_user(as->urb->transfer_buffer, uurb->buffer,
> as->urb->transfer_buffer_length)) {
> --- 25/drivers/usb/core/inode.c~2_KPIAU_NO_LSM 2008-02-15 16:59:09.000000000 +0300
> +++ 25/drivers/usb/core/inode.c 2008-02-25 19:21:09.000000000 +0300
> @@ -728,7 +728,8 @@ static void usbfs_remove_device(struct u
> sinfo.si_errno = EPIPE;
> sinfo.si_code = SI_ASYNCIO;
> sinfo.si_addr = ds->disccontext;
> - kill_pid_info_as_uid(ds->discsignr, &sinfo, ds->disc_pid, ds->disc_uid, ds->disc_euid, ds->secid);
> + kill_pid_info_as_uid(ds->discsignr, &sinfo, ds->disc_pid,
> + ds->disc_uid, ds->disc_euid);
> }
> }
> }
On 02/25, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov ([email protected]):
> > kill_pid_info_as_uid() is solely used by drivers/usb/core/. The original
> > "[PATCH] Fix signal sending in usbdevio on async URB completion" commit
> > 46113830a18847cff8da73005e57bc49c2f95a56 was right, but nowadays we use
> > struct pid and this solves most of the addressed problems.
> >
> > It would be nice to use kill_pid_info() instead, but we can't because USB
> > uses .si_code = SI_ASYNCIO which fools SI_FROMUSER() and thus security checks.
> >
> > I think we should omit the permission checks completely, the task which does
> > ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we should
> > not deny the signal even if the task changes its credentials in any way.
> >
> > For now, we can remove security_task_kill(). It is bogus, the signal has come
> > from kernel.
>
> Ok, could you augment the comment above
> kernel/signal.c:kill_pid_info_as_uid() to say that it is only called
> from within the usb subsystem?
OK, will do a bit later. I still hope we can kill this helper or at least
cleanup it further.
What we need in fact is kill_pid_info_skip_check_kill_permission(), and
only because USB uses SI_ASYNCIO < 0.
Oleg.
On Mon, 2008-02-25 at 20:42 +0300, Oleg Nesterov wrote:
> kill_pid_info_as_uid() is solely used by drivers/usb/core/. The original
> "[PATCH] Fix signal sending in usbdevio on async URB completion" commit
> 46113830a18847cff8da73005e57bc49c2f95a56 was right, but nowadays we use
> struct pid and this solves most of the addressed problems.
>
> It would be nice to use kill_pid_info() instead, but we can't because USB
> uses .si_code = SI_ASYNCIO which fools SI_FROMUSER() and thus security checks.
>
> I think we should omit the permission checks completely, the task which does
> ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we should
> not deny the signal even if the task changes its credentials in any way.
If we are applying checks based on uid/gid to protect suid/sgid
programs, then we ought to also invoke the LSM hook to allow protection
of other credential-changing transformations, like SELinux context
transitions. You either remove all checking or none, please. And if
all, what's the rationale?
> For now, we can remove security_task_kill(). It is bogus, the signal has come
> from kernel.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
>
> include/linux/sched.h | 2 +-
> kernel/signal.c | 5 +----
> drivers/usb/core/usb.h | 1 -
> drivers/usb/core/devio.c | 7 ++-----
> drivers/usb/core/inode.c | 3 ++-
> 5 files changed, 6 insertions(+), 12 deletions(-)
>
> --- 25/include/linux/sched.h~2_KPIAU_NO_LSM 2008-02-15 16:59:17.000000000 +0300
> +++ 25/include/linux/sched.h 2008-02-25 19:13:05.000000000 +0300
> @@ -1690,7 +1690,7 @@ extern int force_sigsegv(int, struct tas
> extern int force_sig_info(int, struct siginfo *, struct task_struct *);
> extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
> extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
> -extern int kill_pid_info_as_uid(int, struct siginfo *, struct pid *, uid_t, uid_t, u32);
> +extern int kill_pid_info_as_uid(int, struct siginfo *, struct pid *, uid_t, uid_t);
> extern int kill_pgrp(struct pid *pid, int sig, int priv);
> extern int kill_pid(struct pid *pid, int sig, int priv);
> extern int kill_proc_info(int, struct siginfo *, pid_t);
> --- 25/kernel/signal.c~2_KPIAU_NO_LSM 2008-02-25 18:15:38.000000000 +0300
> +++ 25/kernel/signal.c 2008-02-25 19:13:56.000000000 +0300
> @@ -1078,7 +1078,7 @@ kill_proc_info(int sig, struct siginfo *
>
> /* like kill_pid_info(), but doesn't use uid/euid of "current" */
> int kill_pid_info_as_uid(int sig, struct siginfo *info, struct pid *pid,
> - uid_t uid, uid_t euid, u32 secid)
> + uid_t uid, uid_t euid)
> {
> int ret = -EINVAL;
> struct task_struct *p;
> @@ -1098,9 +1098,6 @@ int kill_pid_info_as_uid(int sig, struct
> ret = -EPERM;
> goto out_unlock;
> }
> - ret = security_task_kill(p, info, sig, secid);
> - if (ret)
> - goto out_unlock;
> if (sig && p->sighand) {
> unsigned long flags;
> spin_lock_irqsave(&p->sighand->siglock, flags);
> --- 25/drivers/usb/core/usb.h~2_KPIAU_NO_LSM 2008-02-15 16:59:10.000000000 +0300
> +++ 25/drivers/usb/core/usb.h 2008-02-25 19:16:37.000000000 +0300
> @@ -155,7 +155,6 @@ struct dev_state {
> uid_t disc_uid, disc_euid;
> void __user *disccontext;
> unsigned long ifclaimed;
> - u32 secid;
> };
>
> /* internal notify stuff */
> --- 25/drivers/usb/core/devio.c~2_KPIAU_NO_LSM 2008-02-15 16:59:09.000000000 +0300
> +++ 25/drivers/usb/core/devio.c 2008-02-25 19:19:55.000000000 +0300
> @@ -72,7 +72,6 @@ struct async {
> void __user *userurb;
> struct urb *urb;
> int status;
> - u32 secid;
> };
>
> static int usbfs_snoop;
> @@ -321,8 +320,8 @@ static void async_completed(struct urb *
> sinfo.si_errno = as->status;
> sinfo.si_code = SI_ASYNCIO;
> sinfo.si_addr = as->userurb;
> - kill_pid_info_as_uid(as->signr, &sinfo, as->pid, as->uid,
> - as->euid, as->secid);
> + kill_pid_info_as_uid(as->signr, &sinfo, as->pid,
> + as->uid, as->euid);
> }
> snoop(&urb->dev->dev, "urb complete\n");
> snoop_urb(urb, as->userurb);
> @@ -603,7 +602,6 @@ static int usbdev_open(struct inode *ino
> ps->disc_euid = current->euid;
> ps->disccontext = NULL;
> ps->ifclaimed = 0;
> - security_task_getsecid(current, &ps->secid);
> smp_wmb();
> list_add_tail(&ps->list, &dev->filelist);
> file->private_data = ps;
> @@ -1132,7 +1130,6 @@ static int proc_do_submiturb(struct dev_
> as->pid = get_pid(task_pid(current));
> as->uid = current->uid;
> as->euid = current->euid;
> - security_task_getsecid(current, &as->secid);
> if (!is_in) {
> if (copy_from_user(as->urb->transfer_buffer, uurb->buffer,
> as->urb->transfer_buffer_length)) {
> --- 25/drivers/usb/core/inode.c~2_KPIAU_NO_LSM 2008-02-15 16:59:09.000000000 +0300
> +++ 25/drivers/usb/core/inode.c 2008-02-25 19:21:09.000000000 +0300
> @@ -728,7 +728,8 @@ static void usbfs_remove_device(struct u
> sinfo.si_errno = EPIPE;
> sinfo.si_code = SI_ASYNCIO;
> sinfo.si_addr = ds->disccontext;
> - kill_pid_info_as_uid(ds->discsignr, &sinfo, ds->disc_pid, ds->disc_uid, ds->disc_euid, ds->secid);
> + kill_pid_info_as_uid(ds->discsignr, &sinfo, ds->disc_pid,
> + ds->disc_uid, ds->disc_euid);
> }
> }
> }
--
Stephen Smalley
National Security Agency
On 02/25, Stephen Smalley wrote:
>
> On Mon, 2008-02-25 at 20:42 +0300, Oleg Nesterov wrote:
> > kill_pid_info_as_uid() is solely used by drivers/usb/core/. The original
> > "[PATCH] Fix signal sending in usbdevio on async URB completion" commit
> > 46113830a18847cff8da73005e57bc49c2f95a56 was right, but nowadays we use
> > struct pid and this solves most of the addressed problems.
> >
> > It would be nice to use kill_pid_info() instead, but we can't because USB
> > uses .si_code = SI_ASYNCIO which fools SI_FROMUSER() and thus security checks.
> >
> > I think we should omit the permission checks completely, the task which does
> > ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we should
> > not deny the signal even if the task changes its credentials in any way.
>
> If we are applying checks based on uid/gid to protect suid/sgid
> programs, then we ought to also invoke the LSM hook to allow protection
> of other credential-changing transformations, like SELinux context
> transitions. You either remove all checking or none, please.
Yes, you are right. I'd like to remove all uid/euid checks. This patch doesn't
do this because
- perhaps it will be possible to kill this helper
- if we remove these checks, we should do some subsequent cleanups
in drivers/usb/core/, while this series is all about LSM hooks.
I am going to do this later.
> And if all, what's the rationale?
I think it is not good that LSM has some special (and unneeded!) hacks for USB.
Please also look at the next patch.
Do you agree?
Oleg.
--- Stephen Smalley <[email protected]> wrote:
>
> ...
> >
> > I think we should omit the permission checks completely, the task which
> does
> > ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we
> should
> > not deny the signal even if the task changes its credentials in any way.
>
> If we are applying checks based on uid/gid to protect suid/sgid
> programs, then we ought to also invoke the LSM hook to allow protection
> of other credential-changing transformations, like SELinux context
> transitions. You either remove all checking or none, please. And if
> all, what's the rationale?
Perhaps more important to my mind is what lead the developers of
this code to go to such significant lengths to provide this access
check in the first place. Why was it considered sufficiently important?
I concur that it's an ugly bit of hackery, but someone must have felt
it was necessary or they wouldn't have done it.
Casey Schaufler
[email protected]
On 02/25, Casey Schaufler wrote:
>
> --- Stephen Smalley <[email protected]> wrote:
>
> >
> > ...
> > >
> > > I think we should omit the permission checks completely, the task which
> > does
> > > ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we
> > should
> > > not deny the signal even if the task changes its credentials in any way.
> >
> > If we are applying checks based on uid/gid to protect suid/sgid
> > programs, then we ought to also invoke the LSM hook to allow protection
> > of other credential-changing transformations, like SELinux context
> > transitions. You either remove all checking or none, please. And if
> > all, what's the rationale?
>
> Perhaps more important to my mind is what lead the developers of
> this code to go to such significant lengths to provide this access
> check in the first place. Why was it considered sufficiently important?
> I concur that it's an ugly bit of hackery, but someone must have felt
> it was necessary or they wouldn't have done it.
Previously, USB used "pid_t pid" as a target for the signal. This pid
could be reused by the completely unrelated process, these checks ensure
that we at least doesn't kill the wrong user in this case.
Now we are using "struct pid", this means that the signal always goes
to the "right" task.
Oleg.
On 02/25, Oleg Nesterov wrote:
>
> On 02/25, Stephen Smalley wrote:
> >
> > On Mon, 2008-02-25 at 20:42 +0300, Oleg Nesterov wrote:
> > > kill_pid_info_as_uid() is solely used by drivers/usb/core/. The original
> > > "[PATCH] Fix signal sending in usbdevio on async URB completion" commit
> > > 46113830a18847cff8da73005e57bc49c2f95a56 was right, but nowadays we use
> > > struct pid and this solves most of the addressed problems.
> > >
> > > It would be nice to use kill_pid_info() instead, but we can't because USB
> > > uses .si_code = SI_ASYNCIO which fools SI_FROMUSER() and thus security checks.
> > >
> > > I think we should omit the permission checks completely, the task which does
> > > ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, we should
> > > not deny the signal even if the task changes its credentials in any way.
> >
> > If we are applying checks based on uid/gid to protect suid/sgid
> > programs, then we ought to also invoke the LSM hook to allow protection
> > of other credential-changing transformations, like SELinux context
> > transitions. You either remove all checking or none, please.
>
> Yes, you are right. I'd like to remove all uid/euid checks.
Actually, I may be wrong, sorry. If the task does setuid exec, we probably
should do these checks.
OK, please ignore 2nd and 3rd patches.
Still. The usage of security_task_kill(secid) doesn't look good, imho.
We have the similar issues with send_sigio(). In that case we use
security_file_send_sigiotask(), not security_task_kill().
Oleg.