Re: [PATCH v2 2/3] mmc: meson-gx: honour the busy timeout for R1b commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] mmc: meson-gx: UB and truncation when rounding huge timeout before clamping
--

commit 2982319077ff9791c1c57722fef30aa5b7026071
Author: Igor Velkov <iav@iav.lv>

mmc: meson-gx: honour the busy timeout for R1b commands

This commit programs the CMD_CFG timeout field using cmd->busy_timeout for
R1b commands. It aims to resolve an issue where the controller prematurely
times out during long operations like erase and cache flush. The driver
calculates the timeout by rounding up to a power of two and clamping it.

> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c

[ ... ]

> @@ -216,6 +216,20 @@ static unsigned int meson_mmc_get_timeout_msecs(struct mmc_data *data)
>  	return min_t(unsigned int, timeout, SD_EMMC_CMD_TIMEOUT_MAX);
>  }
>  
> +/* For R1b the controller waits for busy to be released within this timeout. */
> +static unsigned int meson_mmc_get_cmd_timeout_msecs(struct mmc_command *cmd)
> +{
> +	unsigned int timeout = cmd->busy_timeout;
> +
> +	if (!timeout)
> +		return SD_EMMC_CMD_TIMEOUT;
> +
> +	timeout = roundup_pow_of_two(timeout);
> +
> +	return clamp_t(unsigned int, timeout, SD_EMMC_CMD_TIMEOUT,
> +		       SD_EMMC_CMD_TIMEOUT_MAX);
> +}

[Severity: Low]
Could cmd->busy_timeout hold a value large enough to cause an overflow here?

Since cmd->busy_timeout is an unsigned int representing milliseconds, it can
potentially be controlled by userspace via the MMC_IOC_CMD ioctl. If a user
provides a timeout greater than 0x80000000, passing it to
roundup_pow_of_two() calculates 1UL << 32.

On 32-bit kernels, shifting a 32-bit unsigned long by 32 bits is undefined
behavior. On 64-bit kernels, 1UL << 32 evaluates to 0x100000000, which is
truncated to 0 when assigned back to the unsigned int timeout variable.

The subsequent clamp_t() call would receive 0 (or a garbage value on 32-bit
systems) and incorrectly return the minimum timeout instead of the maximum
timeout. This might lead to a premature hardware timeout on valid long
commands.

Would it be safer to clamp the value to SD_EMMC_CMD_TIMEOUT_MAX before
rounding it to a power of two, ensuring it stays well within bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-meson-gx-busy-timeout-v2-0-8bde1adfb5b4@iav.lv?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic