| 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
| ||
|
Message-ID: <20141112192819.GB2651@sean.stalley.intel.com>
Date: Wed, 12 Nov 2014 11:28:19 -0800
From: "Sean O. Stalley" <sean.stalley@...el.com>
To: Oliver Neukum <oneukum@...e.de>
Cc: Stephanie Wallick <stephanie.s.wallick@...el.com>,
linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, devel@...verdev.osuosl.org
Subject: Re: [V2 PATCH 02/10] added media agnostic (MA) USB HCD roothubs
Thank you for your review. My responses are inline.
Greg has requested that we clean up the driver internally before
we resubmit another patchset to the mailing list. I will make
sure the changes you requested make it in, but it may be a while
before you see a patchset with the fixes included.
Thanks,
Sean O. Stalley
On Wed, Nov 12, 2014 at 09:35:42AM +0100, Oliver Neukum wrote:
> On Mon, 2014-11-10 at 18:09 -0800, Stephanie Wallick wrote:
> > diff --git a/drivers/staging/mausb/drivers/mausb_hub.c b/drivers/staging/mausb/drivers/mausb_hub.c
> > new file mode 100644
> > index 0000000..63c0fe4
> > --- /dev/null
> > +++ b/drivers/staging/mausb/drivers/mausb_hub.c
>
> > +/**
> > + * Returns true if the given is the superspeed HCD. Note: The primary HCD is
> > + * High Speed and the shared HCD is SuperSpeed.
> > + */
>
> Why in that order?
>
We should probably switch this & make the superspeed hub primary.
That way we match the xhci driver.
> > +bool mausb_is_ss_hcd(struct usb_hcd *hcd)
> > +{
> > + if (usb_hcd_is_primary_hcd(hcd))
> > + return false;
> > + else
> > + return true;
> > +}
>
>
>
> > +
> > +/**
> > + * Called by usb core when polling for a port status change.
> > + *
> > + * @hcd: USB HCD being polled.
> > + * @buf: Holds port status changes (if any).
> > + *
> > + * Returns zero if there is no status change, otherwise returns number of
> > + * bytes in buf. When there is a status change on a port, the bit indexed
> > + * at the port number + 1 (e.g. bit 2 for port 1) is set in the buffer.
> > + */
> > +int mausb_hub_status_data(struct usb_hcd *hcd, char *buf)
> > +{
> > + int i;
> > + u16 port_change = 0;
> > + u32 status = 0;
> > + int ret = 1;
> > + struct mausb_hcd *mhcd = usb_hcd_to_mausb_hcd(hcd);
> > + struct mausb_root_hub *roothub = usb_hcd_to_roothub(hcd);
> > +
> > + /*
> > + * Buf should never be more that 2 bytes. USB 3.0 hubs cannot have
> > + * more than 15 downstream ports.
> > + */
> > + buf[0] = 0;
> > + if (MAUSB_ROOTHUB_NUM_PORTS > 7) {
> > + buf[1] = 0;
> > + ret++;
> > + }
>
> Endianness bug.
>
Could you elaborate?
It was my understanding that this buffer was host-endian.
Is this an unacceptable way to clear the buffer?
> > +
> > + for (i = 0; i < MAUSB_ROOTHUB_NUM_PORTS; i++) {
> > + port_change = roothub->port_status[i].wPortChange;
> > + if (port_change)
> > + status |= (1 << (i + 1));
> > + }
> > +
> > + mausb_dbg(mhcd, "%s: hub status is 0x%x\n", __func__, status);
> > +
> > + /* hcd might be suspended, resume if there is a status change */
> > + if (mhcd->disabled == 0) {
> > + if ((hcd->state == HC_STATE_SUSPENDED) && status)
> > + usb_hcd_resume_root_hub(hcd);
> > + }
> > +
> > + memcpy(buf, (char *)&status, ret);
> > +
> > + return status ? ret : 0;
> > +}
> > +
> > +/**
> > + * Sets the bitfields in the hub descriptor of the 2.0 root hub. Always
> > + * returns zero.
> > + */
> > +int mausb_set_hub_descriptor(struct usb_hub_descriptor *hub_des)
> > +{
> > + /* set the values to the default */
> > + hub_des->bDescLength = sizeof(struct usb_hub_descriptor);
> > + hub_des->bDescriptorType = USB_DT_HUB;
> > + hub_des->bNbrPorts = MAUSB_ROOTHUB_NUM_PORTS;
> > + hub_des->wHubCharacteristics = MAUSB_ROOTHUB_CHAR;
> > + hub_des->bPwrOn2PwrGood = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
> > + hub_des->bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
>
> Is that descriptor in bus or host endianness?
>
All of the fields are little-endian. We should be using cpu_to_le16()
when setting wHubCharacteristics.
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * Sets the bitfields in the hub descriptor of the 3.0 root hub. Always
> > + * returns zero.
>
> Then why return anything?
>
Good point. I will change this (and mausb_set_hub_descriptor()) to be void.
> > + */
> > +int mausb_set_ss_hub_descriptor(struct usb_hub_descriptor *hub_des)
> > +{
> > + /* set the values to the default */
> > + hub_des->bDescLength = sizeof(struct usb_hub_descriptor);
> > + hub_des->bDescriptorType = USB_DT_SS_HUB;
> > + hub_des->bNbrPorts = MAUSB_ROOTHUB_NUM_PORTS;
> > + hub_des->wHubCharacteristics = MAUSB_ROOTHUB_CHAR;
> > + hub_des->bPwrOn2PwrGood = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
> > + hub_des->bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
> > +
> > + /* USB3-specific parameters */
> > + hub_des->u.ss.bHubHdrDecLat = MAUSB_ROOTHUB_HDR_DEC_LAT;
> > + hub_des->u.ss.wHubDelay = MAUSB_ROOTHUB_DELAY;
> > + hub_des->u.ss.DeviceRemovable = MAUSB_ALL_DEV_REMOVABLE;
> > +
> > + return 0;
> > +}
>
>
> > +/**
> > + * Contains all the structures required to emulate a root hub. One instance
> > + * exists per root hub.
> > + */
> > +struct __attribute__((__packed__)) mausb_root_hub {
>
> Why __packed__ ?
Doesn't need to be. I will remove.
> > +
> > + /* hub parameters */
> > + struct usb_hub_descriptor descriptor;
> > + struct usb_hub_status status;
> > +
> > + /* port parameters*/
> > + struct usb_port_status port_status[MAUSB_ROOTHUB_NUM_PORTS];
> > +
> > + /* root hub state */
> > + enum mausb_rh_state rh_state;
> > +
> > +};
>
> HTH
> Oliver
>
> --
> Oliver Neukum <oneukum@...e.de>
>
--
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/
Powered by blists - more mailing lists