netdev - Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d35a962d-dc95-4469-867e-95b704cca474@wanadoo.fr>
Date: Tue, 20 Aug 2024 12:50:17 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Philipp Stanner <pstanner@...hat.com>
Cc: alexandre.torgue@...s.st.com, alvaro.karsz@...id-run.com,
 andy@...nel.org, axboe@...nel.dk, bhelgaas@...gle.com, brgl@...ev.pl,
 broonie@...nel.org, christophe.jaillet@...adoo.fr, corbet@....net,
 davem@...emloft.net, edumazet@...gle.com, eperezma@...hat.com,
 hao.wu@...el.com, jasowang@...hat.com, joabreu@...opsys.com,
 kuba@...nel.org, linus.walleij@...aro.org,
 linux-arm-kernel@...ts.infradead.org, linux-block@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-fpga@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 mcoquelin.stm32@...il.com, mdf@...nel.org, mst@...hat.com,
 netdev@...r.kernel.org, pabeni@...hat.com, richardcochran@...il.com,
 trix@...hat.com, virtualization@...ts.linux.dev, xuanzhuo@...ux.alibaba.com,
 yilun.xu@...el.com
Subject: Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

Le 20/08/2024 à 10:09, Philipp Stanner a écrit :
>>> @@ -556,33 +556,24 @@ static const struct vdpa_config_ops
>>> snet_config_ops = {
>>>    static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
>>> *psnet)
>>>    {
>>>    	char name[50];
>>> -	int ret, i, mask = 0;
>>> +	int i;
>>> +
>>> +	snprintf(name, sizeof(name), "psnet[%s]-bars",
>>> pci_name(pdev));
>>> +
>>>    	/* We don't know which BAR will be used to communicate..
>>>    	 * We will map every bar with len > 0.
>>>    	 *
>>>    	 * Later, we will discover the BAR and unmap all other
>>> BARs.
>>>    	 */
>>>    	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>>> -		if (pci_resource_len(pdev, i))
>>> -			mask |= (1 << i);
>>> -	}
>>> -
>>> -	/* No BAR can be used.. */
>>> -	if (!mask) {
>>> -		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> -	snprintf(name, sizeof(name), "psnet[%s]-bars",
>>> pci_name(pdev));
>>> -	ret = pcim_iomap_regions(pdev, mask, name);
>>> -	if (ret) {
>>> -		SNET_ERR(pdev, "Failed to request and map PCI
>>> BARs\n");
>>> -		return ret;
>>> -	}
>>> +		if (pci_resource_len(pdev, i)) {
>>> +			psnet->bars[i] = pcim_iomap_region(pdev,
>>> i, name);
>>
>> Hi,
>>
>> Unrelated to the patch, but is is safe to have 'name' be on the
>> stack?
>>
>> pcim_iomap_region()
>> --> __pcim_request_region()
>> --> __pcim_request_region_range()
>> --> request_region() or __request_mem_region()
>> --> __request_region()
>> --> __request_region_locked()
>> --> res->name = name;
>>
>> So an address on the stack ends in the 'name' field of a "struct
>> resource".
> 
> Oh oh...
> 
>>
>> According to a few grep, it looks really unusual.
>>
>> I don't know if it is used, but it looks strange to me.
> 
> 
> I have seen it used in the kernel ringbuffer log when you try to
> request something that's already owned. I think it's here, right in
> __request_region_locked():
> 
> /*
>   * mm/hmm.c reserves physical addresses which then
>   * become unavailable to other users.  Conflicts are
>   * not expected.  Warn to aid debugging if encountered.
>   */
> if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
> 	pr_warn("Unaddressable device %s %pR conflicts with %pR",
> 		conflict->name, conflict, res);
> }
> 
> 
> Assuming I interpret the code correctly:
> The conflicting resource is found when a new caller (e.g. another
> driver) tries to get the same region. So conflict->name on the original
> requester's stack is by now gone and you do get UB.
> 
> Very unlikely UB, since only rarely drivers race for the same resource,
> but still UB.
> 
> But there's also a few other places. Grep for "conflict->name".
> 
>>
>>
>> If it is an issue, it was apparently already there before this patch.
> 
> I think this has to be fixed.
> 
> Question would just be whether one wants to fix it locally in this
> driver, or prevent it from happening globally by making the common
> infrastructure copy the string.
> 
> 
> P.
> 

Not a perfect script, but the below coccinelle script only find this 
place, so I would +1 only fixing things here only.

Agree?

CJ



@@
identifier name;
expression x;
constant N;
@@
	char name[N];
	...
(
*	pcim_iomap_region(..., name, ...);
|
*	pcim_iomap_regions(..., name, ...);
|
*	request_region(..., name, ...);
|
*	x = pcim_iomap_region(..., name, ...);
|
*	x = pcim_iomap_regions(..., name, ...);
|
*	x = request_region(..., name, ...);
)