lkml.org 
[lkml]   [2011]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH resend] user namespace: usb: make usb urbs user namespace aware (v2)
    On Fri, 23 Sep 2011, Serge E. Hallyn wrote:

    > (re-sending to Cc: Greg and 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.

    This should be broken up into two separate patches: One to add
    kill_pid_info_as_cred() and the other to modify the usbfs driver.

    > --- a/drivers/usb/core/devio.c
    > +++ b/drivers/usb/core/devio.c

    > @@ -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 = as->pid;
    > - uid = as->uid;
    > - euid = as->euid;
    > + cred = as->cred;
    > secid = as->secid;
    > }
    > snoop(&urb->dev->dev, "urb complete\n");
    > @@ -423,8 +423,7 @@ 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);

    This continues a bug that already exists in the current code. Once
    ps->lock is released, there is no guarantee that the async structure
    will still exist. It may already have been freed, and the reference to
    as->cred may already have been dropped. That's why the local copies
    have to be made above. cred shouldn't be a simple copy of as->cred; it
    should also increment the reference count.

    > @@ -706,8 +705,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_cred(cred);

    You might as well get rid of the "cred" local variable. It isn't used
    for anything except this assignment.

    Alan Stern



    \
     
     \ /
      Last update: 2011-09-23 17:51    [W:0.021 / U:90.572 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site