[RFC] scsi: handling negative CDROM_SEND_PACKET timeouts

[RFC] scsi: handling negative CDROM_SEND_PACKET timeouts

[Wang Yingjie]

Hi,

CDROM_SEND_PACKET accepts a negative cgc.timeout and converts it into a
very long request timeout. Before sending a final fix, I'd like to check
whether rejecting negative values would break an existing userspace
convention: libcdio describes -1 as the default timeout and passes it
through to this ioctl on Linux.

scsi_cdrom_send_packet() passes the signed timeout to clock_t_to_jiffies(),
which takes an unsigned long. On x86-64 with HZ=1000 and USER_HZ=100, -5
produces ULONG_MAX, is stored back as -1 in cgc.timeout, and becomes
UINT_MAX milliseconds in the SG_IO header. The subsequent conversion and
assignment produce rq->timeout = 4294967294 jiffies, nominally about 49.7
days. This is a source-derived timeout, not a measured 49-day hang. The
unchecked conversion is still present at mainline commit
c03c1a14507dfaf8b856c2608f9ac216021fc222.

A QEMU test on 4056b3c9d3c92ea45955641dbafeb9b960e146ef used a malformed
packet with the allowed REQUEST SENSE opcode and timeout -5. The caller
was UID 65534 with no effective capabilities, after being granted access
to /dev/sr0. At the eight-second observation point it was in D state in:

  sr_block_ioctl -> scsi_ioctl -> scsi_cdrom_send_packet
    -> sg_io -> blk_execute_rq

The device-event worker was also blocked. Source reproducer, isolated
guest setup and logs are available privately on request. SG_IO already
allows the same opener to request a large timeout, so this does not by
itself establish a new privilege-boundary bypass.

The candidate below rejects negative values before the conversion, for
both native and compat callers. Earlier testing on Linux 6.12.80 recorded
a hung-task report with the minimized negative-timeout input, but no
hung-task report over a six-minute window with this guard. A timeout=1
control also completed that window without a hung-task report. The
same-base mainline before/after test has not yet been run.

The compatibility concern is in libcdio's mmc_run_cmd() and
run_mmc_cmd_linux() [1,2]. Their comments describe -1 as selecting a
default timeout, but the unsigned argument reaches cgc.timeout without
normalization when the timeout field is enabled. The kernel does not
currently give -1 that default meaning, but rejecting it could also
reject commands that complete normally today. I have not established
whether a particular application relies on this value.

Would rejecting negative values be acceptable here, or should -1 remain
accepted, for example by mapping it to the default timeout?

[1] https://github.com/libcdio/libcdio/blob/master/lib/driver/mmc/mmc.c
[2] https://github.com/libcdio/libcdio/blob/master/lib/driver/gnu_linux.c

Thanks,
Yingjie

---
Candidate for discussion, not a final patch for merging. The unchecked
conversion is already present in the initial Git import; no later refactor
is claimed as its introducing commit. This diff applies to mainline
32c52ec87d6245ee2d12ed5dd509e40062939b50.

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -805,6 +805,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;
 
 	cgc.timeout = clock_t_to_jiffies(cgc.timeout);
 	memset(&hdr, 0, sizeof(hdr));

Re: [RFC] scsi: handling negative CDROM_SEND_PACKET timeouts

[sashiko-bot]

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