Re: [PATCH v2] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

["Krzysztof Wilczyński" <kwilczynski@kernel.org>]

Hello,

> > +	bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
> > +	if (!bus->procdir)
> > +		return -ENOMEM;
> 
> [Severity: High]
> Can this direct assignment overwrite a valid pointer with NULL if a concurrent
> creation fails?
> 
> If an asynchronous probe runs concurrently without holding
> pci_rescan_remove_lock(), two threads might both observe bus->procdir as
> NULL. If one successfully creates the entry, the second could fail due to a
> name collision and return NULL, unconditionally overwriting the valid
> bus->procdir.
> 
> Could this cause subsequent child devices to erroneously create their proc
> files in the root of /proc?

[...]
> >  	sprintf(name, "%02x.%x", PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> > -	e = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR, bus->procdir,
> > -			     &proc_bus_pci_ops, dev);
> > -	if (!e)
> > +	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR,
> > +					dev->bus->procdir, &proc_bus_pci_ops,
> > +					dev);
> > +	if (!dev->procent)
> >  		return -ENOMEM;
> 
> [Severity: High]
> Does this assignment introduce a risk of a use-after-free and resource leak?
> 
> Similar to the bus directory, if concurrent threads call
> pci_proc_attach_device() for the same device, one might overwrite a
> valid dev->procent with NULL when proc_create_data() fails.
> 
> Later, pci_proc_detach_device() would call proc_remove(NULL) which does
> nothing. The procfs file would be leaked, and because the file's data points
> to the freed pci_dev struct, subsequent reads from userspace could trigger a
> use-after-free.

I will send another revision to address this feedback.

Thank you!

	Krzysztof