linux-kernel - Re: [PATCH v3 05/20] usb: host: xhci-plat: Add clocks support

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]

Message-ID: <20140507142616.39f32438@free-electrons.com>
Date:	Wed, 7 May 2014 14:26:16 +0200
From:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
To:	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	Felipe Balbi <balbi@...com>
Cc:	Mathias Nyman <mathias.nyman@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	linux-arm-kernel@...ts.infradead.org,
	Lior Amsalem <alior@...vell.com>,
	Tawfik Bayouk <tawfik@...vell.com>,
	Nadav Haklai <nadavh@...vell.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 05/20] usb: host: xhci-plat: Add clocks support

Felipe, Gregory,

On Tue,  6 May 2014 02:14:00 +0200, Gregory CLEMENT wrote:

> +struct xhci_plat_priv {
> +	struct clk *clk;
> +};
> +
>  static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>  {
>  	/*
> @@ -38,7 +43,8 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
>  static const struct hc_driver xhci_plat_xhci_driver = {
>  	.description =		"xhci-hcd",
>  	.product_desc =		"xHCI Host Controller",
> -	.hcd_priv_size =	sizeof(struct xhci_hcd *),
> +	.hcd_priv_size =	sizeof(struct xhci_hcd *) +
> +				sizeof(struct xhci_plat_priv),
>  
>  	/*
>  	 * generic hardware linkage
> @@ -85,6 +91,40 @@ static const struct hc_driver xhci_plat_xhci_driver = {
>  	.bus_resume =		xhci_bus_resume,
>  };
>  
> +static int xhci_plat_enable_clk(struct platform_device *pdev)
> +{
> +	struct usb_hcd *hcd = platform_get_drvdata(pdev);
> +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct xhci_plat_priv *priv = (struct xhci_plat_priv *) xhci->priv;

Unless I misread the USB code, I believe the way this patch proposes to
handle private data for the XHCI HCD is wrong and leads to memory
corruption.

By growing the size .hcd_priv_size, it increases the memory size
pointed by usb_hcd->hcd_priv. However, this pointer has nothing to do
with xhci->priv, which points to the end of the xhci_hcd structure.

I believe the confusion comes from the fact that OHCI and EHCI do
allocate the entire ohci_hcd and ehci_hcd structure as part of the
usb_hcd private data:

	.hcd_priv_size =	sizeof(struct ohci_hcd),

or

	.hcd_priv_size = 	sizeof(struct ehci_hcd),

In this case, enlarging hcd_priv_size, and having a ehci->priv or
ohci->priv pointing to the end of {ohci,ehci}_hcd structures works fine.

However, in the XHCI case, the usb_hcd private data is not used to hold
the entire xhci_hcd structure, but only a *pointer* to it:

	.hcd_priv_size =	sizeof(struct xhci_hcd *),

Therefore, adding more size to .hcd_priv_size isn't going to give extra
room at the end of the xhci_hcd structure. And therefore the whole
strategy of using xhci->priv pointing at the end of xhci_hcd is broken.

In v4, what I will do is simply to add a 'struct clk *' member to
xhci_hcd. A clock is, like a register area or an interrupt, a very
typical resource for any device, so it makes sense to have a pointer to
it from xhci_hcd. If someone complains that the clock would only be
used by xhci_plat, then I could point him to the fact that xhci_hcd
already contains members such as msix_count and msix_entries, that are
only used in xhci_pci :-)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/