Skip to contentSkip to conversation

blktrace: gate the blktrace interface behind CAP_SYS_ADMIN

PATCH New Tao Cui 2 participants 4 messages Nov 11

Subsystems (4)
lore
Share & tools
lore

Tao Cui

author
#0 lore raw

v2 cover blktrace: gate the blktrace interface behind CAP_SYS_ADMIN

From: Tao Cui <[email protected]>

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

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(+)
Signature · 1 line
2.53.0
author
#1 lore raw

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

From: Tao Cui <[email protected]>

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.

---
 Documentation/ABI/stable/sysfs-block | 18 ++++++++++++++++++
 kernel/trace/blktrace.c              |  6 ++++++
 2 files changed, 24 insertions(+)
Documentation/ABI/stable/sysfs-block +18 -0
index 900b3fc4c72d..177c4b95a47d 100644
@@ -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 <[email protected]>, Masami Hiramatsu <[email protected]>
+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.
kernel/trace/blktrace.c +6 -0
index 8cd2520b4c99..f56d57a924bf 100644
@@ -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;
 
Signature · 1 line
2.53.0
author
#2 lore raw

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

In reply to Tao Cui

From: Tao Cui <[email protected]>

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.

---
 drivers/scsi/sg.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/scsi/sg.c +10 -0
index 6551f1284d44..f863979604ef 100644
@@ -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) {
Signature · 1 line
2.53.0
#3 lore raw

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

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() --

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 <[email protected]>

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.

Quoted 19 lines
drivers/scsi/sg.c +7 -0
index 5408f002e6c01..36a1fb7343607 100644
@@ -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)

drivers/scsi/sg.c sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp, · line 933

Quoted 6 lines
drivers/scsi/sg.c +3 -0
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2