* [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit
@ 2019-05-16 4:09 ` YueHaibing
0 siblings, 0 replies; 9+ messages in thread
From: YueHaibing @ 2019-05-16 4:09 UTC (permalink / raw)
To: jacmet, gregkh, jslaby, shubhrajyoti.datta
Cc: linux-kernel, linux-serial, YueHaibing
If ulite_probe is not called or failed to registed
uart_register_driver, unload the module will call
uart_unregister_driver, which will tigger NULL
pointer dereference like this:
BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100
Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246
CPU: 0 PID: 4246 Comm: syz-executor.0 Tainted: G C 5.1.0+ #26
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
Call Trace:
dump_stack+0xa9/0x10e
? tty_unregister_driver+0x19/0x100
? tty_unregister_driver+0x19/0x100
__kasan_report+0x171/0x18d
? tty_unregister_driver+0x19/0x100
kasan_report+0xe/0x20
tty_unregister_driver+0x19/0x100
uart_unregister_driver+0x30/0xc0
__x64_sys_delete_module+0x244/0x330
? __ia32_sys_delete_module+0x330/0x330
? __x64_sys_clock_gettime+0xe3/0x160
? trace_hardirqs_on_thunk+0x1a/0x1c
? trace_hardirqs_off_caller+0x3e/0x130
? lockdep_hardirqs_off+0xb5/0x100
? mark_held_locks+0x1a/0x90
? do_syscall_64+0x14/0x2a0
do_syscall_64+0x72/0x2a0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
This patch fix this by moving uart_unregister_driver
to ulite_remove.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/tty/serial/uartlite.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index b8b912b..2e49fb6 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
pm_runtime_dont_use_autosuspend(&pdev->dev);
+ uart_unregister_driver(&ulite_uart_driver);
return rc;
}
@@ -897,7 +898,6 @@ static int __init ulite_init(void)
static void __exit ulite_exit(void)
{
platform_driver_unregister(&ulite_platform_driver);
- uart_unregister_driver(&ulite_uart_driver);
}
module_init(ulite_init);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit @ 2019-05-16 4:09 ` YueHaibing 0 siblings, 0 replies; 9+ messages in thread From: YueHaibing @ 2019-05-16 4:09 UTC (permalink / raw) To: jacmet, gregkh, jslaby, shubhrajyoti.datta Cc: linux-kernel, linux-serial, YueHaibing If ulite_probe is not called or failed to registed uart_register_driver, unload the module will call uart_unregister_driver, which will tigger NULL pointer dereference like this: BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 CPU: 0 PID: 4246 Comm: syz-executor.0 Tainted: G C 5.1.0+ #26 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0xa9/0x10e ? tty_unregister_driver+0x19/0x100 ? tty_unregister_driver+0x19/0x100 __kasan_report+0x171/0x18d ? tty_unregister_driver+0x19/0x100 kasan_report+0xe/0x20 tty_unregister_driver+0x19/0x100 uart_unregister_driver+0x30/0xc0 __x64_sys_delete_module+0x244/0x330 ? __ia32_sys_delete_module+0x330/0x330 ? __x64_sys_clock_gettime+0xe3/0x160 ? trace_hardirqs_on_thunk+0x1a/0x1c ? trace_hardirqs_off_caller+0x3e/0x130 ? lockdep_hardirqs_off+0xb5/0x100 ? mark_held_locks+0x1a/0x90 ? do_syscall_64+0x14/0x2a0 do_syscall_64+0x72/0x2a0 entry_SYSCALL_64_after_hwframe+0x49/0xbe This patch fix this by moving uart_unregister_driver to ulite_remove. Reported-by: Hulk Robot <hulkci@huawei.com> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/tty/serial/uartlite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c index b8b912b..2e49fb6 100644 --- a/drivers/tty/serial/uartlite.c +++ b/drivers/tty/serial/uartlite.c @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); pm_runtime_set_suspended(&pdev->dev); pm_runtime_dont_use_autosuspend(&pdev->dev); + uart_unregister_driver(&ulite_uart_driver); return rc; } @@ -897,7 +898,6 @@ static int __init ulite_init(void) static void __exit ulite_exit(void) { platform_driver_unregister(&ulite_platform_driver); - uart_unregister_driver(&ulite_uart_driver); } module_init(ulite_init); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-16 4:09 ` YueHaibing (?) @ 2019-05-16 14:52 ` Shubhrajyoti Datta -1 siblings, 0 replies; 9+ messages in thread From: Shubhrajyoti Datta @ 2019-05-16 14:52 UTC (permalink / raw) To: YueHaibing Cc: jacmet, Greg Kroah-Hartman, jslaby, Shubhrajyoti Datta, linux-kernel, linux-serial Hi YueHaibing, Thanks for the patch. On Thu, May 16, 2019 at 9:42 AM YueHaibing <yuehaibing@huawei.com> wrote: > > If ulite_probe is not called or failed to registed s/registed/register > uart_register_driver, unload the module will call > uart_unregister_driver, which will tigger NULL s/tigger/trigger > pointer dereference like this: > > BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 > Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 > > CPU: 0 PID: 4246 Comm: syz-executor.0 Tainted: G C 5.1.0+ #26 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014 > Call Trace: > dump_stack+0xa9/0x10e > ? tty_unregister_driver+0x19/0x100 > ? tty_unregister_driver+0x19/0x100 > __kasan_report+0x171/0x18d > ? tty_unregister_driver+0x19/0x100 > kasan_report+0xe/0x20 > tty_unregister_driver+0x19/0x100 > uart_unregister_driver+0x30/0xc0 > __x64_sys_delete_module+0x244/0x330 > ? __ia32_sys_delete_module+0x330/0x330 > ? __x64_sys_clock_gettime+0xe3/0x160 > ? trace_hardirqs_on_thunk+0x1a/0x1c > ? trace_hardirqs_off_caller+0x3e/0x130 > ? lockdep_hardirqs_off+0xb5/0x100 > ? mark_held_locks+0x1a/0x90 > ? do_syscall_64+0x14/0x2a0 > do_syscall_64+0x72/0x2a0 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > This patch fix this by moving uart_unregister_driver > to ulite_remove. > Reviewed-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > Reported-by: Hulk Robot <hulkci@huawei.com> > Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/tty/serial/uartlite.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > index b8b912b..2e49fb6 100644 > --- a/drivers/tty/serial/uartlite.c > +++ b/drivers/tty/serial/uartlite.c > @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) > pm_runtime_disable(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > pm_runtime_dont_use_autosuspend(&pdev->dev); > + uart_unregister_driver(&ulite_uart_driver); > return rc; > } > > @@ -897,7 +898,6 @@ static int __init ulite_init(void) > static void __exit ulite_exit(void) > { > platform_driver_unregister(&ulite_platform_driver); > - uart_unregister_driver(&ulite_uart_driver); > } > > module_init(ulite_init); > -- > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-16 4:09 ` YueHaibing (?) (?) @ 2019-05-17 7:55 ` Johan Hovold 2019-05-21 10:10 ` Greg KH -1 siblings, 1 reply; 9+ messages in thread From: Johan Hovold @ 2019-05-17 7:55 UTC (permalink / raw) To: YueHaibing Cc: jacmet, gregkh, jslaby, shubhrajyoti.datta, linux-kernel, linux-serial On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote: > If ulite_probe is not called or failed to registed > uart_register_driver, unload the module will call > uart_unregister_driver, which will tigger NULL > pointer dereference like this: > > BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 > Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 > This patch fix this by moving uart_unregister_driver > to ulite_remove. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/tty/serial/uartlite.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > index b8b912b..2e49fb6 100644 > --- a/drivers/tty/serial/uartlite.c > +++ b/drivers/tty/serial/uartlite.c > @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) > pm_runtime_disable(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > pm_runtime_dont_use_autosuspend(&pdev->dev); > + uart_unregister_driver(&ulite_uart_driver); This broken. Consider what happens if you have tho ports registered and you unbind the first. Someone else sent a fix for this here https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com That fix also has some issues, but is still better given the current state this driver is in. > return rc; > } > > @@ -897,7 +898,6 @@ static int __init ulite_init(void) > static void __exit ulite_exit(void) > { > platform_driver_unregister(&ulite_platform_driver); > - uart_unregister_driver(&ulite_uart_driver); > } > > module_init(ulite_init); Johan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-17 7:55 ` Johan Hovold @ 2019-05-21 10:10 ` Greg KH 2019-05-23 9:18 ` Johan Hovold 0 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2019-05-21 10:10 UTC (permalink / raw) To: Johan Hovold Cc: YueHaibing, jacmet, jslaby, shubhrajyoti.datta, linux-kernel, linux-serial On Fri, May 17, 2019 at 09:55:02AM +0200, Johan Hovold wrote: > On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote: > > If ulite_probe is not called or failed to registed > > uart_register_driver, unload the module will call > > uart_unregister_driver, which will tigger NULL > > pointer dereference like this: > > > > BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 > > Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 > > > This patch fix this by moving uart_unregister_driver > > to ulite_remove. > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > --- > > drivers/tty/serial/uartlite.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > > index b8b912b..2e49fb6 100644 > > --- a/drivers/tty/serial/uartlite.c > > +++ b/drivers/tty/serial/uartlite.c > > @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) > > pm_runtime_disable(&pdev->dev); > > pm_runtime_set_suspended(&pdev->dev); > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > + uart_unregister_driver(&ulite_uart_driver); > > This broken. Consider what happens if you have tho ports registered and > you unbind the first. > > Someone else sent a fix for this here > > https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com > > That fix also has some issues, but is still better given the current > state this driver is in. I'm not taking any of these patches until people agree on what needs to be done here :) Why is this driver so "special" it is having these types of problems? Why can't it do what all other drivers do in this case? thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-21 10:10 ` Greg KH @ 2019-05-23 9:18 ` Johan Hovold 2019-05-23 10:46 ` Michal Simek 0 siblings, 1 reply; 9+ messages in thread From: Johan Hovold @ 2019-05-23 9:18 UTC (permalink / raw) To: Greg KH Cc: Johan Hovold, YueHaibing, jslaby, shubhrajyoti.datta, linux-kernel, linux-serial, Michal Simek On Tue, May 21, 2019 at 12:10:59PM +0200, Greg Kroah-Hartman wrote: > On Fri, May 17, 2019 at 09:55:02AM +0200, Johan Hovold wrote: > > On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote: > > > If ulite_probe is not called or failed to registed > > > uart_register_driver, unload the module will call > > > uart_unregister_driver, which will tigger NULL > > > pointer dereference like this: > > > > > > BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 > > > Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 > > > > > This patch fix this by moving uart_unregister_driver > > > to ulite_remove. > > > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > --- > > > drivers/tty/serial/uartlite.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > > > index b8b912b..2e49fb6 100644 > > > --- a/drivers/tty/serial/uartlite.c > > > +++ b/drivers/tty/serial/uartlite.c > > > @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) > > > pm_runtime_disable(&pdev->dev); > > > pm_runtime_set_suspended(&pdev->dev); > > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > > + uart_unregister_driver(&ulite_uart_driver); > > > > This broken. Consider what happens if you have tho ports registered and > > you unbind the first. > > > > Someone else sent a fix for this here > > > > https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com > > > > That fix also has some issues, but is still better given the current > > state this driver is in. > > I'm not taking any of these patches until people agree on what needs to > be done here :) Good. :) > Why is this driver so "special" it is having these types of problems? > Why can't it do what all other drivers do in this case? Apparently some vendor-tree hacks has made it into mainline. They're trying to register one tty/uart driver per physical port which sounds like a terrible idea. And even the implementation is broken as these bug reports indicate. A series was apparently first posted for xilinks_uartps.c, but that one has not yet been merged AFAICT. A similar series was later posted for uartlite.c, but only the first half or so got in: https://lkml.kernel.org/r/1539685088-13465-1-git-send-email-shubhrajyoti.datta@gmail.com Actually, it looks like the problems started already with: https://lkml.kernel.org/r/1533545534-24458-1-git-send-email-shubhrajyoti.datta@gmail.com So the first broken commit is 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") I think the offending patches should be reverted, but unfortunately they may no longer revert cleanly since there were some new features (e.g. runtime pm) thrown in the mix. Johan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-23 9:18 ` Johan Hovold @ 2019-05-23 10:46 ` Michal Simek 2019-05-23 12:31 ` Johan Hovold 0 siblings, 1 reply; 9+ messages in thread From: Michal Simek @ 2019-05-23 10:46 UTC (permalink / raw) To: Johan Hovold, Greg KH Cc: YueHaibing, jslaby, shubhrajyoti.datta, linux-kernel, linux-serial, Michal Simek Hi Johan, On 23. 05. 19 11:18, Johan Hovold wrote: > On Tue, May 21, 2019 at 12:10:59PM +0200, Greg Kroah-Hartman wrote: >> On Fri, May 17, 2019 at 09:55:02AM +0200, Johan Hovold wrote: >>> On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote: >>>> If ulite_probe is not called or failed to registed >>>> uart_register_driver, unload the module will call >>>> uart_unregister_driver, which will tigger NULL >>>> pointer dereference like this: >>>> >>>> BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 >>>> Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 >>> >>>> This patch fix this by moving uart_unregister_driver >>>> to ulite_remove. >>>> >>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>> --- >>>> drivers/tty/serial/uartlite.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c >>>> index b8b912b..2e49fb6 100644 >>>> --- a/drivers/tty/serial/uartlite.c >>>> +++ b/drivers/tty/serial/uartlite.c >>>> @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) >>>> pm_runtime_disable(&pdev->dev); >>>> pm_runtime_set_suspended(&pdev->dev); >>>> pm_runtime_dont_use_autosuspend(&pdev->dev); >>>> + uart_unregister_driver(&ulite_uart_driver); >>> >>> This broken. Consider what happens if you have tho ports registered and >>> you unbind the first. >>> >>> Someone else sent a fix for this here >>> >>> https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com >>> >>> That fix also has some issues, but is still better given the current >>> state this driver is in. >> >> I'm not taking any of these patches until people agree on what needs to >> be done here :) > > Good. :) > >> Why is this driver so "special" it is having these types of problems? >> Why can't it do what all other drivers do in this case? > > Apparently some vendor-tree hacks has made it into mainline. I have designed this change and it didn't go to vendor tree first. I have been also asking if this is the right way to go. You can find the whole series started with this RFC https://lore.kernel.org/lkml/e2039dc5-92ec-d3a1-bc4f-6565a8c901ac@suse.de/t/ https://lkml.org/lkml/2018/6/6/346 and then https://lkml.org/lkml/2018/9/3/404 And even this step was discussed before in [RFC PATCH 0/4] serial: uartps: Dynamic allocation which was July 2017. And all these patches have been merged by Greg and then I have taken them back to Xilinx Linux tree. And the same concept was also applied to uartlite and intention is also to apply it to pl011 driver to avoid this warning. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/amba-pl011.c?h=v5.2-rc1#n2538 > They're > trying to register one tty/uart driver per physical port which sounds > like a terrible idea. And even the implementation is broken as these bug > reports indicate. I have followed standard process and I have been asking for advices how to do it without hardcoded any number and limiting amount of pre-registered uarts because adding Kconfig options for NR_UARTs was NACK in past too. If you know better way how this can be done, please let us know. And if there is bug we should definitely fix it. > A series was apparently first posted for xilinks_uartps.c, but that one > has not yet been merged AFAICT. Series have been applied by Great 2018-09-18. Feel free to check log here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/tty/serial/xilinx_uartps.c?h=v5.2-rc1 > A similar series was later posted for > uartlite.c, but only the first half or so got in: > > https://lkml.kernel.org/r/1539685088-13465-1-git-send-email-shubhrajyoti.datta@gmail.com > > Actually, it looks like the problems started already with: > > https://lkml.kernel.org/r/1533545534-24458-1-git-send-email-shubhrajyoti.datta@gmail.com > > So the first broken commit is > > 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > > I think the offending patches should be reverted, but unfortunately they > may no longer revert cleanly since there were some new features (e.g. > runtime pm) thrown in the mix. Similar changes have been sent for uartlite but they should use the same concept as I started to use for uartps. But based on what I see in linux-next these patches haven't been merged there. That's why I don't understand the reason for this patch. If you want to see latest state of uartlite you can find it out here. https://github.com/Xilinx/linux-xlnx Thanks, Michal ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-23 10:46 ` Michal Simek @ 2019-05-23 12:31 ` Johan Hovold 2019-05-23 12:47 ` Michal Simek 0 siblings, 1 reply; 9+ messages in thread From: Johan Hovold @ 2019-05-23 12:31 UTC (permalink / raw) To: Michal Simek Cc: Johan Hovold, Greg KH, YueHaibing, jslaby, shubhrajyoti.datta, linux-kernel, linux-serial On Thu, May 23, 2019 at 12:46:38PM +0200, Michal Simek wrote: > Hi Johan, > > On 23. 05. 19 11:18, Johan Hovold wrote: > > On Tue, May 21, 2019 at 12:10:59PM +0200, Greg Kroah-Hartman wrote: > >> On Fri, May 17, 2019 at 09:55:02AM +0200, Johan Hovold wrote: > >>> On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote: > >>>> If ulite_probe is not called or failed to registed > >>>> uart_register_driver, unload the module will call > >>>> uart_unregister_driver, which will tigger NULL > >>>> pointer dereference like this: > >>>> > >>>> BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 > >>>> Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 > >>> > >>>> This patch fix this by moving uart_unregister_driver > >>>> to ulite_remove. > >>>> > >>>> Reported-by: Hulk Robot <hulkci@huawei.com> > >>>> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > >>>> --- > >>>> drivers/tty/serial/uartlite.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > >>>> index b8b912b..2e49fb6 100644 > >>>> --- a/drivers/tty/serial/uartlite.c > >>>> +++ b/drivers/tty/serial/uartlite.c > >>>> @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) > >>>> pm_runtime_disable(&pdev->dev); > >>>> pm_runtime_set_suspended(&pdev->dev); > >>>> pm_runtime_dont_use_autosuspend(&pdev->dev); > >>>> + uart_unregister_driver(&ulite_uart_driver); > >>> > >>> This broken. Consider what happens if you have tho ports registered and > >>> you unbind the first. > >>> > >>> Someone else sent a fix for this here > >>> > >>> https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com > >>> > >>> That fix also has some issues, but is still better given the current > >>> state this driver is in. > >> > >> I'm not taking any of these patches until people agree on what needs to > >> be done here :) > > > > Good. :) > > > >> Why is this driver so "special" it is having these types of problems? > >> Why can't it do what all other drivers do in this case? > > > > Apparently some vendor-tree hacks has made it into mainline. > > I have designed this change and it didn't go to vendor tree first. > I have been also asking if this is the right way to go. > You can find the whole series started with this RFC > https://lore.kernel.org/lkml/e2039dc5-92ec-d3a1-bc4f-6565a8c901ac@suse.de/t/ > > https://lkml.org/lkml/2018/6/6/346 > and then > > https://lkml.org/lkml/2018/9/3/404 Looks like you didn't get any real feedback to your first two RFC series, before you resent as non-RFC and it was merged without any discussion. :/ > And even this step was discussed before in > [RFC PATCH 0/4] serial: uartps: Dynamic allocation > which was July 2017. Yeah, you definitely tried to get feedback on this. > And all these patches have been merged by Greg and then I have taken > them back to Xilinx Linux tree. Right, I had missed some of the back story. > And the same concept was also applied to uartlite and intention is also > to apply it to pl011 driver to avoid this warning. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/amba-pl011.c?h=v5.2-rc1#n2538 Perhaps you should hold off on spreading this further until it can be reviewed. Looks like it's mostly contained to xilinx_uartps right now. > > They're trying to register one tty/uart driver per physical port > > which sounds like a terrible idea. And even the implementation is > > broken as these bug reports indicate. > > I have followed standard process and I have been asking for advices how > to do it without hardcoded any number and limiting amount of > pre-registered uarts because adding Kconfig options for NR_UARTs was > NACK in past too. Yep, you clearly tried to get feedback, but our process fails sometimes. > If you know better way how this can be done, please let us know. Having separate tty drivers and allocating a new major number for every serial port clearly isn't the right way at least (and especially not for fpga uarts of which there could be plenty). If you can't implement what you're after with the current serial-core and tty infrastructure, those subsystems may need to be updated first. > And if there is bug we should definitely fix it. Yes, but not by papering over the problem. > > A series was apparently first posted for xilinks_uartps.c, but that one > > has not yet been merged AFAICT. > > Series have been applied by Great 2018-09-18. Feel free to check log here: "Greg the Great" does have a nice ring to it. ;) > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/tty/serial/xilinx_uartps.c?h=v5.2-rc1 Yeah, sorry, I was obviously looking at the wrong tree this morning. > > A similar series was later posted for > > uartlite.c, but only the first half or so got in: > > > > https://lkml.kernel.org/r/1539685088-13465-1-git-send-email-shubhrajyoti.datta@gmail.com > > > > Actually, it looks like the problems started already with: > > > > https://lkml.kernel.org/r/1533545534-24458-1-git-send-email-shubhrajyoti.datta@gmail.com > > > > So the first broken commit is > > > > 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") > > > > I think the offending patches should be reverted, but unfortunately they > > may no longer revert cleanly since there were some new features (e.g. > > runtime pm) thrown in the mix. > > Similar changes have been sent for uartlite but they should use the same > concept as I started to use for uartps. > But based on what I see in linux-next these patches haven't been merged > there. > That's why I don't understand the reason for this patch. As I mentioned above, only half of the uartlite series was merged, but that was enough to break module unload. > If you want to see latest state of uartlite you can find it out here. > https://github.com/Xilinx/linux-xlnx Thanks for the pointer. Johan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit 2019-05-23 12:31 ` Johan Hovold @ 2019-05-23 12:47 ` Michal Simek 0 siblings, 0 replies; 9+ messages in thread From: Michal Simek @ 2019-05-23 12:47 UTC (permalink / raw) To: Johan Hovold, Michal Simek Cc: Greg KH, YueHaibing, jslaby, shubhrajyoti.datta, linux-kernel, linux-serial On 23. 05. 19 14:31, Johan Hovold wrote: > On Thu, May 23, 2019 at 12:46:38PM +0200, Michal Simek wrote: >> Hi Johan, >> >> On 23. 05. 19 11:18, Johan Hovold wrote: >>> On Tue, May 21, 2019 at 12:10:59PM +0200, Greg Kroah-Hartman wrote: >>>> On Fri, May 17, 2019 at 09:55:02AM +0200, Johan Hovold wrote: >>>>> On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote: >>>>>> If ulite_probe is not called or failed to registed >>>>>> uart_register_driver, unload the module will call >>>>>> uart_unregister_driver, which will tigger NULL >>>>>> pointer dereference like this: >>>>>> >>>>>> BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100 >>>>>> Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246 >>>>> >>>>>> This patch fix this by moving uart_unregister_driver >>>>>> to ulite_remove. >>>>>> >>>>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>>>> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") >>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>>>> --- >>>>>> drivers/tty/serial/uartlite.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c >>>>>> index b8b912b..2e49fb6 100644 >>>>>> --- a/drivers/tty/serial/uartlite.c >>>>>> +++ b/drivers/tty/serial/uartlite.c >>>>>> @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev) >>>>>> pm_runtime_disable(&pdev->dev); >>>>>> pm_runtime_set_suspended(&pdev->dev); >>>>>> pm_runtime_dont_use_autosuspend(&pdev->dev); >>>>>> + uart_unregister_driver(&ulite_uart_driver); >>>>> >>>>> This broken. Consider what happens if you have tho ports registered and >>>>> you unbind the first. >>>>> >>>>> Someone else sent a fix for this here >>>>> >>>>> https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com >>>>> >>>>> That fix also has some issues, but is still better given the current >>>>> state this driver is in. >>>> >>>> I'm not taking any of these patches until people agree on what needs to >>>> be done here :) >>> >>> Good. :) >>> >>>> Why is this driver so "special" it is having these types of problems? >>>> Why can't it do what all other drivers do in this case? >>> >>> Apparently some vendor-tree hacks has made it into mainline. >> >> I have designed this change and it didn't go to vendor tree first. >> I have been also asking if this is the right way to go. >> You can find the whole series started with this RFC >> https://lore.kernel.org/lkml/e2039dc5-92ec-d3a1-bc4f-6565a8c901ac@suse.de/t/ >> >> https://lkml.org/lkml/2018/6/6/346 >> and then >> >> https://lkml.org/lkml/2018/9/3/404 > > Looks like you didn't get any real feedback to your first two RFC > series, before you resent as non-RFC and it was merged without any > discussion. :/ > >> And even this step was discussed before in >> [RFC PATCH 0/4] serial: uartps: Dynamic allocation >> which was July 2017. > > Yeah, you definitely tried to get feedback on this. > >> And all these patches have been merged by Greg and then I have taken >> them back to Xilinx Linux tree. > > Right, I had missed some of the back story. > >> And the same concept was also applied to uartlite and intention is also >> to apply it to pl011 driver to avoid this warning. >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/amba-pl011.c?h=v5.2-rc1#n2538 > > Perhaps you should hold off on spreading this further until it can be > reviewed. Looks like it's mostly contained to xilinx_uartps right now. > >>> They're trying to register one tty/uart driver per physical port >>> which sounds like a terrible idea. And even the implementation is >>> broken as these bug reports indicate. >> >> I have followed standard process and I have been asking for advices how >> to do it without hardcoded any number and limiting amount of >> pre-registered uarts because adding Kconfig options for NR_UARTs was >> NACK in past too. > > Yep, you clearly tried to get feedback, but our process fails sometimes. > >> If you know better way how this can be done, please let us know. > > Having separate tty drivers and allocating a new major number for every > serial port clearly isn't the right way at least (and especially not for > fpga uarts of which there could be plenty). There is no fixed major number for uartps which is not the case for uartlite. It means from description you can see that every uartps instance is getting different major number and also different minor number. I am aware about this and it was also described. I have one issue regarding this flying around. But thinking to save non 0 major number after first registration and pass it to other instances to avoid this behavior. In uartlite where dynamic major number allocation is not present you get correct behavior. uartlite0 204/187 uartlite1 204/188 etc. > If you can't implement what you're after with the current serial-core > and tty infrastructure, those subsystems may need to be updated first. I didn't need to change anything in tty core because all of this is possible to do. There was one patch in connection to reading aliases which was properly reviewed and applied. In general DT aliases behavior should be the same across all boards but in reality it is not. >> And if there is bug we should definitely fix it. > > Yes, but not by papering over the problem. sure. This is the first time I hear about it. Based on my understanding and current linux-next this patch is wrong. If there is any other series please keep in me in CC and I will take a look at will test it on HW. >>> A series was apparently first posted for xilinks_uartps.c, but that one >>> has not yet been merged AFAICT. >> >> Series have been applied by Great 2018-09-18. Feel free to check log here: > > "Greg the Great" does have a nice ring to it. ;) :-) nice typo. > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/tty/serial/xilinx_uartps.c?h=v5.2-rc1 > > Yeah, sorry, I was obviously looking at the wrong tree this morning. good. > >>> A similar series was later posted for >>> uartlite.c, but only the first half or so got in: >>> >>> https://lkml.kernel.org/r/1539685088-13465-1-git-send-email-shubhrajyoti.datta@gmail.com >>> >>> Actually, it looks like the problems started already with: >>> >>> https://lkml.kernel.org/r/1533545534-24458-1-git-send-email-shubhrajyoti.datta@gmail.com >>> >>> So the first broken commit is >>> >>> 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe") >>> >>> I think the offending patches should be reverted, but unfortunately they >>> may no longer revert cleanly since there were some new features (e.g. >>> runtime pm) thrown in the mix. >> >> Similar changes have been sent for uartlite but they should use the same >> concept as I started to use for uartps. >> But based on what I see in linux-next these patches haven't been merged >> there. >> That's why I don't understand the reason for this patch. > > As I mentioned above, only half of the uartlite series was merged, but > that was enough to break module unload. I didn't strictly follow this. Internally I have reviewed it but didn't watch what exactly was merged to mainline. Thanks, Michal ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-23 12:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-16 4:09 [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit YueHaibing 2019-05-16 4:09 ` YueHaibing 2019-05-16 14:52 ` Shubhrajyoti Datta 2019-05-17 7:55 ` Johan Hovold 2019-05-21 10:10 ` Greg KH 2019-05-23 9:18 ` Johan Hovold 2019-05-23 10:46 ` Michal Simek 2019-05-23 12:31 ` Johan Hovold 2019-05-23 12:47 ` Michal Simek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.