netdev - Re: [PATCH v4 07/10] can: kvaser_pciefd: Add devlink support

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: <6cbe9e11-a9b7-48f2-8b13-068fb9eec290@gmail.com>
Date: Fri, 25 Jul 2025 15:50:41 +0200
From: Jimmy Assarsson <jimmyassarsson@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>, Jimmy Assarsson <extja@...ser.com>
Cc: linux-can@...r.kernel.org, Vincent Mailhol <mailhol.vincent@...adoo.fr>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v4 07/10] can: kvaser_pciefd: Add devlink support

On 7/25/25 2:47 PM, Marc Kleine-Budde wrote:
> On 25.07.2025 14:32:27, Jimmy Assarsson wrote:
>> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
>> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
>> @@ -1751,14 +1751,16 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
>>   			       const struct pci_device_id *id)
>>   {
>>   	int ret;
>> +	struct devlink *devlink;
>>   	struct device *dev = &pdev->dev;
>>   	struct kvaser_pciefd *pcie;
>>   	const struct kvaser_pciefd_irq_mask *irq_mask;
>>   
>> -	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>> -	if (!pcie)
>> +	devlink = devlink_alloc(&kvaser_pciefd_devlink_ops, sizeof(*pcie), dev);
>> +	if (!devlink)
>>   		return -ENOMEM;
>>   
>> +	pcie = devlink_priv(devlink);
>>   	pci_set_drvdata(pdev, pcie);
>>   	pcie->pci = pdev;
>>   	pcie->driver_data = (const struct kvaser_pciefd_driver_data *)id->driver_data;
>> @@ -1766,7 +1768,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
>>   
>>   	ret = pci_enable_device(pdev);
>>   	if (ret)
>> -		return ret;
>> +		goto err_free_devlink;
>>   
>>   	ret = pci_request_regions(pdev, KVASER_PCIEFD_DRV_NAME);
>>   	if (ret)
>> @@ -1830,6 +1832,8 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
>>   	if (ret)
>>   		goto err_free_irq;
>>   
>> +	devlink_register(devlink);
>> +
>>   	return 0;
>>   
>>   err_free_irq:
>> @@ -1853,6 +1857,9 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
>>   err_disable_pci:
>>   	pci_disable_device(pdev);
>>   
>> +err_free_devlink:
>> +	devlink_free(devlink);
>> +
>>   	return ret;
>>   }
>>   
>> @@ -1876,6 +1883,8 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)
>>   	for (i = 0; i < pcie->nr_channels; ++i)
>>   		free_candev(pcie->can[i]->can.dev);
>>   
>> +	devlink_unregister(priv_to_devlink(pcie));
>> +	devlink_free(priv_to_devlink(pcie));
>>   	pci_iounmap(pdev, pcie->reg_base);
>                            ^^^^
> 
> This smells like a use after free. Please call the cleanup function in
> reverse order of allocation functions, i.e. move devlink_free() to the
> end of this function.
> 
>>   	pci_release_regions(pdev);
>>   	pci_disable_device(pdev);
> 
> regards,
> Marc


I agree. Thanks for finding this!
I've tested moving devlink_free() to the end of the function, without any
issues.

If you don't find any other problems, do you mind making this change before sending
the PR? Otherwise I need to wait for the netdev 24h grace period, as Paolo
pointed out [1], before sending v5.

[1] https://lore.kernel.org/linux-can/20250725-furry-precise-jerboa-d9e29d-mkl@pengutronix.de/T/#m174402d37840f225b2799fbc53d7658ccc27be72

Best regards,
jimmy