Re: [RFC] scsi: handling negative CDROM_SEND_PACKET timeouts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: scsi_cdrom_send_packet() rejects negative timeouts with -EINVAL
- [Medium] scsi: signed int overflow of cgc.timeout in scsi_cdrom_send_packet()
--

commit aceed06c50fb78815344c356d7d655b72431529e
Author: Wang Yingjie <x-professor@outlook.com>

scsi: handling negative CDROM_SEND_PACKET timeouts

This commit rejects negative cgc.timeout values in scsi_cdrom_send_packet()
with -EINVAL before passing the timeout to clock_t_to_jiffies().

> diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
> index 122403ec3ab8d..c615bb1e39614 100644
> --- a/drivers/scsi/scsi_ioctl.c
> +++ b/drivers/scsi/scsi_ioctl.c
> @@ -806,6 +806,8 @@ static int scsi_cdrom_send_packet(struct scsi_device *sdev, bool open_for_write,
>  	err = scsi_get_cdrom_generic_arg(&cgc, arg);
>  	if (err)
>  		return err;
> +	if (cgc.timeout < 0)
> +		return -EINVAL;

[Severity: High]
Could returning -EINVAL for negative cgc.timeout values cause a userspace
regression for callers (such as libcdio via mmc_run_cmd() and
run_mmc_cmd_linux()) that pass -1 to select the default command timeout?

When userspace invokes ioctl(fd, CDROM_SEND_PACKET, &cgc), sr_block_ioctl()
calls cdrom_ioctl() (which returns -ENOSYS) and then falls back to
scsi_ioctl() -> scsi_cdrom_send_packet(). Previously, negative cgc.timeout
values were not rejected and commands completed normally.

In drivers/scsi/scsi_ioctl.c, scsi_fill_sghdr_rq() already maps a zero
hdr->timeout to the device's default timeout:

drivers/scsi/scsi_ioctl.c:scsi_fill_sghdr_rq() {
	...
	rq->timeout = msecs_to_jiffies(hdr->timeout);
	if (!rq->timeout)
		rq->timeout = sdev->sg_timeout;
	if (!rq->timeout)
		rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
	...
}

Similarly, sr_packet() in drivers/scsi/sr.c maps non-positive timeouts to a
default timeout rather than failing:

drivers/scsi/sr.c:sr_packet() {
	...
	if (cgc->timeout <= 0)
		cgc->timeout = IOCTL_TIMEOUT;
	...
}

Would clamping negative cgc.timeout values to 0 (or only converting positive
cgc.timeout values with clock_t_to_jiffies()) avoid the unsigned conversion
into a 49.7-day timeout while preserving compatibility with callers passing
-1?

>  
>  	cgc.timeout = clock_t_to_jiffies(cgc.timeout);
>  	memset(&hdr, 0, sizeof(hdr));

[Severity: Medium]
What happens here when a positive cgc.timeout is large enough that
clock_t_to_jiffies(cgc.timeout) exceeds INT_MAX on configurations where
HZ > USER_HZ (for example, cgc.timeout >= 214748365 when HZ=1000 and
USER_HZ=100)?

Because cgc.timeout is a 32-bit signed int, storing the unsigned long return
value of clock_t_to_jiffies() back into cgc.timeout overflows and truncates
the value.

Later in scsi_cdrom_send_packet():

	hdr.timeout = jiffies_to_msecs(cgc.timeout);
	...
	if (scsi_put_cdrom_generic_arg(&cgc, arg))
		return -EFAULT;

Depending on bit 31 of the truncated 32-bit value, cgc.timeout can wrap to a
negative int (which sign-extends to unsigned long in jiffies_to_msecs(),
hits the (int)m < 0 check in __msecs_to_jiffies() to produce the same
4294967294-jiffy rq->timeout, and gets copied back to userspace as a negative
timeout by scsi_put_cdrom_generic_arg()), or wrap modulo 2^32 to a tiny
positive timeout (for example, cgc.timeout = 429496730 truncates to 4
jiffies).

Should the jiffies value be held in an unsigned long or clamped before
converting to milliseconds instead of being stored back into the signed int
cgc.timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019.cdrom-timeout.13a6f4a2f552@outlook.com?part=1