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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
