linux-kernel - Re: [PATCH] cdc-acm: rework notification

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1596532623.19923.5.camel@suse.com>
Date:   Tue, 04 Aug 2020 11:17:03 +0200
From:   Oliver Neukum <oneukum@...e.com>
To:     trix@...hat.com, gregkh@...uxfoundation.org, t-herzog@....de
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cdc-acm: rework notification_buffer resizing

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