Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] cdc-acm: rework notification_buffer resizing
@ 2020-08-01 15:21 trix
  2020-08-04  9:17 ` Oliver Neukum
  2020-08-04  9:17 ` Oliver Neukum
  0 siblings, 2 replies; 3+ messages in thread
From: trix @ 2020-08-01 15:21 UTC (permalink / raw)
  To: oneukum, gregkh, t-herzog; +Cc: linux-usb, linux-kernel, Tom Rix

From: Tom Rix <trix@redhat.com>

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 <trix@redhat.com>
---
 drivers/usb/class/cdc-acm.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

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,
-- 
2.18.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] cdc-acm: rework notification_buffer resizing
  2020-08-01 15:21 [PATCH] cdc-acm: rework notification_buffer resizing trix
@ 2020-08-04  9:17 ` Oliver Neukum
  2020-08-04  9:17 ` Oliver Neukum
  1 sibling, 0 replies; 3+ messages in thread
From: Oliver Neukum @ 2020-08-04  9:17 UTC (permalink / raw)
  To: trix, gregkh, t-herzog; +Cc: linux-usb, linux-kernel

Am Samstag, den 01.08.2020, 08:21 -0700 schrieb trix@redhat.com:
> From: Tom Rix <trix@redhat.com>
> 
> 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 <trix@redhat.com>
Acked-by: Oliver Neukum <oneukum@suse.com>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] cdc-acm: rework notification_buffer resizing
  2020-08-01 15:21 [PATCH] cdc-acm: rework notification_buffer resizing trix
  2020-08-04  9:17 ` Oliver Neukum
@ 2020-08-04  9:17 ` Oliver Neukum
  1 sibling, 0 replies; 3+ messages in thread
From: Oliver Neukum @ 2020-08-04  9:17 UTC (permalink / raw)
  To: trix, gregkh, t-herzog; +Cc: linux-usb, linux-kernel

Am Samstag, den 01.08.2020, 08:21 -0700 schrieb trix@redhat.com:
> From: Tom Rix <trix@redhat.com>
> 
> 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

Hi,

thank you, good catch.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-01 15:21 [PATCH] cdc-acm: rework notification_buffer resizing trix
2020-08-04  9:17 ` Oliver Neukum
2020-08-04  9:17 ` Oliver Neukum

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git