| Message ID | 20250313190753.450546-1-chenyuan0y@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | cxl/acpi: Add Null check for adev | expand |
On Thu, Mar 13, 2025 at 02:07:53PM -0500, Chenyuan Yang wrote: > Not all devices have an ACPI companion fwnode, so adev might be NULL. > This is similar to the commit cd2fd6eab480 > ("platform/x86: int3472: Check for adev == NULL"). > > Add a check for adev not being set and return -ENODEV in that case to > avoid a possible NULL pointer deref in cxl_acpi_probe(). > Avoiding the NULL ptr deref seems obvious as ACPI_COMPANION() return is routinely checked throughout the kernel. Why the reference to the other commit? Do these devices have something in common? I'm curious as to when *this* specific adev can be NULL. Looks good to check it like you do here, or if someone chimes in that it can never be NULL, just add a code comment saying so. Perhaps emit a message on NULL too. > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> > --- > drivers/cxl/acpi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index cb14829bb9be..9195001db3c1 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -823,6 +823,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) > struct acpi_device *adev = ACPI_COMPANION(host); > struct cxl_cfmws_context ctx; > > + if (!adev) > + return -ENODEV; > + > device_lock_set_class(&pdev->dev, &cxl_root_key); > rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class, > &pdev->dev); > -- > 2.34.1 >
On 3/13/25 3:04 PM, Alison Schofield wrote: > On Thu, Mar 13, 2025 at 02:07:53PM -0500, Chenyuan Yang wrote: >> Not all devices have an ACPI companion fwnode, so adev might be NULL. >> This is similar to the commit cd2fd6eab480 >> ("platform/x86: int3472: Check for adev == NULL"). >> >> Add a check for adev not being set and return -ENODEV in that case to >> avoid a possible NULL pointer deref in cxl_acpi_probe(). >> > > Avoiding the NULL ptr deref seems obvious as ACPI_COMPANION() return > is routinely checked throughout the kernel. Why the reference to the > other commit? Do these devices have something in common? > > I'm curious as to when *this* specific adev can be NULL. You get a NULL by either the the platform device being probed is not setup correctly and the fwnode end up failing is_acpi_device_node() or CONFIG_ACPI is not set. The second is impossible as cxl_acpi depends on CONFIG_CXL_ACPI which depends on CONFIG_ACPI. The likelihood of the first I don't think is high unless the platform device for ACPI0017 is corrupted. So in this instance in the probe function, the checking while is fine for code correctness, may not be absolutely necessary perhaps. Just my 2 cents. DJ > > Looks good to check it like you do here, or if someone chimes in that > it can never be NULL, just add a code comment saying so. > > Perhaps emit a message on NULL too. > > > >> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> >> --- >> drivers/cxl/acpi.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c >> index cb14829bb9be..9195001db3c1 100644 >> --- a/drivers/cxl/acpi.c >> +++ b/drivers/cxl/acpi.c >> @@ -823,6 +823,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) >> struct acpi_device *adev = ACPI_COMPANION(host); >> struct cxl_cfmws_context ctx; >> >> + if (!adev) >> + return -ENODEV; >> + >> device_lock_set_class(&pdev->dev, &cxl_root_key); >> rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class, >> &pdev->dev); >> -- >> 2.34.1 >>
Chenyuan Yang wrote: > Not all devices have an ACPI companion fwnode, so adev might be NULL. > This is similar to the commit cd2fd6eab480 ("platform/x86: int3472: > Check for adev == NULL"). No, please do not consume review bandwidth with patches like this. Read the full commit you referenced it explicitly states the failing condition: "This can e.g. (theoretically) happen when a user manually binds one of the int3472 drivers to another i2c/platform device through sysfs." Then think through whether that theoretical condition applies to the cxl_acpi driver and the ACPI0017 device.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index cb14829bb9be..9195001db3c1 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -823,6 +823,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) struct acpi_device *adev = ACPI_COMPANION(host); struct cxl_cfmws_context ctx; + if (!adev) + return -ENODEV; + device_lock_set_class(&pdev->dev, &cxl_root_key); rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class, &pdev->dev);
Not all devices have an ACPI companion fwnode, so adev might be NULL. This is similar to the commit cd2fd6eab480 ("platform/x86: int3472: Check for adev == NULL"). Add a check for adev not being set and return -ENODEV in that case to avoid a possible NULL pointer deref in cxl_acpi_probe(). Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> --- drivers/cxl/acpi.c | 3 +++ 1 file changed, 3 insertions(+)