| 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
| ||
|
Message-ID: <CABXOdTe02ALsv6sghnhWMkn7-7kidXhjvWzpDn7dGh4zKEkO8g@mail.gmail.com>
Date: Wed, 10 Apr 2024 12:48:46 -0700
From: Guenter Roeck <groeck@...gle.com>
To: Noah Loomans <noah@...hloomans.com>
Cc: Bhanu Prakash Maiya <bhanumaiya@...omium.org>, Benson Leung <bleung@...omium.org>,
Tzung-Bi Shih <tzungbi@...nel.org>, Guenter Roeck <groeck@...omium.org>,
Robert Zieba <robertzieba@...gle.com>, chrome-platform@...ts.linux.dev,
linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] platform/chrome: cros_ec_uart: properly fix race condition
On Wed, Apr 10, 2024 at 11:29 AM Noah Loomans <noah@...hloomans.com> wrote:
>
> The cros_ec_uart_probe() function calls devm_serdev_device_open() before
> it calls serdev_device_set_client_ops(). This can trigger a NULL pointer
> dereference:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> ...
> CPU: 5 PID: 103 Comm: kworker/u16:3 Not tainted 6.8.4-zen1-1-zen #1 4a88f2661038c2a3bb69aa70fb41a5735338823c
> Hardware name: Google Morphius/Morphius, BIOS MrChromebox-4.22.2-1-g2a93624aebf 01/22/2024
> Workqueue: events_unbound flush_to_ldisc
> RIP: 0010:ttyport_receive_buf+0x3f/0xf0
> ...
> Call Trace:
> <TASK>
> ? __die+0x10f/0x120
> ? page_fault_oops+0x171/0x4e0
> ? srso_return_thunk+0x5/0x5f
> ? exc_page_fault+0x7f/0x180
> ? asm_exc_page_fault+0x26/0x30
> ? ttyport_receive_buf+0x3f/0xf0
> flush_to_ldisc+0x9b/0x1c0
> process_one_work+0x17b/0x340
> worker_thread+0x301/0x490
> ? __pfx_worker_thread+0x10/0x10
> kthread+0xe8/0x120
> ? __pfx_kthread+0x10/0x10
> ret_from_fork+0x34/0x50
> ? __pfx_kthread+0x10/0x10
> ret_from_fork_asm+0x1b/0x30
> </TASK>
>
> A simplified version of crashing code is as follows:
>
> static inline size_t serdev_controller_receive_buf(struct serdev_controller *ctrl,
> const u8 *data,
> size_t count)
> {
> struct serdev_device *serdev = ctrl->serdev;
>
> if (!serdev || !serdev->ops->receive_buf) // CRASH!
> return 0;
>
> return serdev->ops->receive_buf(serdev, data, count);
> }
>
> static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> const u8 *fp, size_t count)
> {
> struct serdev_controller *ctrl = port->client_data;
> [...]
>
> if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> return 0;
>
> ret = serdev_controller_receive_buf(ctrl, cp, count);
>
> [...]
> return ret;
> }
>
> It assumes that if SERPORT_ACTIVE is set and serdev exists, serdev->ops
> will also exist. This conflicts with the existing cros_ec_uart_probe()
> logic, as it first calls devm_serdev_device_open() (which sets
> SERPORT_ACTIVE), and only later sets serdev->ops via
> serdev_device_set_client_ops().
>
> Commit 01f95d42b8f4 ("platform/chrome: cros_ec_uart: fix race
> condition") attempted to fix a similar race condition, but while doing
> so, made the window of error for this race condition to happen much
> wider.
>
> Attempt to fix the race condition again, making sure we fully setup
> before calling devm_serdev_device_open().
>
> Fixes: 01f95d42b8f4 ("platform/chrome: cros_ec_uart: fix race condition")
> Cc: stable@...r.kernel.org
> Signed-off-by: Noah Loomans <noah@...hloomans.com>
> ---
>
> This is my first time contributing to Linux, I hope this is a good
> patch. Feedback on how to improve is welcome!
>
The commit message is a bit long, but the patch itself looks good to me.
Reviewed-by: Guenter Roeck <groeck@...omium.org>
Guenter
> drivers/platform/chrome/cros_ec_uart.c | 28 +++++++++++++-------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
> index 8ea867c2a01a..62bc24f6dcc7 100644
> --- a/drivers/platform/chrome/cros_ec_uart.c
> +++ b/drivers/platform/chrome/cros_ec_uart.c
> @@ -263,12 +263,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
> if (!ec_dev)
> return -ENOMEM;
>
> - ret = devm_serdev_device_open(dev, serdev);
> - if (ret) {
> - dev_err(dev, "Unable to open UART device");
> - return ret;
> - }
> -
> serdev_device_set_drvdata(serdev, ec_dev);
> init_waitqueue_head(&ec_uart->response.wait_queue);
>
> @@ -280,14 +274,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
> return ret;
> }
>
> - ret = serdev_device_set_baudrate(serdev, ec_uart->baudrate);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set up host baud rate (%d)", ret);
> - return ret;
> - }
> -
> - serdev_device_set_flow_control(serdev, ec_uart->flowcontrol);
> -
> /* Initialize ec_dev for cros_ec */
> ec_dev->phys_name = dev_name(dev);
> ec_dev->dev = dev;
> @@ -301,6 +287,20 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
>
> serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
>
> + ret = devm_serdev_device_open(dev, serdev);
> + if (ret) {
> + dev_err(dev, "Unable to open UART device");
> + return ret;
> + }
> +
> + ret = serdev_device_set_baudrate(serdev, ec_uart->baudrate);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set up host baud rate (%d)", ret);
> + return ret;
> + }
> +
> + serdev_device_set_flow_control(serdev, ec_uart->flowcontrol);
> +
> return cros_ec_register(ec_dev);
> }
>
> --
> 2.44.0
>
Powered by blists - more mailing lists