Re: [PATCH] blktrace: require CAP_SYS_ADMIN for the BLKTRACE ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] blktrace: missing CAP_SYS_ADMIN check in sg_ioctl_common()
--

commit 8392196724a6f06dfb860d63cf54fba3357cc04e
Author: Tao Cui <cuitao@kylinos.cn>

blktrace: require CAP_SYS_ADMIN for the BLKTRACE ioctls

This commit adds a CAP_SYS_ADMIN capability check to blk_trace_ioctl() to
restrict block tracing ioctls to privileged users.

> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 8cd2520b4c99e..c1bf8ca350fee 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -927,6 +927,9 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
>  	int ret, start = 0;
>  	char b[BDEVNAME_SIZE];
>  
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
>  	switch (cmd) {
>  	case BLKTRACESETUP2:
>  		snprintf(b, sizeof(b), "%pg", bdev);

[Severity: Medium]
Does checking CAP_SYS_ADMIN only in blk_trace_ioctl() leave the sg
passthrough path in sg_ioctl_common() ungated?

drivers/scsi/sg.c:sg_ioctl_common() {
    ...
	case BLKTRACESETUP:
		return blk_trace_setup(sdp->device->request_queue, sdp->name,
				       MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
				       NULL, p);
	case BLKTRACESTART:
		return blk_trace_startstop(sdp->device->request_queue, 1);
	case BLKTRACESTOP:
		return blk_trace_startstop(sdp->device->request_queue, 0);
	case BLKTRACETEARDOWN:
		return blk_trace_remove(sdp->device->request_queue);
    ...
}

Because sg_ioctl_common() calls blk_trace_setup(), blk_trace_startstop()
and blk_trace_remove() directly rather than going through
blk_trace_ioctl(), an unprivileged process with access to an /dev/sg* node
can still allocate persistent relay buffers and control tracing state on
the underlying request_queue.

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