| 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
| ||
|
Message-Id: <201201151436.29794.oliver@neukum.org>
Date: Sun, 15 Jan 2012 14:36:29 +0100
From: Oliver Neukum <oliver@...kum.org>
To: Bjørn Mork <bjorn@...k.no>
Cc: netdev@...r.kernel.org, linux-usb@...r.kernel.org,
Dan Williams <dcbw@...hat.com>,
Thomas Schäfer <tschaefer@...nline.de>
Subject: Re: [PATCH v2 2/3] net: usb: cdc_enc: New driver exposing USB CDC encapsulated protocols
Am Sonntag, 15. Januar 2012, 07:40:36 schrieb Bjørn Mork:
> USB CDC Ethernet devices may encapsulate vendor specific protocols inside
> USB_CDC_SEND_ENCAPSULATED_COMMAND +USB_CDC_GET_ENCAPSULATED_RESPONSE
> control messages. Devices based on Qualcomm MSM
> chipsets are known to use this for exporting the Qualcomm
> MSM Interface (QMI) protocol. Examples of such devices are
> the Huawei E392 and E398 LTE modems.
The problem is that this is another version of the code already
in cdc-wdm, which is tested well.
I'd really like to unify this stuff.
> +static ssize_t cdc_enc_fops_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
> +{
> + struct cdc_enc_client *client = file->private_data;
> + int ret = -EFAULT;
> +
> + if (!client || !client->cdc_enc)
> + return -ENODEV;
> +
> + while (!test_bit(CDC_ENC_CLIENT_RX, &client->flags)) {/* no data */
> + if (file->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + if (wait_for_completion_interruptible(&client->ready) < 0)
> + return -EINTR;
> +
> + /* shutdown requested? */
> + if (test_bit(CDC_ENC_CLIENT_SHUTDOWN, &client->flags))
> + return -ENXIO;
> + }
> +
> + /* someone else is using our buffer */
> + if (test_and_set_bit(CDC_ENC_CLIENT_BUSY, &client->flags))
> + return -ERESTARTSYS;
> +
> + /* must read a complete packet */
> + if (client->len > size || copy_to_user(buf, client->buf, client->len)) {
This is not nice at all.
> + ret = -EFAULT;
> + goto err;
> + }
> + ret = client->len;
> +
> +err:
> + /* order is important! */
> + clear_bit(CDC_ENC_CLIENT_RX, &client->flags);
> + clear_bit(CDC_ENC_CLIENT_BUSY, &client->flags);
If order is important here, you need write barriers.
And read barriers at another place.
> + return ret;
> +}
> +
> +static ssize_t cdc_enc_fops_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
> +{
> + struct cdc_enc_client *client = file->private_data;
> + int ret = -EFAULT;
> +
> + if (!client || !client->cdc_enc)
> + return -ENODEV;
> +
> + /* shutdown requested? */
> + if (test_bit(CDC_ENC_CLIENT_SHUTDOWN, &client->flags))
> + return -ENXIO;
> +
> + /* no meaning in attempting to send an incomplete packet */
> + if (size > sizeof(client->tx_buf))
> + return -EFAULT;
> +
> + /* are someone else using our buffer? */
> + if (test_and_set_bit(CDC_ENC_CLIENT_TX, &client->flags))
> + return -ERESTARTSYS;
> +
> + if (size > sizeof(client->tx_buf) || copy_from_user(client->tx_buf, buf, size))
> + goto err;
> +
> + /* send to the device */
> + ret = cdc_enc_send_sync(client, client->tx_buf, size);
> + if (ret < 0)
> + return -EFAULT;
Error handling is shot. The device is now eternally busy.
> +static int cdc_enc_fops_open(struct inode *inode, struct file *file)
> +{
> + struct cdc_enc_state *cdc_enc;
> + struct cdc_enc_client *client;
> +
> + /* associate the file with our backing CDC_ENC device */
> + cdc_enc = container_of(inode->i_cdev, struct cdc_enc_state, cdev);
> + if (!cdc_enc)
> + return -ENODEV;
> +
> + /* don't allow interface to sleep while we are using it */
> + usb_autopm_get_interface(cdc_enc->intf);
This can fail.
> + /* enable status URB even if networking is down */
> + cdc_enc_status_urb(cdc_enc, GFP_KERNEL);
> +
> + /* set up a ring buffer to receive our readable data? */
> + client = cdc_enc_add_client(cdc_enc, cdc_enc_newdata_rcvd);
> +
> + if (!client)
> + return -ENOMEM;
Error handling. The device now can never again autosuspend.
> +
> + file->private_data = client;
> + return 0;
> +}
[..]
> +/* disable and free a CDC_ENC state device */
> +int cdc_enc_free_one(struct cdc_enc_state *cdc_enc)
> +{
> + struct cdc_enc_client *tmp;
> +
> + /* kill any pending recv urb */
> + cdc_enc_kill_readurb(cdc_enc);
> +
> + /* wait for all clients to exit first ... */
> + list_for_each_entry(tmp, &cdc_enc->clients, list) {
> + dev_dbg(&cdc_enc->intf->dev, "waiting for client %p to die\n", tmp);
> + set_bit(CDC_ENC_CLIENT_SHUTDOWN, &tmp->flags);
> + complete(&tmp->ready);
> + }
> + wait_event_interruptible(cdc_enc->waitq, list_empty(&cdc_enc->clients));
If you wait interruptably, you need to handle being interrupted.
> +
> +/* per CDC_ENC interface state */
> +struct cdc_enc_state {
> + struct usb_interface *intf;
> + struct urb *urb; /* receive urb */
> + struct urb *interrupt; /* interrupt urb */
> + unsigned char rcvbuf[CDC_ENC_BUFLEN]; /* receive buffer */
This is a violation of the DMA requirements. You must use kmalloc.
I had a not very detailed look at this driver, but I would really prefer
if you could have a look at making unified code for this, cdc-wdm and cdc-acm.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists