From f4b9d8a582f738c24ebeabce5cc15f4b8159d74e Mon Sep 17 00:00:00 2001 From: Tom Rix Date: Sat, 1 Aug 2020 08:21:54 -0700 Subject: USB: cdc-acm: rework notification_buffer resizing 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: ea2583529cd1 ("cdc-acm: reassemble fragmented notifications") Signed-off-by: Tom Rix Cc: stable Acked-by: Oliver Neukum Link: https://lore.kernel.org/r/20200801152154.20683-1-trix@redhat.com Signed-off-by: Greg Kroah-Hartman --- drivers/usb/class/cdc-acm.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) (limited to 'drivers/usb/class/cdc-acm.c') diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 991786876dbb..7f6f3ab5b8a6 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -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, -- cgit v1.2.3-59-g8ed1b