Re: [PATCH v7 2/8] usb: misc: qcom_eud: add sysfs attribute for port selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race condition between 'port_store' and 'enable_store' allows userspace to bypass hardware safety checks and corrupt hardware state.
- [Medium] The software state 'chip->port_idx' is not synchronized with the hardware register 'EUD_REG_PORT_SEL' during probe.
--

commit 9b038bddb764549d4a4936b0665df772d79af0b5
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: add sysfs attribute for port selection

EUD can be mapped to either the primary USB port or the secondary USB port
depending on the value of the EUD_PORT_SEL register. Add a 'port' sysfs
attribute to allow userspace to select which port EUD should operate on
and update the ABI documentation.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 926419ca560fc..25a6a8f110ec8 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
[ ... ]
> @@ -104,8 +113,38 @@ static ssize_t enable_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(enable);
>  
> +static ssize_t port_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct eud_chip *chip = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%s\n", eud_port_names[chip->port_idx]);

[Severity: Medium]
Is the software state 'chip->port_idx' synchronized with the hardware register
EUD_REG_PORT_SEL during probe?

If the system boots with EUD_REG_PORT_SEL left in a non-zero state (e.g. from
a warm reboot or bootloader), the software 'chip->port_idx' (implicitly
initialized to 0 during probe) will be out of sync. This causes port_show() to
report the wrong active port and enable_eud() to power on the wrong USB PHY.

Should the driver explicitly enforce the default software state by writing 0 to
the register during probe, or read the register to populate 'chip->port_idx'?

> +}
> +
> +static ssize_t port_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct eud_chip *chip = dev_get_drvdata(dev);
> +	int port;
> +
> +	port = sysfs_match_string(eud_port_names, buf);
> +	if (port < 0)
> +		return port;
> +
> +	/* Port selection must be done before enabling EUD */
> +	if (chip->enabled)
> +		return -EBUSY;

[Severity: Medium]
Does this check introduce a Time-of-Check to Time-of-Use (TOCTOU) race
condition against enable_store()?

Because this accesses 'chip->enabled' without holding a lock (even after
'state_lock' is introduced for enable_store() later in the series), concurrent
writes to the 'enable' and 'port' sysfs attributes could allow port selection
to proceed simultaneously with the EUD enable sequence.

Modifying the port selection register concurrently with the EUD enable sequence
violates the hardware constraint mentioned above, potentially causing undefined
hardware behavior.

> +
> +	writel(port, chip->base + EUD_REG_PORT_SEL);
> +	chip->port_idx = port;

[Severity: Medium]
Can concurrent writes to the 'port' sysfs attribute corrupt the state here?

These operations modify hardware and software state without synchronization.
Concurrent writes to port_store() could leave the software state
'chip->port_idx' permanently out of sync with the hardware register.

> +
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831030147.1500616-1-elson.serrao@oss.qualcomm.com?part=2