| Message ID | 20221221163249.1058459-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] serdev: ttyport: fix use-after-free on closed TTY | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #96: Unable to handle kernel paging request at virtual address 0072662f67726fd7 total: 0 errors, 1 warnings, 72 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13078922.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 10: B1 Line exceeds max length (99>80): " CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G W 6.1.0-rt5-00325-g8a5f56bcfcca #8" 24: B1 Line exceeds max length (99>80): " CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28" |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
On Wed, Dec 21, 2022 at 05:32:48PM +0100, Krzysztof Kozlowski wrote: > use-after-free is visible in serdev-ttyport, e.g. during system reboot > with Qualcomm Atheros Bluetooth. The TTY is closed, thus "struct > tty_struct" is being released, but the hci_uart_qca driver performs > writes and flushes during system shutdown in qca_serdev_shutdown(). > > Unable to handle kernel paging request at virtual address 0072662f67726fd7 > ... > CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G W 6.1.0-rt5-00325-g8a5f56bcfcca #8 > Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) > Call trace: > tty_driver_flush_buffer+0x4/0x30 > serdev_device_write_flush+0x24/0x34 > qca_serdev_shutdown+0x80/0x130 [hci_uart] > device_shutdown+0x15c/0x260 > kernel_restart+0x48/0xac > > KASAN report: > > BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50 > Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1 > > CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28 > Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) > Call trace: > dump_backtrace.part.0+0xdc/0xf0 > show_stack+0x18/0x30 > dump_stack_lvl+0x68/0x84 > print_report+0x188/0x488 > kasan_report+0xa4/0xf0 > __asan_load8+0x80/0xac > tty_driver_flush_buffer+0x1c/0x50 > ttyport_write_flush+0x34/0x44 > serdev_device_write_flush+0x48/0x60 > qca_serdev_shutdown+0x124/0x274 > device_shutdown+0x1e8/0x350 > kernel_restart+0x48/0xb0 > __do_sys_reboot+0x244/0x2d0 > __arm64_sys_reboot+0x54/0x70 > invoke_syscall+0x60/0x190 > el0_svc_common.constprop.0+0x7c/0x160 > do_el0_svc+0x44/0xf0 > el0_svc+0x2c/0x6c > el0t_64_sync_handler+0xbc/0x140 > el0t_64_sync+0x190/0x194 > > Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver") > Cc: <stable@vger.kernel.org> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > index d367803e2044..3d2bab91a988 100644 > --- a/drivers/tty/serdev/serdev-ttyport.c > +++ b/drivers/tty/serdev/serdev-ttyport.c > @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl) > struct serport *serport = serdev_controller_get_drvdata(ctrl); > struct tty_struct *tty = serport->tty; > > + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) > + return; Shouldn't that be a more useful macro/function instead? serport_is_active(serport) Anyway, what prevents this from changing _right_ after you test it and before you call the next line in this function (same for all invocations here.) thanks, greg k-h
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=706276
---Test result---
Test Summary:
CheckPatch FAIL 1.90 seconds
GitLint FAIL 0.96 seconds
SubjectPrefix FAIL 0.48 seconds
BuildKernel PASS 33.90 seconds
CheckAllWarning PASS 37.11 seconds
CheckSparse PASS 41.60 seconds
BuildKernel32 PASS 32.92 seconds
TestRunnerSetup PASS 468.46 seconds
TestRunner_l2cap-tester PASS 17.01 seconds
TestRunner_iso-tester PASS 17.65 seconds
TestRunner_bnep-tester PASS 5.99 seconds
TestRunner_mgmt-tester PASS 114.31 seconds
TestRunner_rfcomm-tester PASS 9.40 seconds
TestRunner_sco-tester PASS 8.66 seconds
TestRunner_ioctl-tester PASS 10.16 seconds
TestRunner_mesh-tester PASS 7.47 seconds
TestRunner_smp-tester PASS 8.60 seconds
TestRunner_userchan-tester PASS 6.34 seconds
IncrementalBuild PASS 37.02 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[1/2] serdev: ttyport: fix use-after-free on closed TTY
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#96:
Unable to handle kernel paging request at virtual address 0072662f67726fd7
total: 0 errors, 1 warnings, 72 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13078922.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[2/2] Bluetooth: hci_qca: Fix driver shutdown on closed serdev
WARNING: Avoid unnecessary line continuations
#122: FILE: drivers/bluetooth/hci_qca.c:2174:
+ if (test_bit(QCA_BT_OFF, &qca->flags) || \
total: 0 errors, 1 warnings, 17 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13078923.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[1/2] serdev: ttyport: fix use-after-free on closed TTY
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
10: B1 Line exceeds max length (99>80): " CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G W 6.1.0-rt5-00325-g8a5f56bcfcca #8"
24: B1 Line exceeds max length (99>80): " CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28"
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
---
Regards,
Linux Bluetooth
On 21/12/2022 17:39, Greg Kroah-Hartman wrote: > On Wed, Dec 21, 2022 at 05:32:48PM +0100, Krzysztof Kozlowski wrote: >> use-after-free is visible in serdev-ttyport, e.g. during system reboot >> with Qualcomm Atheros Bluetooth. The TTY is closed, thus "struct >> tty_struct" is being released, but the hci_uart_qca driver performs >> writes and flushes during system shutdown in qca_serdev_shutdown(). >> >> Unable to handle kernel paging request at virtual address 0072662f67726fd7 >> ... >> CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G W 6.1.0-rt5-00325-g8a5f56bcfcca #8 >> Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) >> Call trace: >> tty_driver_flush_buffer+0x4/0x30 >> serdev_device_write_flush+0x24/0x34 >> qca_serdev_shutdown+0x80/0x130 [hci_uart] >> device_shutdown+0x15c/0x260 >> kernel_restart+0x48/0xac >> >> KASAN report: >> >> BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50 >> Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1 >> >> CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28 >> Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) >> Call trace: >> dump_backtrace.part.0+0xdc/0xf0 >> show_stack+0x18/0x30 >> dump_stack_lvl+0x68/0x84 >> print_report+0x188/0x488 >> kasan_report+0xa4/0xf0 >> __asan_load8+0x80/0xac >> tty_driver_flush_buffer+0x1c/0x50 >> ttyport_write_flush+0x34/0x44 >> serdev_device_write_flush+0x48/0x60 >> qca_serdev_shutdown+0x124/0x274 >> device_shutdown+0x1e8/0x350 >> kernel_restart+0x48/0xb0 >> __do_sys_reboot+0x244/0x2d0 >> __arm64_sys_reboot+0x54/0x70 >> invoke_syscall+0x60/0x190 >> el0_svc_common.constprop.0+0x7c/0x160 >> do_el0_svc+0x44/0xf0 >> el0_svc+0x2c/0x6c >> el0t_64_sync_handler+0xbc/0x140 >> el0t_64_sync+0x190/0x194 >> >> Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver") >> Cc: <stable@vger.kernel.org> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c >> index d367803e2044..3d2bab91a988 100644 >> --- a/drivers/tty/serdev/serdev-ttyport.c >> +++ b/drivers/tty/serdev/serdev-ttyport.c >> @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl) >> struct serport *serport = serdev_controller_get_drvdata(ctrl); >> struct tty_struct *tty = serport->tty; >> >> + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) >> + return; > > Shouldn't that be a more useful macro/function instead? > serport_is_active(serport) Sure, makes sense. > > Anyway, what prevents this from changing _right_ after you test it and > before you call the next line in this function (same for all invocations > here.) Eh, you're right. I got suggested by such solution in ttyport_write_buf() assuming it was correct in the first place. Is holding tty_lock for entire function here reasonable? Anyway the issue also is in the caller, which should not talk over closed TTY, which should be fixed in patch 2. Best regards, Krzysztof
On Wed, Dec 21, 2022 at 06:37:59PM +0100, Krzysztof Kozlowski wrote: > On 21/12/2022 17:39, Greg Kroah-Hartman wrote: > > On Wed, Dec 21, 2022 at 05:32:48PM +0100, Krzysztof Kozlowski wrote: > >> use-after-free is visible in serdev-ttyport, e.g. during system reboot > >> with Qualcomm Atheros Bluetooth. The TTY is closed, thus "struct > >> tty_struct" is being released, but the hci_uart_qca driver performs > >> writes and flushes during system shutdown in qca_serdev_shutdown(). > >> > >> Unable to handle kernel paging request at virtual address 0072662f67726fd7 > >> ... > >> CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G W 6.1.0-rt5-00325-g8a5f56bcfcca #8 > >> Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) > >> Call trace: > >> tty_driver_flush_buffer+0x4/0x30 > >> serdev_device_write_flush+0x24/0x34 > >> qca_serdev_shutdown+0x80/0x130 [hci_uart] > >> device_shutdown+0x15c/0x260 > >> kernel_restart+0x48/0xac > >> > >> KASAN report: > >> > >> BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50 > >> Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1 > >> > >> CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28 > >> Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) > >> Call trace: > >> dump_backtrace.part.0+0xdc/0xf0 > >> show_stack+0x18/0x30 > >> dump_stack_lvl+0x68/0x84 > >> print_report+0x188/0x488 > >> kasan_report+0xa4/0xf0 > >> __asan_load8+0x80/0xac > >> tty_driver_flush_buffer+0x1c/0x50 > >> ttyport_write_flush+0x34/0x44 > >> serdev_device_write_flush+0x48/0x60 > >> qca_serdev_shutdown+0x124/0x274 > >> device_shutdown+0x1e8/0x350 > >> kernel_restart+0x48/0xb0 > >> __do_sys_reboot+0x244/0x2d0 > >> __arm64_sys_reboot+0x54/0x70 > >> invoke_syscall+0x60/0x190 > >> el0_svc_common.constprop.0+0x7c/0x160 > >> do_el0_svc+0x44/0xf0 > >> el0_svc+0x2c/0x6c > >> el0t_64_sync_handler+0xbc/0x140 > >> el0t_64_sync+0x190/0x194 > >> > >> Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver") > >> Cc: <stable@vger.kernel.org> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> --- > >> drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > >> index d367803e2044..3d2bab91a988 100644 > >> --- a/drivers/tty/serdev/serdev-ttyport.c > >> +++ b/drivers/tty/serdev/serdev-ttyport.c > >> @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl) > >> struct serport *serport = serdev_controller_get_drvdata(ctrl); > >> struct tty_struct *tty = serport->tty; > >> > >> + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) > >> + return; > > > > Shouldn't that be a more useful macro/function instead? > > serport_is_active(serport) > > Sure, makes sense. > > > > > Anyway, what prevents this from changing _right_ after you test it and > > before you call the next line in this function (same for all invocations > > here.) > > Eh, you're right. I got suggested by such solution in > ttyport_write_buf() assuming it was correct in the first place. Is > holding tty_lock for entire function here reasonable? For every function you added this check to? I don't know, would need to audit them all before being able to answer that :( thanks, greg k-h
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c index d367803e2044..3d2bab91a988 100644 --- a/drivers/tty/serdev/serdev-ttyport.c +++ b/drivers/tty/serdev/serdev-ttyport.c @@ -91,6 +91,9 @@ static void ttyport_write_flush(struct serdev_controller *ctrl) struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return; + tty_driver_flush_buffer(tty); } @@ -99,6 +102,9 @@ static int ttyport_write_room(struct serdev_controller *ctrl) struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return 0; + return tty_write_room(tty); } @@ -172,6 +178,9 @@ static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigne struct tty_struct *tty = serport->tty; struct ktermios ktermios = tty->termios; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return -ENXIO; + ktermios.c_cflag &= ~CBAUD; tty_termios_encode_baud_rate(&ktermios, speed, speed); @@ -186,6 +195,9 @@ static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable struct tty_struct *tty = serport->tty; struct ktermios ktermios = tty->termios; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return; + if (enable) ktermios.c_cflag |= CRTSCTS; else @@ -201,6 +213,9 @@ static int ttyport_set_parity(struct serdev_controller *ctrl, struct tty_struct *tty = serport->tty; struct ktermios ktermios = tty->termios; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return -ENXIO; + ktermios.c_cflag &= ~(PARENB | PARODD | CMSPAR); if (parity != SERDEV_PARITY_NONE) { ktermios.c_cflag |= PARENB; @@ -222,6 +237,9 @@ static void ttyport_wait_until_sent(struct serdev_controller *ctrl, long timeout struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return; + tty_wait_until_sent(tty, timeout); } @@ -230,6 +248,9 @@ static int ttyport_get_tiocm(struct serdev_controller *ctrl) struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return -ENXIO; + if (!tty->ops->tiocmget) return -ENOTSUPP; @@ -241,6 +262,9 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) + return -ENXIO; + if (!tty->ops->tiocmset) return -ENOTSUPP;
use-after-free is visible in serdev-ttyport, e.g. during system reboot with Qualcomm Atheros Bluetooth. The TTY is closed, thus "struct tty_struct" is being released, but the hci_uart_qca driver performs writes and flushes during system shutdown in qca_serdev_shutdown(). Unable to handle kernel paging request at virtual address 0072662f67726fd7 ... CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G W 6.1.0-rt5-00325-g8a5f56bcfcca #8 Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) Call trace: tty_driver_flush_buffer+0x4/0x30 serdev_device_write_flush+0x24/0x34 qca_serdev_shutdown+0x80/0x130 [hci_uart] device_shutdown+0x15c/0x260 kernel_restart+0x48/0xac KASAN report: BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50 Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1 CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28 Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) Call trace: dump_backtrace.part.0+0xdc/0xf0 show_stack+0x18/0x30 dump_stack_lvl+0x68/0x84 print_report+0x188/0x488 kasan_report+0xa4/0xf0 __asan_load8+0x80/0xac tty_driver_flush_buffer+0x1c/0x50 ttyport_write_flush+0x34/0x44 serdev_device_write_flush+0x48/0x60 qca_serdev_shutdown+0x124/0x274 device_shutdown+0x1e8/0x350 kernel_restart+0x48/0xb0 __do_sys_reboot+0x244/0x2d0 __arm64_sys_reboot+0x54/0x70 invoke_syscall+0x60/0x190 el0_svc_common.constprop.0+0x7c/0x160 do_el0_svc+0x44/0xf0 el0_svc+0x2c/0x6c el0t_64_sync_handler+0xbc/0x140 el0t_64_sync+0x190/0x194 Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver") Cc: <stable@vger.kernel.org> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/tty/serdev/serdev-ttyport.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)