| 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: Sat, 18 Mar 2017 19:52:26 +0100
From: Tobias Herzog <t-herzog@....de>
To: Oliver Neukum <oneukum@...e.com>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/4] cdc-acm: reassemble fragmented notifications
Hi,
thank you for your feedback. I tried to fix all issues with v2. I was
just unsure with one point (see comment).
/tobias
> Am Dienstag, den 14.03.2017, 21:14 +0100 schrieb Tobias Herzog:
> >
> > USB devices may have very limitited endpoint packet sizes, so that
> > notifications can not be transferred within one single usb packet.
> > Reassembling of multiple packages may be necessary.
> Hi,
>
> thank you for the patch. Unfortunately it has some issue.
> Please see the comments inside.
>
> Regards
> Oliver
>
> >
> >
> > Signed-off-by: Tobias Herzog <t-herzog@....de>
> > ---
> > drivers/usb/class/cdc-acm.c | 102 +++++++++++++++++++++++++++++++-
> > ------------
> > drivers/usb/class/cdc-acm.h | 2 +
> > 2 files changed, 75 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-
> > acm.c
> > index e35b150..40714fe 100644
> > --- a/drivers/usb/class/cdc-acm.c
> > +++ b/drivers/usb/class/cdc-acm.c
> > @@ -282,39 +282,13 @@ static DEVICE_ATTR(iCountryCodeRelDate,
> > S_IRUGO, show_country_rel_date, NULL);
> > * Interrupt handlers for various ACM device responses
> > */
> >
> > -/* control interface reports status changes with "interrupt"
> > transfers */
> > -static void acm_ctrl_irq(struct urb *urb)
> > +static void acm_process_notification(struct acm *acm, unsigned
> > char *buf)
> > {
> > - struct acm *acm = urb->context;
> > - struct usb_cdc_notification *dr = urb->transfer_buffer;
> > - unsigned char *data;
> > int newctrl;
> > int difference;
> > - int retval;
> > - int status = urb->status;
> > + struct usb_cdc_notification *dr = (struct
> > usb_cdc_notification *)buf;
> > + unsigned char *data = (unsigned char *)(dr + 1);
> >
> > - switch (status) {
> > - case 0:
> > - /* success */
> > - break;
> > - case -ECONNRESET:
> > - case -ENOENT:
> > - case -ESHUTDOWN:
> > - /* this urb is terminated, clean up */
> > - dev_dbg(&acm->control->dev,
> > - "%s - urb shutting down with status:
> > %d\n",
> > - __func__, status);
> > - return;
> > - default:
> > - dev_dbg(&acm->control->dev,
> > - "%s - nonzero urb status received: %d\n",
> > - __func__, status);
> > - goto exit;
> > - }
> > -
> > - usb_mark_last_busy(acm->dev);
> > -
> > - data = (unsigned char *)(dr + 1);
> > switch (dr->bNotificationType) {
> > case USB_CDC_NOTIFY_NETWORK_CONNECTION:
> > dev_dbg(&acm->control->dev,
> > @@ -363,8 +337,74 @@ static void acm_ctrl_irq(struct urb *urb)
> > __func__,
> > dr->bNotificationType, dr->wIndex,
> > dr->wLength, data[0], data[1]);
> > + }
> > +}
> > +
> > +/* control interface reports status changes with "interrupt"
> > transfers */
> > +static void acm_ctrl_irq(struct urb *urb)
> > +{
> > + struct acm *acm = urb->context;
> > + struct usb_cdc_notification *dr = urb->transfer_buffer;
> > + unsigned int current_size = urb->actual_length;
> > + unsigned int expected_size, copy_size;
> > + int retval;
> > + int status = urb->status;
> > +
> > + switch (status) {
> > + case 0:
> > + /* success */
> > break;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + /* this urb is terminated, clean up */
> > + kfree(acm->notification_buffer);
> > + acm->notification_buffer = NULL;
> Why? Disconnect() will free it anyway. It should be enough
> to discard the content.
>
> >
> > + dev_dbg(&acm->control->dev,
> > + "%s - urb shutting down with status:
> > %d\n",
> > + __func__, status);
> > + return;
> > + default:
> > + dev_dbg(&acm->control->dev,
> > + "%s - nonzero urb status received: %d\n",
> > + __func__, status);
> > + goto exit;
> > }
> > +
> > + usb_mark_last_busy(acm->dev);
> > +
> > + if (acm->notification_buffer)
> > + dr = (struct usb_cdc_notification *)acm-
> > >notification_buffer;
> > +
> > + /* assume the first package contains at least two bytes */
> > + expected_size = dr->wLength + 8;
> You need the explain where you got the 8 from. In fact a define would
> be best.
I feel better to use 'sizeof(struct usb_cdc_notification)' here, than
using an separate, hard-coded define. In fact this is really what we
want here, as the buffer needs to cover the notification header (i.e.
struct usb_cdc_notification) + the additional (optional) data.
>
> >
> > +
> > + if (current_size < expected_size) {
> > + /* notification is transmitted framented,
> > reassemble */
> Please fix the typo.
>
> >
> > + if (!acm->notification_buffer) {
> > + acm->notification_buffer =
> > + kmalloc(expected_size,
> > GFP_ATOMIC);
> This can fail. You _must_ check for that.
>
> >
> > + acm->nb_index = 0;
> > + }
> > +
> > + copy_size = min(current_size,
> > + expected_size - acm->nb_index);
> > +
> > + memcpy(&acm->notification_buffer[acm->nb_index],
> > + urb->transfer_buffer, copy_size);
> > + acm->nb_index += copy_size;
> > + current_size = acm->nb_index;
> > + }
> > +
> > + if (current_size < expected_size)
> > + goto exit;
> This is an unneeded goto.
>
> >
> > + /* notification complete */
> > + acm_process_notification(acm, (unsigned char *)dr);
> > +
> > + kfree(acm->notification_buffer);
> Why? If one message was fragmented, the next one will also likely be
> fragmented. Why release the buffer before you know whether it can be
> reused?
>
> >
> > + acm->notification_buffer = NULL;
> > +
> > exit:
> > retval = usb_submit_urb(urb, GFP_ATOMIC);
> > if (retval && retval != -EPERM)
> > @@ -1488,6 +1528,8 @@ static int acm_probe(struct usb_interface
> > *intf,
> > epctrl->bInterval ? epctrl->bInterval :
> > 16);
> > acm->ctrlurb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > acm->ctrlurb->transfer_dma = acm->ctrl_dma;
> > + acm->notification_buffer = NULL;
> > + acm->nb_index = 0;
> >
> > dev_info(&intf->dev, "ttyACM%d: USB ACM device\n", minor);
> >
> > @@ -1580,6 +1622,8 @@ static void acm_disconnect(struct
> > usb_interface *intf)
> > usb_free_coherent(acm->dev, acm->ctrlsize, acm-
> > >ctrl_buffer, acm->ctrl_dma);
> > acm_read_buffers_free(acm);
> >
> > + kfree(acm->notification_buffer);
> > +
> > if (!acm->combined_interfaces)
> > usb_driver_release_interface(&acm_driver, intf ==
> > acm->control ?
> > acm->data : acm->control);
> > diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-
> > acm.h
> > index c980f11..bc07fb2 100644
> > --- a/drivers/usb/class/cdc-acm.h
> > +++ b/drivers/usb/class/cdc-acm.h
> > @@ -98,6 +98,8 @@ struct acm {
> > struct acm_wb *putbuffer; /* for
> > acm_tty_put_char() */
> > int rx_buflimit;
> > spinlock_t read_lock;
> > + u8 *notification_buffer; /* to
> > reassemble fragmented notifications */
> > + unsigned int nb_index;
> > int write_used; /*
> > number of non-empty write buffers */
> > int transmitting;
> > spinlock_t write_lock;
Powered by blists - more mailing lists