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