linux-kernel - RE: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

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]

Date:	Fri, 26 Jul 2013 02:06:55 +0000
From:	Paul Zimmerman <Paul.Zimmerman@...opsys.com>
To:	"balbi@...com" <balbi@...com>
CC:	"Ivan T. Ivanov" <iivanov@...sol.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: dwc3: core: modify IO memory resource after
 deferred probe completes

> From: Felipe Balbi [mailto:balbi@...com]
> Sent: Thursday, July 25, 2013 1:52 PM
> 
> On Thu, Jul 25, 2013 at 07:46:58PM +0000, Paul Zimmerman wrote:
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 607bef8..50c833f 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -384,21 +384,6 @@ static int dwc3_probe(struct platform_device *pdev)
> > >  		dev_err(dev, "missing memory resource\n");
> > >  		return -ENODEV;
> > >  	}
> > > -	dwc->xhci_resources[0].start = res->start;
> > > -	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
> > > -					DWC3_XHCI_REGS_END;
> > > -	dwc->xhci_resources[0].flags = res->flags;
> > > -	dwc->xhci_resources[0].name = res->name;
> > > -
> > > -	res->start += DWC3_GLOBALS_REGS_START;
> > > -
> > > -	 /*
> > > -	  * Request memory region but exclude xHCI regs,
> > > -	  * since it will be requested by the xhci-plat driver.
> > > -	  */
> > > -	regs = devm_ioremap_resource(dev, res);
> > > -	if (IS_ERR(regs))
> > > -		return PTR_ERR(regs);
> > >
> > >  	if (node) {
> > >  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
> > > @@ -452,6 +437,22 @@ static int dwc3_probe(struct platform_device *pdev)
> > >  		return -EPROBE_DEFER;
> > >  	}
> > >
> > > +	dwc->xhci_resources[0].start = res->start;
> > > +	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
> > > +					DWC3_XHCI_REGS_END;
> > > +	dwc->xhci_resources[0].flags = res->flags;
> > > +	dwc->xhci_resources[0].name = res->name;
> > > +
> > > +	res->start += DWC3_GLOBALS_REGS_START;
> >
> > Ick. The driver is modifying the struct resource passed to it by the
> 
> heh...
> 
> > platform code? That seems like a layering violation, and is fragile as
> > hell. In addition to this bug, what would happen if the struct resource
> > was declared 'const'?
> 
> nothing would happen if it was declared const since platform_add_device
> makes a copy of what was declared, and that's always non-const.

OK.

> Also, this is not *modifying* what was passed, just skipping the xHCI
> address space so we don't request_mem_region() an area we won't really
> handle and prevent xhci-hcd.ko from probing.

Hmm? platform_get_resource() returns a pointer to an entry in the
platform_device's resource[] array. And "res->start +=" modifies the
entry pointed at. If it didn't, the bug fixed by this patch wouldn't
have happened.

Are you sure this code will work OK if you build the driver as a module,
modprobe it, rmmod it, and then modprobe it again? Seems like it won't,
unless the dev->resource[] array gets reinitialized in between somehow.

All this assumes I'm reading the code correctly, of course. If I'm not,
then never mind :)

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/