Skip to content

Commit

Permalink
USB: cdc-acm: rework notification_buffer resizing
Browse files Browse the repository at this point in the history
commit f4b9d8a upstream.

Clang static analysis reports this error

cdc-acm.c:409:3: warning: Use of memory after it is freed
        acm_process_notification(acm, (unsigned char *)dr);

There are three problems, the first one is that dr is not reset

The variable dr is set with

if (acm->nb_index)
	dr = (struct usb_cdc_notification *)acm->notification_buffer;

But if the notification_buffer is too small it is resized with

		if (acm->nb_size) {
			kfree(acm->notification_buffer);
			acm->nb_size = 0;
		}
		alloc_size = roundup_pow_of_two(expected_size);
		/*
		 * kmalloc ensures a valid notification_buffer after a
		 * use of kfree in case the previous allocation was too
		 * small. Final freeing is done on disconnect.
		 */
		acm->notification_buffer =
			kmalloc(alloc_size, GFP_ATOMIC);

dr should point to the new acm->notification_buffer.

The second problem is any data in the notification_buffer is lost
when the pointer is freed.  In the normal case, the current data
is accumulated in the notification_buffer here.

	memcpy(&acm->notification_buffer[acm->nb_index],
	       urb->transfer_buffer, copy_size);

When a resize happens, anything before
notification_buffer[acm->nb_index] is garbage.

The third problem is the acm->nb_index is not reset on a
resizing buffer error.

So switch resizing to using krealloc and reassign dr and
reset nb_index.

Fixes: ea25835 ("cdc-acm: reassemble fragmented notifications")
Signed-off-by: Tom Rix <trix@redhat.com>
Cc: stable <stable@vger.kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>
Link: https://lore.kernel.org/r/20200801152154.20683-1-trix@redhat.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
trixirt authored and gregkh committed Sep 3, 2020
1 parent afb4204 commit fad19f6
Showing 1 changed file with 10 additions and 12 deletions.
22 changes: 10 additions & 12 deletions drivers/usb/class/cdc-acm.c
Expand Up @@ -378,21 +378,19 @@ static void acm_ctrl_irq(struct urb *urb)
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;
}
u8 *new_buffer;
alloc_size = roundup_pow_of_two(expected_size);
/*
* kmalloc ensures a valid notification_buffer after a
* use of kfree in case the previous allocation was too
* small. Final freeing is done on disconnect.
*/
acm->notification_buffer =
kmalloc(alloc_size, GFP_ATOMIC);
if (!acm->notification_buffer)
/* Final freeing is done on disconnect. */
new_buffer = krealloc(acm->notification_buffer,
alloc_size, GFP_ATOMIC);
if (!new_buffer) {
acm->nb_index = 0;
goto exit;
}

acm->notification_buffer = new_buffer;
acm->nb_size = alloc_size;
dr = (struct usb_cdc_notification *)acm->notification_buffer;
}

copy_size = min(current_size,
Expand Down

0 comments on commit fad19f6

Please sign in to comment.