Re: [PATCH] serial: serial_core: Perform NULL checks for break_ctl ops

[Jiangfeng Xiao]

On 2019/11/20 23:42, Greg KH wrote:
> On Wed, Nov 20, 2019 at 11:18:53PM +0800, Jiangfeng Xiao wrote:
>> Doing fuzz test on sbsa uart device, causes a kernel crash
>> due to NULL pointer dereference:
>>
>> ------------[ cut here ]------------
>> Unable to handle kernel paging request at virtual address fffffffffffffffc
>> CPU: 2 PID: 2385 Comm: tty_fuzz_test Tainted: G           O    4.4.193 #1
>> task: ffffffe32b23f110 task.stack: ffffffe32bda4000
>> PC is at uart_break_ctl+0x44/0x84
>> LR is at uart_break_ctl+0x34/0x84
>> pc : [<ffffff8393196098>] lr : [<ffffff8393196088>] pstate: 80000005
>> sp : ffffffe32bda7cc0
>> [<ffffff8393196098>] uart_break_ctl+0x44/0x84
>> [<ffffff8393177718>] send_break+0xa0/0x114
>> [<ffffff8393179a1c>] tty_ioctl+0xc50/0xe84
>> [<ffffff8392fa5a40>] do_vfs_ioctl+0xc4/0x6e8
>> [<ffffff8392fa60cc>] SyS_ioctl+0x68/0x9c
>> [<ffffff8392e02e78>] __sys_trace_return+0x0/0x4
>> Code: b9410ea0 34000160 f9408aa0 f9402814 (b85fc280)
>> ---[ end trace 8606094f1960c5e0 ]---
>> Kernel panic - not syncing: Fatal exception
>>
>> Fix this problem by adding NULL checks prior to calling break_ctl ops.
>>
>> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@xxxxxxxxxx>
>> ---
>>  drivers/tty/serial/serial_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index c4a414a..b0a6eb1 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1111,7 +1111,7 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state)
>>  	if (!uport)
>>  		goto out;
>>  
>> -	if (uport->type != PORT_UNKNOWN)
>> +	if (uport->type != PORT_UNKNOWN && uport->ops->break_ctl)
> 
> What serial driver does not define break_ctl?

I'm using arm_sbsa_uart_platform_driver, and sbsa_uart_pops does not define break_ctl.
I did a troubleshooting on the latest mainline 5.4-rc8 kernel.
There are 7 uart_ops without defining break_ctl:
./drivers/tty/serial/men_z135_uart.c:774:static const struct uart_ops men_z135_ops = {
./drivers/tty/serial/amba-pl011.c:2173:static const struct uart_ops sbsa_uart_pops = {
./drivers/tty/serial/owl-uart.c:464:static const struct uart_ops owl_uart_ops = {
./drivers/tty/serial/meson_uart.c:430:static const struct uart_ops meson_uart_ops = {
./drivers/tty/serial/qcom_geni_serial.c:1212:static const struct uart_ops qcom_geni_console_pops = {
./drivers/tty/serial/qcom_geni_serial.c:1232:static const struct uart_ops qcom_geni_uart_pops = {
./drivers/tty/serial/rda-uart.c:559:static const struct uart_ops rda_uart_ops = {

> You are running with a bunch of "out-of-tree" drivers, perhaps one of
> those needs to be fixed here instead?

I carefully confirmed it again, the kernel crash was caused by arm_sbsa_uart_platform_driver which is "in-tree".

There are two ways to fix this problem, one is to add a null pointer check,
and the other is to define break_ctl for each uart_ops.
I am a newbie on serial_core, so I don't know which way is more reasonable.

Based on the principle of minimal change, I chose to add a null pointer check.

Thank you for your review.