Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 21 Nov 2019 08:11:10 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Jiangfeng Xiao <xiaojiangfeng@huawei.com>
Cc:     jslaby@suse.com, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org, leeyou.li@huawei.com,
        nixiaoming@huawei.com, zhangwen8@huawei.com
Subject: Re: [PATCH] serial: serial_core: Perform NULL checks for break_ctl
 ops
Message-ID: <20191121071110.GA345427@kroah.com>
References: <1574263133-28259-1-git-send-email-xiaojiangfeng@huawei.com>
 <20191120154235.GA3004157@kroah.com>
 <35ac1c40-d2f4-f6dc-5e2f-f168a0a17c8b@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <35ac1c40-d2f4-f6dc-5e2f-f168a0a17c8b@huawei.com>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 21, 2019 at 10:14:02AM +0800, Jiangfeng Xiao wrote:
> 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@huawei.com>
> >> ---
> >>  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.

Yeah, that makes sense to me as well, I'll go queue this up.  Thanks for
the detailed information.

greg k-h