Re: [PATCH v2 2/2] scsi: sg: Require CAP_SYS_ADMIN for BLKTRACE passthrough ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] scsi: sg: duplicate BLKTRACE ioctl command check in
  sg_ioctl_common()
--

commit 245abdd71d86c36b043cbaf6080f0c6a16895527
Author: Tao Cui <cuitao@kylinos.cn>

scsi: sg: Require CAP_SYS_ADMIN for BLKTRACE passthrough ioctls

This commit requires CAP_SYS_ADMIN in sg_ioctl_common() before handling
the four blktrace passthrough ioctl commands.

> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 5408f002e6c01..36a1fb7343607 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -910,6 +910,13 @@ static int put_compat_request_table(struct compat_sg_req_info __user *o,
>  }
>  #endif
>  
> +/* keep in sync with the commands blk_trace_ioctl() gates */
> +static bool sg_is_blktrace_cmd(unsigned int cmd)
> +{
> +	return cmd == BLKTRACESETUP || cmd == BLKTRACESTART ||
> +	       cmd == BLKTRACESTOP || cmd == BLKTRACETEARDOWN;
> +}
> +
>  static long
>  sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,
>  		unsigned int cmd_in, void __user *p)
> @@ -921,6 +928,9 @@ sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,
>  
>  	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
>  				   "sg_ioctl: cmd=0x%x\n", (int) cmd_in));
> +	if (IS_ENABLED(CONFIG_BLK_DEV_IO_TRACE) &&
> +	    sg_is_blktrace_cmd(cmd_in) && !capable(CAP_SYS_ADMIN))
> +		return -EPERM;

[Severity: Low]
This isn't a bug, but sg_ioctl_common() already matches BLKTRACESETUP,
BLKTRACESTART, BLKTRACESTOP, and BLKTRACETEARDOWN in switch (cmd_in)
immediately below.  Checking sg_is_blktrace_cmd() before the switch
duplicates the four command constants and runs up to four extra comparisons
on every ioctl call, including the SG_IO path.  Could the capability check
be done inside the existing blktrace switch cases instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111145034.248717-1-cui.tao@linux.dev?part=2