linux-kernel - Re: [PATCH v2] greybus/uart: Fix atomicity violation in get_serial

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c36f9d8-b6ee-476c-9daf-feb2fd7ef15c@ieee.org>
Date: Wed, 6 Nov 2024 07:23:14 -0600
From: Alex Elder <elder@...e.org>
To: Qiu-ji Chen <chenqiuji666@...il.com>, dtwlin@...il.com, johan@...nel.org,
 elder@...nel.org, gregkh@...uxfoundation.org
Cc: greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org, baijiaju1990@...il.com
Subject: Re: [PATCH v2] greybus/uart: Fix atomicity violation in
 get_serial_info()

On 11/6/24 3:58 AM, Qiu-ji Chen wrote:
> Our static checker found a bug where set_serial_info() uses a mutex, but
> get_serial_info() does not. Fortunately, the impact of this is relatively
> minor. It doesn't cause a crash or any other serious issues. However, if a
> race condition occurs between set_serial_info() and get_serial_info(),
> there is a chance that the data returned by get_serial_info() will be
> meaningless.
> 
> Signed-off-by: Qiu-ji Chen <chenqiuji666@...il.com>
> Fixes: 0aad5ad563c8 ("greybus/uart: switch to ->[sg]et_serial()")

Looks good.

Reviewed-by: Alex Elder <elder@...cstar.com>

PS  I was going to suggest computing the close delay and closing wait
     outside the mutex, as get_serial_info() above it does.  But it's
     minor and there's no reason to hold up your patch for that.

> ---
> V2:
> Modified the patch description to make it more concise and easier to understand.
> Changed the fix code to ensure the logic is correct.
> Thanks to Johan Hovold and Dan Carpenter for helpful suggestion.
> ---
>   drivers/staging/greybus/uart.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index cdf4ebb93b10..8eab94cb06fa 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -596,11 +596,13 @@ static int get_serial_info(struct tty_struct *tty,
>   	struct gb_tty *gb_tty = tty->driver_data;
>   
>   	ss->line = gb_tty->minor;
> +	mutex_lock(&gb_tty->port.mutex);
>   	ss->close_delay = jiffies_to_msecs(gb_tty->port.close_delay) / 10;
>   	ss->closing_wait =
>   		gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
>   		ASYNC_CLOSING_WAIT_NONE :
>   		jiffies_to_msecs(gb_tty->port.closing_wait) / 10;
> +	mutex_unlock(&gb_tty->port.mutex);
>   
>   	return 0;
>   }