Linux Kernel Mentees Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
       [not found] <00000000000042d60805933945b5@google.com>
@ 2019-11-05 23:36 ` Phong Tran
  2019-11-06 11:42   ` Oliver Neukum
  2019-11-06 12:10   ` Andrey Konovalov via Linux-kernel-mentees
  0 siblings, 2 replies; 7+ messages in thread
From: Phong Tran @ 2019-11-05 23:36 UTC (permalink / raw)
  To: syzbot+495dab1f175edc9c2f13
  Cc: alex.theissen, andreyknvl, linux-usb, 2pi, linux-kernel,
	syzkaller-bugs, linux-kernel-mentees

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.

Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
Reported-and-tested-by:
syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com

https://groups.google.com/d/msg/syzkaller-bugs/dRmkh2UYusY/l2a6Mg3FAQAJ

Signed-off-by: Phong Tran <tranmanphong@gmail.com>
---
 drivers/usb/misc/appledisplay.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index ac92725458b5..3e3dfa5a3954 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -164,7 +164,8 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
 		0,
 		pdata->msgdata, 2,
 		ACD_USB_TIMEOUT);
-	brightness = pdata->msgdata[1];
+	if (retval >= 0)
+		brightness = pdata->msgdata[1];
 	mutex_unlock(&pdata->sysfslock);
 
 	if (retval < 0)
-- 
2.20.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
  2019-11-05 23:36 ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
@ 2019-11-06 11:42   ` Oliver Neukum
  2019-11-06 12:26     ` [Linux-kernel-mentees] KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
  2019-11-06 14:22     ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
  2019-11-06 12:10   ` Andrey Konovalov via Linux-kernel-mentees
  1 sibling, 2 replies; 7+ messages in thread
From: Oliver Neukum @ 2019-11-06 11:42 UTC (permalink / raw)
  To: Phong Tran, syzbot+495dab1f175edc9c2f13
  Cc: alex.theissen, andreyknvl, linux-usb, 2pi, linux-kernel,
	syzkaller-bugs, linux-kernel-mentees

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.

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];
+	}
 	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);
-- 
2.16.4


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
  2019-11-05 23:36 ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
  2019-11-06 11:42   ` Oliver Neukum
@ 2019-11-06 12:10   ` Andrey Konovalov via Linux-kernel-mentees
  2019-11-06 12:11     ` Andrey Konovalov via Linux-kernel-mentees
  1 sibling, 1 reply; 7+ messages in thread
From: Andrey Konovalov via Linux-kernel-mentees @ 2019-11-06 12:10 UTC (permalink / raw)
  To: Phong Tran
  Cc: alex.theissen, USB list, syzkaller-bugs, LKML,
	syzbot+495dab1f175edc9c2f13, 2pi, linux-kernel-mentees

On Wed, Nov 6, 2019 at 12:37 AM Phong Tran <tranmanphong@gmail.com> wrote:
>
> 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.
>
> Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
> Reported-and-tested-by:
> syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
>
> https://groups.google.com/d/msg/syzkaller-bugs/dRmkh2UYusY/l2a6Mg3FAQAJ

Hi Phong,

FYI, when testing patches with the usb-fuzzer instance, you need to
provide the same kernel commit id as the one where the bug was
triggered. Please see here for details:

>
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
>  drivers/usb/misc/appledisplay.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> index ac92725458b5..3e3dfa5a3954 100644
> --- a/drivers/usb/misc/appledisplay.c
> +++ b/drivers/usb/misc/appledisplay.c
> @@ -164,7 +164,8 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
>                 0,
>                 pdata->msgdata, 2,
>                 ACD_USB_TIMEOUT);
> -       brightness = pdata->msgdata[1];
> +       if (retval >= 0)
> +               brightness = pdata->msgdata[1];
>         mutex_unlock(&pdata->sysfslock);
>
>         if (retval < 0)
> --
> 2.20.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
  2019-11-06 12:10   ` Andrey Konovalov via Linux-kernel-mentees
@ 2019-11-06 12:11     ` Andrey Konovalov via Linux-kernel-mentees
  0 siblings, 0 replies; 7+ messages in thread
From: Andrey Konovalov via Linux-kernel-mentees @ 2019-11-06 12:11 UTC (permalink / raw)
  To: Phong Tran
  Cc: alex.theissen, USB list, syzkaller-bugs, LKML,
	syzbot+495dab1f175edc9c2f13, 2pi, linux-kernel-mentees

On Wed, Nov 6, 2019 at 1:10 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Wed, Nov 6, 2019 at 12:37 AM Phong Tran <tranmanphong@gmail.com> wrote:
> >
> > 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.
> >
> > Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
> > Reported-and-tested-by:
> > syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
> >
> > https://groups.google.com/d/msg/syzkaller-bugs/dRmkh2UYusY/l2a6Mg3FAQAJ
>
> Hi Phong,
>
> FYI, when testing patches with the usb-fuzzer instance, you need to
> provide the same kernel commit id as the one where the bug was
> triggered. Please see here for details:

https://github.com/google/syzkaller/blob/master/docs/syzbot.md#usb-bugs

>
> >
> > Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> > ---
> >  drivers/usb/misc/appledisplay.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> > index ac92725458b5..3e3dfa5a3954 100644
> > --- a/drivers/usb/misc/appledisplay.c
> > +++ b/drivers/usb/misc/appledisplay.c
> > @@ -164,7 +164,8 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
> >                 0,
> >                 pdata->msgdata, 2,
> >                 ACD_USB_TIMEOUT);
> > -       brightness = pdata->msgdata[1];
> > +       if (retval >= 0)
> > +               brightness = pdata->msgdata[1];
> >         mutex_unlock(&pdata->sysfslock);
> >
> >         if (retval < 0)
> > --
> > 2.20.1
> >
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] KASAN: use-after-free Read in appledisplay_bl_get_brightness
  2019-11-06 11:42   ` Oliver Neukum
@ 2019-11-06 12:26     ` syzbot
  2019-11-06 14:22     ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
  1 sibling, 0 replies; 7+ messages in thread
From: syzbot @ 2019-11-06 12:26 UTC (permalink / raw)
  To: 2pi, alex.theissen, andreyknvl, gregkh, linux-kernel-mentees,
	linux-kernel, linux-usb, oneukum, syzkaller-bugs, tranmanphong

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger  
crash:

Reported-and-tested-by:  
syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com

Tested on:

commit:         e0bd8d79 usb-fuzzer: main usb gadget fuzzer driver
git tree:       https://github.com/google/kasan.git
kernel config:  https://syzkaller.appspot.com/x/.config?x=8847e5384a16f66a
dashboard link: https://syzkaller.appspot.com/bug?extid=495dab1f175edc9c2f13
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14d463b2e00000

Note: testing is done by a robot and is best-effort only.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
  2019-11-06 11:42   ` Oliver Neukum
  2019-11-06 12:26     ` [Linux-kernel-mentees] KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
@ 2019-11-06 14:22     ` Phong Tran
  2019-11-07  8:50       ` Oliver Neukum
  1 sibling, 1 reply; 7+ messages in thread
From: Phong Tran @ 2019-11-06 14:22 UTC (permalink / raw)
  To: Oliver Neukum, syzbot+495dab1f175edc9c2f13
  Cc: linux-usb, alex.theissen, andreyknvl, 2pi, syzkaller-bugs,
	linux-kernel, linux-kernel-mentees



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);
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
  2019-11-06 14:22     ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
@ 2019-11-07  8:50       ` Oliver Neukum
  0 siblings, 0 replies; 7+ messages in thread
From: Oliver Neukum @ 2019-11-07  8:50 UTC (permalink / raw)
  To: Phong Tran, syzbot+495dab1f175edc9c2f13
  Cc: alex.theissen, andreyknvl, linux-usb, 2pi, linux-kernel,
	syzkaller-bugs, linux-kernel-mentees

Am Mittwoch, den 06.11.2019, 21:22 +0700 schrieb Phong Tran:

> compare with message size (2) can be considered.
> 
> if (retval == 2) {
> 	brightness = pdata->msgdata[1];
> } else {
> 	retval = -EMSGSIZE;
> }

If you do it that way, the error reporting will be distorted.
Hence I did it a bit more complexly.

	Regards
		Oliver
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <00000000000042d60805933945b5@google.com>
2019-11-05 23:36 ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
2019-11-06 11:42   ` Oliver Neukum
2019-11-06 12:26     ` [Linux-kernel-mentees] KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
2019-11-06 14:22     ` [Linux-kernel-mentees] [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
2019-11-07  8:50       ` Oliver Neukum
2019-11-06 12:10   ` Andrey Konovalov via Linux-kernel-mentees
2019-11-06 12:11     ` Andrey Konovalov via Linux-kernel-mentees

Linux Kernel Mentees Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kernel-mentees/0 linux-kernel-mentees/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-kernel-mentees linux-kernel-mentees/ https://lore.kernel.org/linux-kernel-mentees \
		linux-kernel-mentees@lists.linuxfoundation.org
	public-inbox-index linux-kernel-mentees

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxfoundation.lists.linux-kernel-mentees


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