* [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 related [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, other threads:[~2020-08-04 9:17 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).