[PATCH v2 0/2] blktrace: gate the blktrace interface behind CAP_SYS_ADMIN

[PATCH v2 0/2] blktrace: gate the blktrace interface behind CAP_SYS_ADMIN

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

hello,

This series gates the blktrace interface behind CAP_SYS_ADMIN.
BLKTRACESETUP allocates user-sized relay buffers and sets up traces
that outlive the creating fd, but holding the block device node, an
sg node, or write access to the sysfs trace attributes is the only
requirement to drive that state today.  Split by tree so each patch
stays in one subsystem: patch 1 gates the block side (the ioctl
dispatch and the sysfs store) and documents the sysfs attributes,
patch 2 gates the sg passthrough.  The two are independent and can
be applied in any order; patch 1 should go through the block tree,
patch 2 needs a SCSI Ack (taking both through block with the Ack
works too).

Unprivileged callers get a single answer (-EPERM for the ioctls,
-EACCES for the sysfs store) instead of values that revealed whether
a trace existed and was running.  With CONFIG_BLK_DEV_IO_TRACE=n the
sg passthrough still returns -ENOTTY, as before.  Privileged callers
behave identically with and without the series.

Verified on next-20261104 under qemu: with the series applied,
BLKTRACESETUP with all capabilities dropped is rejected with -EPERM
through both /dev/sda and /dev/sg0, and a dropped-privilege write to
the sysfs enable attribute is rejected with -EACCES; without the
series the unprivileged ioctl and the sysfs write both succeed.

Base: next-20261104.

Changes since v1:
 - also gate the sysfs trace attribute group, which reached the same
   tracing state without any check (found in follow-up review)
 - document the sysfs trace group in Documentation/ABI, where it has
   been missing since 2009
 - gate the sg passthrough commands in a new patch; the v1 commit
   message mentioned the sg path but gated only blk_trace_ioctl()
   (flagged by sashiko's review of the first posting)
 - commit messages now claim only what each patch covers

Link: https://lore.kernel.org/all/20261106142852.3233950-1-cui.tao@linux.dev/

Tao Cui (2):
  blktrace: require CAP_SYS_ADMIN for the blktrace interface
  scsi: sg: Require CAP_SYS_ADMIN for BLKTRACE passthrough ioctls

 Documentation/ABI/stable/sysfs-block | 18 ++++++++++++++++++
 drivers/scsi/sg.c                    | 10 ++++++++++
 kernel/trace/blktrace.c              |  6 ++++++
 3 files changed, 34 insertions(+)

-- 
2.53.0

[PATCH v2 1/2] blktrace: require CAP_SYS_ADMIN for the blktrace interface

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

BLKTRACESETUP allocates user-sized relay buffers and sets up a trace
that outlives the creating fd.  Every command of the interface,
BLKTRACESETUP, BLKTRACESETUP2, BLKTRACESTART, BLKTRACESTOP,
BLKTRACETEARDOWN and, on x86_64 with CONFIG_COMPAT, the 32-bit
variant can be issued by anyone who can open the block device node,
and the sysfs trace attribute group reaches the same tracing state
without any check.  A container that is handed the node can hold
trace state and relay memory on a shared queue; the relay files
themselves stay root-only (0400).

Gate blk_trace_ioctl() and sysfs_blk_trace_attr_store() with
CAP_SYS_ADMIN.  The ioctls return -EPERM, the sysfs store -EACCES
like the md store callbacks it resembles.  Unprivileged callers now
get one answer that depends only on their credentials, instead of
values that revealed whether a trace existed and was running.

Traces survive the creating fd by design, so a creator that drops
its privileges can no longer tear down its own trace.  Such traces
wait for a privileged teardown or for the queue to go away;
blk_trace_remove() from sg_device_destroy() and
blk_trace_shutdown() from blk_unregister_queue() bound that wait for
unplugged devices.
Document the sysfs attributes in Documentation/ABI, where the group
has been missing since it was added.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 Documentation/ABI/stable/sysfs-block | 18 ++++++++++++++++++
 kernel/trace/blktrace.c              |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
index 900b3fc4c72d..177c4b95a47d 100644
--- a/Documentation/ABI/stable/sysfs-block
+++ b/Documentation/ABI/stable/sysfs-block
@@ -939,3 +939,21 @@ Description:
 		==  ==============================================
 
 		For more details refer Documentation/admin-guide/iostats.rst
+
+
+What:		/sys/block/<disk>/trace/{enable,act_mask,pid,start_lba,end_lba}
+		/sys/block/<disk>/<partition>/trace/{enable,act_mask,pid,start_lba,end_lba}
+Date:		January 2009
+Contact:	Steven Rostedt <rostedt@goodmis.org>, Masami Hiramatsu <mhiramat@kernel.org>
+Description:
+		Configure blktrace for this device's request queue.
+		"enable" creates (1) or removes (0) the per-queue trace
+		bookkeeping; "act_mask" selects the traced action
+		categories, either as a number or as a list of category
+		names; "pid" restricts tracing to one process;
+		"start_lba" and "end_lba" bound the traced sector range.
+		Reading an attribute shows the current setting and needs
+		no privilege; writing any attribute requires
+		CAP_SYS_ADMIN.  Enabling from sysfs does not set up
+		relay buffers; full tracing is armed through the
+		BLKTRACESETUP ioctl.
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 8cd2520b4c99..f56d57a924bf 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);
@@ -2079,6 +2082,9 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	u64 value;
 	ssize_t ret = -EINVAL;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
 	if (count == 0)
 		goto out;
 
-- 
2.53.0

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

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

sg_ioctl_common() calls blk_trace_setup(), blk_trace_startstop()
and blk_trace_remove() directly instead of going through
blk_trace_ioctl(), so gating the block side does not close the sg
passthrough: an unprivileged process with access to an /dev/sg node
could still allocate relay buffers and control tracing state on the
underlying request queue.

Require CAP_SYS_ADMIN for the four passthrough commands.  The check
sits after the command log line so the new rejections stay visible
in the ioctl trace, and it is qualified with
IS_ENABLED(CONFIG_BLK_DEV_IO_TRACE) so kernels without blktrace
keep returning -ENOTTY for these commands instead of a new -EPERM.

BLKTRACE ioctls through sg now get the same answer they would get
through the block device node: -EPERM for unprivileged callers,
regardless of the trace or queue state behind the node.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 drivers/scsi/sg.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 6551f1284d44..f863979604ef 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -911,6 +911,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)
@@ -922,6 +929,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;
 	read_only = (O_RDWR != (filp->f_flags & O_ACCMODE));
 
 	switch (cmd_in) {
-- 
2.53.0

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

[sashiko-bot]

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