linux-kernel - Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Mar 2017 16:02:59 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Tobias Herzog <t-herzog@....de>
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications

Am Samstag, den 18.03.2017, 19:52 +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,

almost perfect.
A few new issue. Comments inline.

> 
> +	struct usb_cdc_notification *dr = (struct usb_cdc_notification *)buf;
> +	unsigned char *data = (unsigned char *)(dr + 1);

This is border line incorrect. It depends on the compiler not
adding padding. Please make it

buf + sizeof(struct usb_cdc_notification)

> -	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,77 @@ 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 */
> +		acm->nb_index = 0;
> +		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->nb_index)
> +		dr = (struct usb_cdc_notification *)acm->notification_buffer;
> +
> +	/* size = notification-header + (optional) data */
> +	expected_size = sizeof(struct usb_cdc_notification) +
> +					le16_to_cpu(dr->wLength);
> +
> +	if (current_size < expected_size) {
> +		/* notification is transmitted fragmented, reassemble */
> +		if (acm->nb_size < expected_size) {
> +			if (acm->nb_size) {
> +				kfree(acm->notification_buffer);
> +				acm->nb_size = 0;
> +			}
> +			acm->notification_buffer =
> +					kmalloc(expected_size, GFP_ATOMIC);

Given how kmalloc works you'd better round this up to a power of two.

> +			if (!acm->notification_buffer)
> +				goto exit;

This is most subtle. Please add a comment that this prevents a double
free if we get a disconnect()

> +			acm->nb_size = expected_size;
> +		}

	Regards
		Oliver