| Message ID | 20250111180118.2274516-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] platform/x86: dell-uart-backlight: fix NULL pointer dereference in probe | expand |
Hi, On 11-Jan-25 7:01 PM, 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(). > Note, serdev_device_set_baudrate() and > serdev_device_set_flow_control() calls should be > after the devm_serdev_device_open() call. > > Fixes: 484bae9e4d6a ("platform/x86: Add new Dell UART backlight driver") > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> > CC: Hans de Goede <hdegoede@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/platform/x86/dell/dell-uart-backlight.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > index 6e5dc7e3674f..bcc5c0f3bb4d 100644 > --- a/drivers/platform/x86/dell/dell-uart-backlight.c > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > @@ -283,6 +283,9 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) > init_waitqueue_head(&dell_bl->wait_queue); > dell_bl->dev = dev; > > + 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"); > @@ -290,8 +293,6 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) > /* 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); > > get_version[0] = DELL_SOF(GET_CMD_LEN); > get_version[1] = CMD_GET_VERSION;
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c index 6e5dc7e3674f..bcc5c0f3bb4d 100644 --- a/drivers/platform/x86/dell/dell-uart-backlight.c +++ b/drivers/platform/x86/dell/dell-uart-backlight.c @@ -283,6 +283,9 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) init_waitqueue_head(&dell_bl->wait_queue); dell_bl->dev = dev; + 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"); @@ -290,8 +293,6 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) /* 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); get_version[0] = DELL_SOF(GET_CMD_LEN); get_version[1] = CMD_GET_VERSION;
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(). Note, serdev_device_set_baudrate() and serdev_device_set_flow_control() calls should be after the devm_serdev_device_open() call. Fixes: 484bae9e4d6a ("platform/x86: Add new Dell UART backlight driver") Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> CC: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell/dell-uart-backlight.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)