linux-kernel - Re: [PATCH] usb: appledisplay: fix use-after-free in bl_get

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <d6c628d0-71ad-7343-d80e-1b0cd0795242@gmail.com>
Date:   Wed, 6 Nov 2019 21:22:34 +0700
From:   Phong Tran <tranmanphong@...il.com>
To:     Oliver Neukum <oneukum@...e.com>,
        syzbot+495dab1f175edc9c2f13@...kaller.appspotmail.com
Cc:     tranmanphong@...il.com, 2pi@....nu, alex.theissen@...com,
        andreyknvl@...gle.com, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        syzkaller-bugs@...glegroups.com,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] usb: appledisplay: fix use-after-free in
 bl_get_brightness



On 11/6/19 6:42 PM, Oliver Neukum wrote:
> Am Mittwoch, den 06.11.2019, 06:36 +0700 schrieb Phong Tran:
>> In context of USB disconnect, the delaywork trigger and calling
>> appledisplay_bl_get_brightness() and the msgdata was freed.
>>
>> add the checking return value of usb_control_msg() and only update the
>> data while the retval is valid.
> 
> Hi,
> 
> I am afraid there are some issues with your patch. First, let me stress
> that you found the right place to fix an issue and you partially fixed
> an issue. But the the fix you applied is incomplete and left another
> issue open.
> 
> Your patch still allows doing IO to a device that may already be bound
> to another driver. That is bad, especially as the buffer is already
> free. Yes, if IO is failing, you have fixed that narrow issue.
> But it need not fail.
> 
> If you look into appledisplay_probe() you will see that it can fail
> because backlight_device_register() fails. The error handling will
> thereupon kill the URB and free memory. But it will not kill an already
> scheduled work. The scheduled work will then call usb_control_msg()
> on pdata->msgdata, which has been freed. If that IO fails, all is well.
> If not, the issue still exists.
> 
Hello Oliver,

argee, there need a cancel workqueue in case error of probe().

> Secondly, your error check is off by 2. You are checking only for
> usb_control_msg() failing. But it can return only one byte instead
> of two. If that happens, the value you return is stale, although
> the buffer is correctly allocated.
> 
> 	Regards
> 		Oliver
> 
> The correct fix for both issues would be:
> 
> #syz test: https://github.com/google/kasan.git e0bd8d79
> 
>  From 2497a62bdbeb9bd94f690c22d96dd1b8cf22861f Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@...e.com>
> Date: Wed, 6 Nov 2019 12:36:28 +0100
> Subject: [PATCH] appledisplay: fix error handling in the scheduled work
> 
> The work item can operate on
> 
> 1. stale memory left over from the last transfer
> the actual length of the data transfered needs to be checked
> 2. memory already freed
> the error handling in appledisplay_probe() needs
> to cancel the work in that case
> 
> Signed-off-by: Oliver Neukum <oneukum@...e.com>
> ---
>   drivers/usb/misc/appledisplay.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> index ac92725458b5..ba1eaabc7796 100644
> --- a/drivers/usb/misc/appledisplay.c
> +++ b/drivers/usb/misc/appledisplay.c
> @@ -164,7 +164,12 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
>   		0,
>   		pdata->msgdata, 2,
>   		ACD_USB_TIMEOUT);
> -	brightness = pdata->msgdata[1];
> +	if (retval < 2) {
> +		if (retval >= 0)
> +			retval = -EMSGSIZE;
> +	} else {
> +		brightness = pdata->msgdata[1];
> +	}

compare with message size (2) can be considered.

if (retval == 2) {
	brightness = pdata->msgdata[1];
} else {
	retval = -EMSGSIZE;
}

Regards,
Phong.

>   	mutex_unlock(&pdata->sysfslock);
>   
>   	if (retval < 0)
> @@ -299,6 +304,7 @@ static int appledisplay_probe(struct usb_interface *iface,
>   	if (pdata) {
>   		if (pdata->urb) {
>   			usb_kill_urb(pdata->urb);
> +			cancel_delayed_work_sync(&pdata->work);
>   			if (pdata->urbdata)
>   				usb_free_coherent(pdata->udev, ACD_URB_BUFFER_LEN,
>   					pdata->urbdata, pdata->urb->transfer_dma);
>