diff mbox series

[1/2] serdev: ttyport: fix use-after-free on closed TTY

Message ID 20221221163249.1058459-1-krzysztof.kozlowski@linaro.org
State New
Headers show
Series [1/2] serdev: ttyport: fix use-after-free on closed TTY | expand

Commit Message

Krzysztof Kozlowski Dec. 21, 2022, 4:32 p.m. UTC 
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(+)

Comments

bluez.test.bot@gmail.com Dec. 21, 2022, 5:07 p.m. UTC | #1 
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
Greg Kroah-Hartman Dec. 21, 2022, 5:42 p.m. UTC | #2 
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 mbox series

Patch

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;