* [PATCH] pid_ns: ensure pid is not freed during kill_pid_info_as_uid @ 2011-09-26 15:18 Serge Hallyn 2011-09-26 15:45 ` [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) Serge Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Serge Hallyn @ 2011-09-26 15:18 UTC (permalink / raw) To: Alan Stern Cc: Serge E. Hallyn, Greg KH, Oleg Nesterov, lkml, richard, Andrew Morton, Eric W. Biederman, Tejun Heo, linux-usb, Serge E. Hallyn Alan Stern points out that after spin_unlock(&ps->lock) there is no guarantee that ps->pid won't be freed. Since kill_pid_info_as_uid() is called after the spin_unlock(), the pid passed to it must be pinned. Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- drivers/usb/core/devio.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 37518df..eea53eb 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -407,7 +407,7 @@ static void async_completed(struct urb *urb) sinfo.si_errno = as->status; sinfo.si_code = SI_ASYNCIO; sinfo.si_addr = as->userurb; - pid = as->pid; + pid = get_pid(as->pid); uid = as->uid; euid = as->euid; secid = as->secid; @@ -422,9 +422,11 @@ static void async_completed(struct urb *urb) cancel_bulk_urbs(ps, as->bulk_addr); spin_unlock(&ps->lock); - if (signr) + if (signr) { kill_pid_info_as_uid(sinfo.si_signo, &sinfo, pid, uid, euid, secid); + put_pid(pid); + } wake_up(&ps->wait); } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) 2011-09-26 15:18 [PATCH] pid_ns: ensure pid is not freed during kill_pid_info_as_uid Serge Hallyn @ 2011-09-26 15:45 ` Serge Hallyn 2011-09-26 23:45 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Serge Hallyn @ 2011-09-26 15:45 UTC (permalink / raw) To: Alan Stern Cc: Serge E. Hallyn, Greg KH, Oleg Nesterov, lkml, richard, Andrew Morton, Eric W. Biederman, Tejun Heo, linux-usb Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check. Changelog: Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace, uid, and euid each separately, pass a struct cred. Sep 26: Address Alan Stern's comments: don't define a struct cred at usbdev_open(), and take and put a cred at async_completed() to ensure it lasts for the duration of kill_pid_info_as_cred(). Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> Cc: Greg KH <greg@kroah.com> Cc: Oleg Nesterov <oleg@redhat.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Tejun Heo <tj@kernel.org> --- drivers/usb/core/devio.c | 30 +++++++++++++----------------- include/linux/sched.h | 3 ++- kernel/signal.c | 24 ++++++++++++++++-------- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index eea53eb..000c25d 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -46,6 +46,7 @@ #include <linux/cdev.h> #include <linux/notifier.h> #include <linux/security.h> +#include <linux/user_namespace.h> #include <asm/uaccess.h> #include <asm/byteorder.h> #include <linux/moduleparam.h> @@ -68,7 +69,7 @@ struct dev_state { wait_queue_head_t wait; /* wake up if a request completed */ unsigned int discsignr; struct pid *disc_pid; - uid_t disc_uid, disc_euid; + const struct cred *cred; void __user *disccontext; unsigned long ifclaimed; u32 secid; @@ -79,7 +80,7 @@ struct async { struct list_head asynclist; struct dev_state *ps; struct pid *pid; - uid_t uid, euid; + const struct cred *cred; unsigned int signr; unsigned int ifnum; void __user *userbuffer; @@ -248,6 +249,7 @@ static struct async *alloc_async(unsigned int numisoframes) static void free_async(struct async *as) { put_pid(as->pid); + put_cred(as->cred); kfree(as->urb->transfer_buffer); kfree(as->urb->setup_packet); usb_free_urb(as->urb); @@ -393,9 +395,8 @@ static void async_completed(struct urb *urb) struct dev_state *ps = as->ps; struct siginfo sinfo; struct pid *pid = NULL; - uid_t uid = 0; - uid_t euid = 0; u32 secid = 0; + const struct cred *cred = NULL; int signr; spin_lock(&ps->lock); @@ -408,8 +409,7 @@ static void async_completed(struct urb *urb) sinfo.si_code = SI_ASYNCIO; sinfo.si_addr = as->userurb; pid = get_pid(as->pid); - uid = as->uid; - euid = as->euid; + cred = get_cred(as->cred); secid = as->secid; } snoop(&urb->dev->dev, "urb complete\n"); @@ -423,9 +423,9 @@ static void async_completed(struct urb *urb) spin_unlock(&ps->lock); if (signr) { - kill_pid_info_as_uid(sinfo.si_signo, &sinfo, pid, uid, - euid, secid); + kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred, secid); put_pid(pid); + put_cred(cred); } wake_up(&ps->wait); @@ -658,7 +658,6 @@ static int usbdev_open(struct inode *inode, struct file *file) { struct usb_device *dev = NULL; struct dev_state *ps; - const struct cred *cred = current_cred(); int ret; ret = -ENOMEM; @@ -708,8 +707,7 @@ static int usbdev_open(struct inode *inode, struct file *file) init_waitqueue_head(&ps->wait); ps->discsignr = 0; ps->disc_pid = get_pid(task_pid(current)); - ps->disc_uid = cred->uid; - ps->disc_euid = cred->euid; + ps->cred = get_current_cred(); ps->disccontext = NULL; ps->ifclaimed = 0; security_task_getsecid(current, &ps->secid); @@ -751,6 +749,7 @@ static int usbdev_release(struct inode *inode, struct file *file) usb_unlock_device(dev); usb_put_dev(dev); put_pid(ps->disc_pid); + put_cred(ps->cred); as = async_getcompleted(ps); while (as) { @@ -1050,7 +1049,6 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb, struct usb_host_endpoint *ep; struct async *as; struct usb_ctrlrequest *dr = NULL; - const struct cred *cred = current_cred(); unsigned int u, totlen, isofrmlen; int ret, ifnum = -1; int is_in; @@ -1264,8 +1262,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb, as->signr = uurb->signr; as->ifnum = ifnum; as->pid = get_pid(task_pid(current)); - as->uid = cred->uid; - as->euid = cred->euid; + as->cred = get_current_cred(); security_task_getsecid(current, &as->secid); if (!is_in && uurb->buffer_length > 0) { if (copy_from_user(as->urb->transfer_buffer, uurb->buffer, @@ -1983,9 +1980,8 @@ static void usbdev_remove(struct usb_device *udev) sinfo.si_errno = EPIPE; sinfo.si_code = SI_ASYNCIO; sinfo.si_addr = ps->disccontext; - kill_pid_info_as_uid(ps->discsignr, &sinfo, - ps->disc_pid, ps->disc_uid, - ps->disc_euid, ps->secid); + kill_pid_info_as_cred(ps->discsignr, &sinfo, + ps->disc_pid, ps->cred, ps->secid); } } } diff --git a/include/linux/sched.h b/include/linux/sched.h index 4ac2c05..57ddb52 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2166,7 +2166,8 @@ extern int force_sigsegv(int, struct task_struct *); 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_cred(int, struct siginfo *, struct pid *, + const struct cred *, u32); 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); diff --git a/kernel/signal.c b/kernel/signal.c index 291c970..d252be2 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1344,13 +1344,24 @@ int kill_proc_info(int sig, struct siginfo *info, pid_t pid) return error; } +static int kill_as_cred_perm(const struct cred *cred, + struct task_struct *target) +{ + const struct cred *pcred = __task_cred(target); + if (cred->user_ns != pcred->user_ns) + return 0; + if (cred->euid != pcred->suid && cred->euid != pcred->uid && + cred->uid != pcred->suid && cred->uid != pcred->uid) + return 0; + return 1; +} + /* 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) +int kill_pid_info_as_cred(int sig, struct siginfo *info, struct pid *pid, + const struct cred *cred, u32 secid) { int ret = -EINVAL; struct task_struct *p; - const struct cred *pcred; unsigned long flags; if (!valid_signal(sig)) @@ -1362,10 +1373,7 @@ int kill_pid_info_as_uid(int sig, struct siginfo *info, struct pid *pid, ret = -ESRCH; goto out_unlock; } - pcred = __task_cred(p); - if (si_fromuser(info) && - euid != pcred->suid && euid != pcred->uid && - uid != pcred->suid && uid != pcred->uid) { + if (si_fromuser(info) && !kill_as_cred_perm(cred, p)) { ret = -EPERM; goto out_unlock; } @@ -1384,7 +1392,7 @@ out_unlock: rcu_read_unlock(); return ret; } -EXPORT_SYMBOL_GPL(kill_pid_info_as_uid); +EXPORT_SYMBOL_GPL(kill_pid_info_as_cred); /* * kill_something_info() interprets pid in interesting ways just like kill(2). -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) 2011-09-26 15:45 ` [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) Serge Hallyn @ 2011-09-26 23:45 ` Greg KH 2011-09-27 12:41 ` Serge E. Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2011-09-26 23:45 UTC (permalink / raw) To: Serge Hallyn Cc: Alan Stern, Serge E. Hallyn, Oleg Nesterov, lkml, richard, Andrew Morton, Eric W. Biederman, Tejun Heo, linux-usb On Mon, Sep 26, 2011 at 10:45:18AM -0500, Serge Hallyn wrote: > Add to the dev_state and alloc_async structures the user namespace > corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), > which can then implement a proper, user-namespace-aware uid check. > > Changelog: > Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace, > uid, and euid each separately, pass a struct cred. > Sep 26: Address Alan Stern's comments: don't define a struct cred at > usbdev_open(), and take and put a cred at async_completed() to > ensure it lasts for the duration of kill_pid_info_as_cred(). > > Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> > Cc: Greg KH <greg@kroah.com> I have no objection to this, is it going to go through your tree, or somewhere else? If so, please add: Acked-by: Greg Kroah-Hartman <gregkh@suse.de> to it. greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) 2011-09-26 23:45 ` Greg KH @ 2011-09-27 12:41 ` Serge E. Hallyn 2011-09-27 14:56 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Serge E. Hallyn @ 2011-09-27 12:41 UTC (permalink / raw) To: Greg KH Cc: Alan Stern, Serge E. Hallyn, Oleg Nesterov, lkml, richard, Andrew Morton, Eric W. Biederman, Tejun Heo, linux-usb Quoting Greg KH (greg@kroah.com): > On Mon, Sep 26, 2011 at 10:45:18AM -0500, Serge Hallyn wrote: > > Add to the dev_state and alloc_async structures the user namespace > > corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), > > which can then implement a proper, user-namespace-aware uid check. > > > > Changelog: > > Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace, > > uid, and euid each separately, pass a struct cred. > > Sep 26: Address Alan Stern's comments: don't define a struct cred at > > usbdev_open(), and take and put a cred at async_completed() to > > ensure it lasts for the duration of kill_pid_info_as_cred(). > > > > Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> > > Cc: Greg KH <greg@kroah.com> > > I have no objection to this, is it going to go through your tree, or > somewhere else? (Silly question from me, but just to make sure - were you asking this of Alan?) > If so, please add: > Acked-by: Greg Kroah-Hartman <gregkh@suse.de> > to it. Thanks, -serge ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) 2011-09-27 12:41 ` Serge E. Hallyn @ 2011-09-27 14:56 ` Greg KH 2011-09-27 15:18 ` Serge Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2011-09-27 14:56 UTC (permalink / raw) To: Serge E. Hallyn Cc: Alan Stern, Serge E. Hallyn, Oleg Nesterov, lkml, richard, Andrew Morton, Eric W. Biederman, Tejun Heo, linux-usb On Tue, Sep 27, 2011 at 07:41:57AM -0500, Serge E. Hallyn wrote: > Quoting Greg KH (greg@kroah.com): > > On Mon, Sep 26, 2011 at 10:45:18AM -0500, Serge Hallyn wrote: > > > Add to the dev_state and alloc_async structures the user namespace > > > corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), > > > which can then implement a proper, user-namespace-aware uid check. > > > > > > Changelog: > > > Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace, > > > uid, and euid each separately, pass a struct cred. > > > Sep 26: Address Alan Stern's comments: don't define a struct cred at > > > usbdev_open(), and take and put a cred at async_completed() to > > > ensure it lasts for the duration of kill_pid_info_as_cred(). > > > > > > Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> > > > Cc: Greg KH <greg@kroah.com> > > > > I have no objection to this, is it going to go through your tree, or > > somewhere else? > > (Silly question from me, but just to make sure - were you asking this of > Alan?) Nope, you. Do you have a tree for this type of namespace work? I haven't been paying attention to it at all. If not, I'll gladly take it myself, I just don't want to cause any merge conflicts anywhere else if at all possible. thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) 2011-09-27 14:56 ` Greg KH @ 2011-09-27 15:18 ` Serge Hallyn 0 siblings, 0 replies; 6+ messages in thread From: Serge Hallyn @ 2011-09-27 15:18 UTC (permalink / raw) To: Greg KH Cc: Alan Stern, Serge E. Hallyn, Oleg Nesterov, lkml, richard, Andrew Morton, Eric W. Biederman, Tejun Heo, linux-usb Quoting Greg KH (greg@kroah.com): > On Tue, Sep 27, 2011 at 07:41:57AM -0500, Serge E. Hallyn wrote: > > Quoting Greg KH (greg@kroah.com): > > > On Mon, Sep 26, 2011 at 10:45:18AM -0500, Serge Hallyn wrote: > > > > Add to the dev_state and alloc_async structures the user namespace > > > > corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), > > > > which can then implement a proper, user-namespace-aware uid check. > > > > > > > > Changelog: > > > > Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace, > > > > uid, and euid each separately, pass a struct cred. > > > > Sep 26: Address Alan Stern's comments: don't define a struct cred at > > > > usbdev_open(), and take and put a cred at async_completed() to > > > > ensure it lasts for the duration of kill_pid_info_as_cred(). > > > > > > > > Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> > > > > Cc: Greg KH <greg@kroah.com> > > > > > > I have no objection to this, is it going to go through your tree, or > > > somewhere else? > > > > (Silly question from me, but just to make sure - were you asking this of > > Alan?) > > Nope, you. Do you have a tree for this type of namespace work? I > haven't been paying attention to it at all. No, at the moment I don't. > If not, I'll gladly take it myself, I just don't want to cause any merge > conflicts anywhere else if at all possible. Great, thanks. -serge ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-27 15:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-26 15:18 [PATCH] pid_ns: ensure pid is not freed during kill_pid_info_as_uid Serge Hallyn 2011-09-26 15:45 ` [PATCH 2/2] user namespace: usb: make usb urbs user namespace aware (v2) Serge Hallyn 2011-09-26 23:45 ` Greg KH 2011-09-27 12:41 ` Serge E. Hallyn 2011-09-27 14:56 ` Greg KH 2011-09-27 15:18 ` Serge Hallyn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox