Gustavo A. R. Silva wrote:
>
> On 2/1/21 21:58, Thinh Nguyen wrote:
> [..]
>
>>>> +/**
>>>> + * get_port_ssp_rate - Match the extended port status to SSP rate
>>>> + * @hdev: The hub device
>>>> + * @ext_portstatus: extended port status
>>>> + *
>>>> + * Match the extended port status speed id to the SuperSpeed Plus sublink speed
>>>> + * capability attributes. Base on the number of connected lanes and speed,
>>>> + * return the corresponding enum usb_ssp_rate.
>>>> + */
>>>> +static enum usb_ssp_rate get_port_ssp_rate(struct usb_device *hdev,
>>>> + u32 ext_portstatus)
>>>> +{
>>>> + struct usb_ssp_cap_descriptor *ssp_cap = hdev->bos->ssp_cap;
>>>> + u32 attr;
>>>> + u8 speed_id;
>>>> + u8 ssac;
>>>> + u8 lanes;
>>>> + int i;
>>>> +
>>>> + if (!ssp_cap)
>>>> + goto out;
>>>> +
>>>> + speed_id = ext_portstatus & USB_EXT_PORT_STAT_RX_SPEED_ID;
>>>> + lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
>>>> +
>>>> + ssac = le32_to_cpu(ssp_cap->bmAttributes) &
>>>> + USB_SSP_SUBLINK_SPEED_ATTRIBS;
>>>> +
>>>> + for (i = 0; i <= ssac; i++) {
>>> Why a less than or equal to comparison here?
>>> Why not just a less than comparison (i < ssac) ?
>>>
>>> Thanks
>>> --
>>> Gustavo
>> The SSAC here matches with the SSAC (Sublink Speed Attribute Count) from
>> the USB 3.2 spec. It's zero based. E.g. so SSAC of 3 is 4 number of
>> Sublink Speed Attributes. That's why "<=".
> I see, what's worthy of attention is that _i_ is being used as an index
> for bmSublinkSpeedAttr[], just a couple of lines below:
>
>>>> + u8 ssid;
>>>> +
>>>> + attr = le32_to_cpu(ssp_cap->bmSublinkSpeedAttr[i]);
> ^^^
> here
>
> are we sure that this doesn't cause an out-of-bounds read?
>
> --
> Gustavo
It can if the hub output some bogus value for SSAC. Please note that I'm
using the same logic as the previous implementation for port_speed_is_ssp().
Thanks,
Thinh
>
>>>> + ssid = FIELD_GET(USB_SSP_SUBLINK_SPEED_SSID, attr);
>>>> + if (speed_id == ssid) {
>>>> + u16 mantissa;
>>>> + u8 lse;
>>>> + u8 type;
>>>> +
>>>> + /*
>>>> + * Note: currently asymmetric lane types are only
>>>> + * applicable for SSIC operate in SuperSpeed protocol
>>>> + */
>>>> + type = FIELD_GET(USB_SSP_SUBLINK_SPEED_ST, attr);
>>>> + if (type == USB_SSP_SUBLINK_SPEED_ST_ASYM_RX ||
>>>> + type == USB_SSP_SUBLINK_SPEED_ST_ASYM_TX)
>>>> + goto out;
>>>> +
>>>> + if (FIELD_GET(USB_SSP_SUBLINK_SPEED_LP, attr) !=
>>>> + USB_SSP_SUBLINK_SPEED_LP_SSP)
>>>> + goto out;
>>>> +
>>>> + lse = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSE, attr);
>>>> + mantissa = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSM, attr);
>>>> +
>>>> + /* Convert to Gbps */
>>>> + for (; lse < USB_SSP_SUBLINK_SPEED_LSE_GBPS; lse++)
>>>> + mantissa /= 1000;
>>>> +
>>>> + if (mantissa >= 10 && lanes == 1)
>>>> + return USB_SSP_GEN_2x1;
>>>> +
>>>> + if (mantissa >= 10 && lanes == 2)
>>>> + return USB_SSP_GEN_2x2;
>>>> +
>>>> + if (mantissa >= 5 && lanes == 2)
>>>> + return USB_SSP_GEN_1x2;
>>>> +
>>>> + goto out;
>>>> + }
>>>> + }
>>>> +
>>>> +out:
>>>> + return USB_SSP_GEN_UNKNOWN;
>>>> +}
>>>> +
>>>> /*
>>>> * Return 1 if port speed is SuperSpeedPlus, 0 otherwise
>>>> * check it from the link protocol field of the current speed ID attribute.
>>>> @@ -2850,9 +2926,11 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>>>> /* extended portstatus Rx and Tx lane count are zero based */
>>>> udev->rx_lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
>>>> udev->tx_lanes = USB_EXT_PORT_TX_LANES(ext_portstatus) + 1;
>>>> + udev->ssp_rate = get_port_ssp_rate(hub->hdev, ext_portstatus);
>>>> } else {
>>>> udev->rx_lanes = 1;
>>>> udev->tx_lanes = 1;
>>>> + udev->ssp_rate = USB_SSP_GEN_UNKNOWN;
>>>> }
>>>> if (hub_is_wusb(hub))
>>>> udev->speed = USB_SPEED_WIRELESS;
