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] [day] [month] [year] [list]
Message-ID: <01b1e7d505a2b3e670f1613ce3e6a60efd3449ab.camel@redhat.com>
Date: Tue, 20 Aug 2024 13:14:20 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
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,  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

On Tue, 2024-08-20 at 12:50 +0200, Christophe JAILLET wrote:
> 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?

Yup, sounds good. Copying the string would cause trouble (GFP flags)
anyways.

I'll provide a fix in v2.

Thanks,
P.

> 
> 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, ...);
> )
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ