linux-kernel - Re: [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info

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: <ZGTubmBViN9rTiZC@hovoldconsulting.com>
Date:   Wed, 17 May 2023 17:10:38 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
Cc:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Felipe Balbi <balbi@...nel.org>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, quic_pkondeti@...cinc.com,
        quic_ppratap@...cinc.com, quic_wcheng@...cinc.com,
        quic_jackp@...cinc.com, quic_harshq@...cinc.com,
        ahalaney@...hat.com
Subject: Re: [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space
 temporarily to read port info

On Wed, May 17, 2023 at 05:51:45PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/17/2023 1:05 PM, Johan Hovold wrote:

> >>>> +	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
> >>>> +	if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) {
> >>>> +		dev_err(dwc->dev,
> >>>> +			"Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp));
> >>>> +		ret = -EINVAL;
> >>>> +		goto unmap_reg;
> >>>> +	}
> >>>
> >>> Not sure this is needed either.
> >>>
> >>> Could this risk regressing platforms which does not have currently have
> >>> all PHYs described in DT?
> >>>
> >> No, it doesn't. AFAIK, this only tells how many ports are present as per
> >> the core consultant configuration of the device. I tried to explain what
> >> would happen incase phy's are not present in DT in [2] & [3].
> > 
> > Right, whether the PHYs are described in DT is not directly related to
> > this.
> > 
> > As long as HCS_MAX_PORTS by definition (assumption) is always
> > (dwc->num_usb3_ports + dwc->num_usb2_ports) any such machines would
> > continue to work.
> > 
> > But if you want to catch machines where this assumption does not hold,
> > you could also end up regressing machines which have so far been working
> > despite these numbers not adding up.
> > 
> > That may be acceptable, but I'm still not sure what the value of this
> > check is (e.g. as xhci core will handle basic sanity checks like usb2 +
> > usb3 <= max_ports).

>    Thanks for the review comments. Ideally the HCC_PARAMS1 must indicate 
> total number of ports supported. If not then I believe the core 
> consultant configuration is wrong.
> 
> According to the spec:
> 
> "The MaxPorts value in the HCSPARAMS1 register defines the number of
> Port Register Sets (e.g. PORTSC, PORTPMSC, and PORTLI register sets)."
> 
> So shouldn't the (usb2+usb3 ports be equal to MaxPorts to ensure each 
> port properly accesses the respective PortSC etc., ?

Sure, that's what is expected, but why do you need to add a check for
this in the glue driver all of a sudden? Your series does not seem to
rely on this. This is the xHCI driver's business (as is parsing these
registers in the first place, really).

Johan