Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Sep 2022 13:16:46 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Li Zhong <floridsleeves@gmail.com>
cc:     linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        gregkh@linuxfoundation.org, jirislaby@kernel.org
Subject: Re: [PATCH v3] drivers/tty/serial: check the return value of
 uart_port_check()
In-Reply-To: <20220904004524.2281227-1-floridsleeves@gmail.com>
Message-ID: <acf3b04e-14f7-030-bdf9-3427bbb688@linux.intel.com>
References: <20220904004524.2281227-1-floridsleeves@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sat, 3 Sep 2022, Li Zhong wrote:

> uart_port_check() will return NULL pointer when state->uart_port is
> NULL. Check the return value before dereference it to avoid
> null-pointer-dereference error

> because the locking does not guarantee the return value is not NULL.

Please include also the answer to the "Why it doesn't guarantee?"
question.

In addition, it's expected you'll keep the people who have expressed 
interest in your patch among the receipients for any new version you 
send out. Thank you.

-- 
 i.

> Here we do not need unlock in the error
> handling because the mutex_unlock() is called in callers.
> 
> Signed-off-by: Li Zhong <floridsleeves@gmail.com>
> ---
> 
> v3: Add the reason why we need to check the NULL value in the commit
> message.  The bug is detected by static analysis.
> 
> ---
>  drivers/tty/serial/serial_core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 12c87cd201a7..760e177166cf 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -194,6 +194,9 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>  	unsigned long page;
>  	int retval = 0;
>  
> +	if (!uport)
> +		return -EIO;
> +
>  	if (uport->type == PORT_UNKNOWN)
>  		return 1;
>  
> @@ -498,6 +501,8 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
>  	struct ktermios *termios;
>  	int hw_stopped;
>  
> +	if (!uport)
> +		return;
>  	/*
>  	 * If we have no tty, termios, or the port does not exist,
>  	 * then we can't set the parameters for this port.
> @@ -1045,6 +1050,8 @@ static int uart_get_lsr_info(struct tty_struct *tty,
>  	struct uart_port *uport = uart_port_check(state);
>  	unsigned int result;
>  
> +	if (!uport)
> +		return -EIO;
>  	result = uport->ops->tx_empty(uport);
>  
>  	/*
>