linux-kernel - Re: [PATCH] cxl/acpi: Add Null check for adev

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: <b2bb61f0-6250-4b90-a765-c2873eccb07d@intel.com>
Date: Thu, 13 Mar 2025 15:27:18 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Alison Schofield <alison.schofield@...el.com>,
 Chenyuan Yang <chenyuan0y@...il.com>
Cc: dave@...olabs.net, jonathan.cameron@...wei.com, vishal.l.verma@...el.com,
 ira.weiny@...el.com, dan.j.williams@...el.com, linux-cxl@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cxl/acpi: Add Null check for adev



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@...il.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
>>