| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 4 Mar 2015 10:31:50 -0500 (EST)
From: Alan Stern <stern@...land.harvard.edu>
To: Al Viro <viro@...IV.linux.org.uk>
cc: Alexander Holler <holler@...oftware.de>,
Richard Weinberger <richard.weinberger@...il.com>,
USB list <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Felipe Balbi <balbi@...com>
Subject: Re: gadgetfs broken since 7f7f25e8
On Tue, 3 Mar 2015, Al Viro wrote:
> On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote:
> > On Tue, 3 Mar 2015, Al Viro wrote:
> >
> > > Looking at that thing again... why do they need to be dummy? After all,
> > > those methods start with get_ready_ep(), which will fail unless we have
> > > ->state == STATE_EP_ENABLED. So they'd be failing just fine until that
> > > first write() anyway. Let's do the following:
> >
> > In addition to the changes you made, it looks like you will need the
> > following or something similar (also untested). I'm not sure if this
> > is race-free, but it's better than before.
>
> Right, ep0 has the same kind of problem...
>
>
> > @@ -1240,6 +1241,10 @@ static int
> > ep0_fasync (int f, struct file *fd, int on)
> > {
> > struct dev_data *dev = fd->private_data;
> > +
> > + if (dev->state <= STATE_DEV_OPENED)
> > + return -ENODEV;
> > +
>
> Er... What is protecting dev->state here? Matter of fact, what's the
> point of that check at all? Right now you have .fasync = ep0_fasync
> both in ep0_io_operations and in dev_init_operations, so your delta
> changes the existing semantics...
That was a mistake; it shouldn't have been in the patch. I wrote this
rather hastily without doing a careful check at the end.
> On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote:
> > @@ -1279,6 +1284,9 @@ ep0_poll (struct file *fd, poll_table *w
> > struct dev_data *dev = fd->private_data;
> > int mask = 0;
> >
> > + if (dev->state <= STATE_DEV_OPENED)
> > + return -ENODEV;
> > +
> > poll_wait(fd, &dev->wait, wait);
> >
> > spin_lock_irq (&dev->lock);
>
> That, BTW, is wrong. ->poll() does *not* return negatives - to imitate
> "we don't have ->poll()" we need it to return DEFAULT_POLLMASK.
Yes, this should return whatever the default value is when the ->poll
method pointer isn't set. Likewise for the ->read method. I didn't
check to see what those values actually were. It's easy enough to fix
up, though; revised patch below.
Alan Stern
Index: usb-3.19/drivers/usb/gadget/legacy/inode.c
===================================================================
--- usb-3.19.orig/drivers/usb/gadget/legacy/inode.c
+++ usb-3.19/drivers/usb/gadget/legacy/inode.c
@@ -987,6 +987,10 @@ ep0_read (struct file *fd, char __user *
enum ep0_state state;
spin_lock_irq (&dev->lock);
+ if (dev->state <= STATE_DEV_OPENED) {
+ retval = -EINVAL;
+ goto done;
+ }
/* report fd mode change before acting on it */
if (dev->setup_abort) {
@@ -1185,8 +1189,6 @@ ep0_write (struct file *fd, const char _
struct dev_data *dev = fd->private_data;
ssize_t retval = -ESRCH;
- spin_lock_irq (&dev->lock);
-
/* report fd mode change before acting on it */
if (dev->setup_abort) {
dev->setup_abort = 0;
@@ -1232,7 +1234,6 @@ ep0_write (struct file *fd, const char _
} else
DBG (dev, "fail %s, state %d\n", __func__, dev->state);
- spin_unlock_irq (&dev->lock);
return retval;
}
@@ -1279,6 +1280,9 @@ ep0_poll (struct file *fd, poll_table *w
struct dev_data *dev = fd->private_data;
int mask = 0;
+ if (dev->state <= STATE_DEV_OPENED)
+ return DEFAULT_POLLMASK;
+
poll_wait(fd, &dev->wait, wait);
spin_lock_irq (&dev->lock);
@@ -1314,19 +1318,6 @@ static long dev_ioctl (struct file *fd,
return ret;
}
-/* used after device configuration */
-static const struct file_operations ep0_io_operations = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
-
- .read = ep0_read,
- .write = ep0_write,
- .fasync = ep0_fasync,
- .poll = ep0_poll,
- .unlocked_ioctl = dev_ioctl,
- .release = dev_release,
-};
-
/*----------------------------------------------------------------------*/
/* The in-kernel gadget driver handles most ep0 issues, in particular
@@ -1850,6 +1841,14 @@ dev_config (struct file *fd, const char
u32 tag;
char *kbuf;
+ spin_lock_irq(&dev->lock);
+ if (dev->state > STATE_DEV_OPENED) {
+ value = ep0_write(fd, buf, len, ptr);
+ spin_unlock_irq(&dev->lock);
+ return value;
+ }
+ spin_unlock_irq(&dev->lock);
+
if (len < (USB_DT_CONFIG_SIZE + USB_DT_DEVICE_SIZE + 4))
return -EINVAL;
@@ -1923,7 +1922,6 @@ dev_config (struct file *fd, const char
* on, they can work ... except in cleanup paths that
* kick in after the ep0 descriptor is closed.
*/
- fd->f_op = &ep0_io_operations;
value = len;
}
return value;
@@ -1954,12 +1952,14 @@ dev_open (struct inode *inode, struct fi
return value;
}
-static const struct file_operations dev_init_operations = {
+static const struct file_operations ep0_operations = {
.llseek = no_llseek,
.open = dev_open,
+ .read = ep0_read,
.write = dev_config,
.fasync = ep0_fasync,
+ .poll = ep0_poll,
.unlocked_ioctl = dev_ioctl,
.release = dev_release,
};
@@ -2075,7 +2075,7 @@ gadgetfs_fill_super (struct super_block
goto Enomem;
dev->sb = sb;
- dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &dev_init_operations);
+ dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
if (!dev->dentry) {
put_dev(dev);
goto Enomem;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists