linux-kernel - Re: [PATCH v3] drivers/tty/serial: check the return value of uart_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 5 Sep 2022 13:16:46 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Li Zhong <floridsleeves@...il.com>
cc:     linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        gregkh@...uxfoundation.org, jirislaby@...nel.org
Subject: Re: [PATCH v3] drivers/tty/serial: check the return value of
 uart_port_check()

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@...il.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);
>  
>  	/*
>