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: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
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;
+
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;
--
2.34.1
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: <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> 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: <[email protected]>
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>> ---
>> 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: <[email protected]>
> >> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> >> ---
> >> 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