| Message ID | 20250111054717.1830450-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: dell-uart-backlight: fix NULL pointer dereference in probe | expand |
Hi, On 11-Jan-25 6:47 AM, Chenyuan Yang wrote: > The dell_uart_bl_serdev_probe() function calls devm_serdev_device_open() > before setting the client ops via serdev_device_set_client_ops(). This > ordering can trigger a NULL pointer dereference in the serdev controller's > receive_buf handler, as it assumes serdev->ops is valid when > SERPORT_ACTIVE is set. > > This is similar to the issue fixed in commit 5e700b384ec1 > ("platform/chrome: cros_ec_uart: properly fix race condition") where > devm_serdev_device_open() was called before fully initializing the > device. > > Fix the race by ensuring client ops are set before enabling the port via > devm_serdev_device_open(). > > Fixes: 484bae9e4d6a ("platform/x86: Add new Dell UART backlight driver") > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> Thank you for your patch. > --- > drivers/platform/x86/dell/dell-uart-backlight.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > index 6e5dc7e3674f..41162ff1686b 100644 > --- a/drivers/platform/x86/dell/dell-uart-backlight.c > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > @@ -283,16 +283,16 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) > init_waitqueue_head(&dell_bl->wait_queue); > dell_bl->dev = dev; > > - ret = devm_serdev_device_open(dev, serdev); > - if (ret) > - return dev_err_probe(dev, ret, "opening UART device\n"); > - > /* 9600 bps, no flow control, these are the default but set them to be sure */ > serdev_device_set_baudrate(serdev, 9600); > serdev_device_set_flow_control(serdev, false); > serdev_device_set_drvdata(serdev, dell_bl); > serdev_device_set_client_ops(serdev, &dell_uart_bl_serdev_ops); > > + ret = devm_serdev_device_open(dev, serdev); > + if (ret) > + return dev_err_probe(dev, ret, "opening UART device\n"); > + serdev_device_open() resets the termios settings such as flow-control, so you need to move the serdev_device_set_baudrate() and serdev_device_set_flow_control() calls to after the new location of the devm_serdev_device_open() call, just like commit 5e700b384ec1 ("platform/chrome: cros_ec_uart: properly fix race condition") does. Regards, Hans > get_version[0] = DELL_SOF(GET_CMD_LEN); > get_version[1] = CMD_GET_VERSION; > get_version[2] = dell_uart_checksum(get_version, 2);
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c index 6e5dc7e3674f..41162ff1686b 100644 --- a/drivers/platform/x86/dell/dell-uart-backlight.c +++ b/drivers/platform/x86/dell/dell-uart-backlight.c @@ -283,16 +283,16 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) init_waitqueue_head(&dell_bl->wait_queue); dell_bl->dev = dev; - ret = devm_serdev_device_open(dev, serdev); - if (ret) - return dev_err_probe(dev, ret, "opening UART device\n"); - /* 9600 bps, no flow control, these are the default but set them to be sure */ serdev_device_set_baudrate(serdev, 9600); serdev_device_set_flow_control(serdev, false); serdev_device_set_drvdata(serdev, dell_bl); serdev_device_set_client_ops(serdev, &dell_uart_bl_serdev_ops); + ret = devm_serdev_device_open(dev, serdev); + if (ret) + return dev_err_probe(dev, ret, "opening UART device\n"); + get_version[0] = DELL_SOF(GET_CMD_LEN); get_version[1] = CMD_GET_VERSION; get_version[2] = dell_uart_checksum(get_version, 2);
The dell_uart_bl_serdev_probe() function calls devm_serdev_device_open() before setting the client ops via serdev_device_set_client_ops(). This ordering can trigger a NULL pointer dereference in the serdev controller's receive_buf handler, as it assumes serdev->ops is valid when SERPORT_ACTIVE is set. This is similar to the issue fixed in commit 5e700b384ec1 ("platform/chrome: cros_ec_uart: properly fix race condition") where devm_serdev_device_open() was called before fully initializing the device. Fix the race by ensuring client ops are set before enabling the port via devm_serdev_device_open(). Fixes: 484bae9e4d6a ("platform/x86: Add new Dell UART backlight driver") Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> --- drivers/platform/x86/dell/dell-uart-backlight.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)