Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Cc:     tranmanphong@gmail.com, 2pi@mok.nu, alex.theissen@me.com,
        andreyknvl@google.com, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        syzkaller-bugs@googlegroups.com,
        linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH] usb: appledisplay: fix use-after-free in
 bl_get_brightness
To:     Oliver Neukum <oneukum@suse.com>,
        syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
References: <00000000000042d60805933945b5@google.com>
 <20191105233652.21033-1-tranmanphong@gmail.com>
 <1573040577.3090.22.camel@suse.com>
From:   Phong Tran <tranmanphong@gmail.com>
Message-ID: <d6c628d0-71ad-7343-d80e-1b0cd0795242@gmail.com>
Date:   Wed, 6 Nov 2019 21:22:34 +0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <1573040577.3090.22.camel@suse.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@suse.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@suse.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);
>