[PATCH 0/3] SG_IO command filtering via sysfs

[PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

Currently, SG_IO ioctls are implemented so that non-CAP_SYS_RAWIO users
can send commands from a predetermined whitelist.  The whitelist is very
simple-minded though, and basically corresponds to MMC commands---the idea
being that it would be nice for local users to read/copy/burn CDs.

This was probably sensible when the whitelist was first added (in the pre-git
era), but quite a few things have changed since then:

- there is a lot more focus on not running things as root unnecessarily;
it is generally much more common to have non-root processes accessing disks
and we would like that to happen more, not less.

- there is also a lot more focus on not giving capabilities unnecessarily.
Using CAP_SYS_RAWIO, which gives full access to all commands, allows
you to send a WRITE SCSI command to a file opened for reading, which is
a nice recipe for data corruption.  A more fine-grained whitelist allows
you to give the desired access to the application.

- we've discovered that some commands conflict between the various
SCSI standards.  UNMAP (a write command) in SBC has the same number as
the obscure MMC command READ SUBCHANNEL.  As such it's allowed if a
block device is opened for reading!

This series, which was last sent in 2012 before I lost interest in the
endless discussions that followed, adds the possibility to make the filter
mutable via sysfs, so that it can be set up per device.  This of course can
go both ways; interested applications can set a wider filter, but one can
also imagine setting much more restrictive filters by default (possibly
allowing little more than INQUIRY, TEST UNIT READY, READ CAPACITY and the
like).

Back then there was opposition to giving unfettered access to "dangerous"
or "too easily destructive" commands such as WRITE SAME or PERSISTENT
RESERVE OUT to unprivileged users.  Even then, I think this objection
is now moot thanks to the following things that have happened in 2012:

- WRITE SAME commands, which were considered too destructive, have
been added to the filter since commit 25cdb6451064 ("block: allow
WRITE_SAME commands with the SG_IO ioctl", 2016-12-15, Linux 4.10).
They are basically the only non-MMC commands included in the filter,
by the way.

- persistent reservations are also allowed now via PR ioctls (commit
924d55b06347, "sd: implement the Persistent Reservation API", 2015-10-21,
Linux 4.4).  These require CAP_SYS_ADMIN, which is the same capability
that is needed to *grant* access to PR commands via the SG_IO filter.

So, here is the 2018 version of these patches.  Please review! :)

Paolo

Paolo Bonzini (3):
  block: add back queue-private command filter
  scsi: create an all-one filter for scanners
  block: add back command filter modification via sysfs

 Documentation/block/queue-sysfs.txt |  19 +++++
 block/Kconfig                       |  10 +++
 block/blk-sysfs.c                   |  43 ++++++++++++
 block/bsg-lib.c                     |   4 +-
 block/bsg.c                         |   8 +--
 block/scsi_ioctl.c                  | 136 +++++++++++++++++++++++++++++++++---
 drivers/scsi/scsi_scan.c            |  13 ++++
 drivers/scsi/sg.c                   |   6 +-
 include/linux/blkdev.h              |  18 ++++-
 include/linux/bsg.h                 |   4 +-
 10 files changed, 238 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] block: add back queue-private command filter

[Paolo Bonzini]

The command filter used to be mutable via sysfs, but this was broken
and backed out. Let's add it back. This patch adds the infrastructure
for filtering, but unlike the old code this one just adds a pointer to
request_queue, so as to make it cheaper in the majority of cases where
no special filtering is desired.

This is a revert of commit 018e044 ("block: get rid of queue-private
command filter", 2009-06-26), though with many changes.

Cc: linux-scsi@vger.kernel.org
Cc: Hannes Reinecke <hare@suse.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/blk-sysfs.c      |  2 ++
 block/bsg-lib.c        |  4 ++--
 block/bsg.c            |  8 ++++----
 block/scsi_ioctl.c     | 19 +++++++++----------
 drivers/scsi/sg.c      |  5 +++--
 include/linux/blkdev.h |  9 ++++++++-
 include/linux/bsg.h    |  4 ++--
 7 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 3772671cf2bc..d1ec150a3478 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -838,6 +838,8 @@ static void __blk_release_queue(struct work_struct *work)
 
 	blk_exit_rl(q, &q->root_rl);
 
+	kfree(q->cmd_filter);
+
 	if (q->queue_tags)
 		__blk_queue_free_tags(q);
 
diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index f3501cdaf1a6..23200d8b035d 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -41,8 +41,8 @@ static int bsg_transport_check_proto(struct sg_io_v4 *hdr)
 	return 0;
 }
 
-static int bsg_transport_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
-		fmode_t mode)
+static int bsg_transport_fill_hdr(struct request_queue *q, struct request *rq,
+				  struct sg_io_v4 *hdr, fmode_t mode)
 {
 	struct bsg_job *job = blk_mq_rq_to_pdu(rq);
 
diff --git a/block/bsg.c b/block/bsg.c
index 9a442c23a715..ec8cbf3bf734 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -69,8 +69,8 @@ static int bsg_scsi_check_proto(struct sg_io_v4 *hdr)
 	return 0;
 }
 
-static int bsg_scsi_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
-		fmode_t mode)
+static int bsg_scsi_fill_hdr(struct request_queue *q, struct request *rq,
+			     struct sg_io_v4 *hdr, fmode_t mode)
 {
 	struct scsi_request *sreq = scsi_req(rq);
 
@@ -83,7 +83,7 @@ static int bsg_scsi_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
 
 	if (copy_from_user(sreq->cmd, uptr64(hdr->request), sreq->cmd_len))
 		return -EFAULT;
-	if (blk_verify_command(sreq->cmd, mode))
+	if (blk_verify_command(q, sreq->cmd, mode))
 		return -EPERM;
 	return 0;
 }
@@ -159,7 +159,7 @@ static void bsg_scsi_free_rq(struct request *rq)
 	if (IS_ERR(rq))
 		return rq;
 
-	ret = q->bsg_dev.ops->fill_hdr(rq, hdr, mode);
+	ret = q->bsg_dev.ops->fill_hdr(q, rq, hdr, mode);
 	if (ret)
 		goto out;
 
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 533f4aee8567..5d577c89f9e6 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -34,11 +34,6 @@
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsi_cmnd.h>
 
-struct blk_cmd_filter {
-	unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
-	unsigned long write_ok[BLK_SCSI_CMD_PER_LONG];
-};
-
 static struct blk_cmd_filter blk_default_cmd_filter;
 
 /* Command group 3 is reserved and should never be used.  */
@@ -207,14 +202,18 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
 	__set_bit(GPCMD_SET_READ_AHEAD, filter->write_ok);
 }
 
-int blk_verify_command(unsigned char *cmd, fmode_t mode)
+int blk_verify_command(struct request_queue *q, unsigned char *cmd,
+		       fmode_t mode)
 {
-	struct blk_cmd_filter *filter = &blk_default_cmd_filter;
-
+	struct blk_cmd_filter *filter = q->cmd_filter;
+	
 	/* root can do any command. */
 	if (capable(CAP_SYS_RAWIO))
 		return 0;
 
+	if (!filter)
+		filter = &blk_default_cmd_filter;
+
 	/* Anybody who can open the device can do a read-safe command */
 	if (test_bit(cmd[0], filter->read_ok))
 		return 0;
@@ -234,7 +233,7 @@ static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
 
 	if (copy_from_user(req->cmd, hdr->cmdp, hdr->cmd_len))
 		return -EFAULT;
-	if (blk_verify_command(req->cmd, mode))
+	if (blk_verify_command(q, req->cmd, mode))
 		return -EPERM;
 
 	/*
@@ -468,7 +467,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 	if (in_len && copy_from_user(buffer, sic->data + cmdlen, in_len))
 		goto error;
 
-	err = blk_verify_command(req->cmd, mode);
+	err = blk_verify_command(q, req->cmd, mode);
 	if (err)
 		goto error;
 
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 8a254bb46a9b..1b04016d3bb8 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -240,11 +240,12 @@ static int sg_check_file_access(struct file *filp, const char *caller)
 static int sg_allow_access(struct file *filp, unsigned char *cmd)
 {
 	struct sg_fd *sfp = filp->private_data;
+	struct scsi_device *device = sfp->parentdp->device;
 
-	if (sfp->parentdp->device->type == TYPE_SCANNER)
+	if (device->type == TYPE_SCANNER)
 		return 0;
 
-	return blk_verify_command(cmd, filp->f_mode);
+	return blk_verify_command(device->request_queue, cmd, filp->f_mode);
 }
 
 static int
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6980014357d4..df46a36c9467 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -352,6 +352,11 @@ struct blk_queue_tag {
 #define BLK_SCSI_MAX_CMDS	(256)
 #define BLK_SCSI_CMD_PER_LONG	(BLK_SCSI_MAX_CMDS / (sizeof(long) * 8))
 
+struct blk_cmd_filter {
+	unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
+	unsigned long write_ok[BLK_SCSI_CMD_PER_LONG];
+};
+
 /*
  * Zoned block device models (zoned limit).
  */
@@ -639,6 +644,7 @@ struct request_queue {
 	bsg_job_fn		*bsg_job_fn;
 	struct bsg_class_device bsg_dev;
 #endif
+	struct blk_cmd_filter	*cmd_filter;
 
 #ifdef CONFIG_BLK_DEV_THROTTLING
 	/* Throttle data */
@@ -1424,7 +1430,8 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
 				    gfp_mask, 0);
 }
 
-extern int blk_verify_command(unsigned char *cmd, fmode_t mode);
+extern int blk_verify_command(struct request_queue *q, unsigned char *cmd,
+			      fmode_t mode);
 
 enum blk_default_limits {
 	BLK_MAX_SEGMENTS	= 128,
diff --git a/include/linux/bsg.h b/include/linux/bsg.h
index dac37b6e00ec..461ac68691b9 100644
--- a/include/linux/bsg.h
+++ b/include/linux/bsg.h
@@ -9,8 +9,8 @@
 #ifdef CONFIG_BLK_DEV_BSG
 struct bsg_ops {
 	int	(*check_proto)(struct sg_io_v4 *hdr);
-	int	(*fill_hdr)(struct request *rq, struct sg_io_v4 *hdr,
-				fmode_t mode);
+	int	(*fill_hdr)(struct request_queue *q, struct request *rq,
+			    struct sg_io_v4 *hdr, fmode_t mode);
 	int	(*complete_rq)(struct request *rq, struct sg_io_v4 *hdr);
 	void	(*free_rq)(struct request *rq);
 };
-- 
1.8.3.1

[PATCH 2/3] scsi: create an all-one filter for scanners

[Paolo Bonzini]

Any command is allowed for scanners when /dev/sg is used.
Reimplement this using customizable command filters, so that the
sysfs knobs will work in this case, too.

Cc: linux-scsi@vger.kernel.org
Cc: Hannes Reinecke <hare@suse.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/scsi/scsi_scan.c | 13 +++++++++++++
 drivers/scsi/sg.c        |  3 ---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 78ca63dfba4a..ceb7f5535f44 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -844,6 +844,19 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			*bflags |= BLIST_NOREPORTLUN;
 	}
 
+	if (sdev->type == TYPE_SCANNER) {
+		sdev->request_queue->cmd_filter =
+			kzalloc(sizeof(struct blk_cmd_filter), GFP_KERNEL);
+
+		if (!sdev->request_queue->cmd_filter)
+			return SCSI_SCAN_NO_RESPONSE;
+
+		memset(sdev->request_queue->cmd_filter->read_ok, 0xFF,
+		       sizeof(sdev->request_queue->cmd_filter->read_ok));
+		memset(sdev->request_queue->cmd_filter->write_ok, 0xFF,
+		       sizeof(sdev->request_queue->cmd_filter->write_ok));
+	}
+
 	/*
 	 * For a peripheral qualifier (PQ) value of 1 (001b), the SCSI
 	 * spec says: The device server is capable of supporting the
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 1b04016d3bb8..e04acf41f283 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -242,9 +242,6 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	struct sg_fd *sfp = filp->private_data;
 	struct scsi_device *device = sfp->parentdp->device;
 
-	if (device->type == TYPE_SCANNER)
-		return 0;
-
 	return blk_verify_command(device->request_queue, cmd, filp->f_mode);
 }
 
-- 
1.8.3.1

[PATCH 3/3] block: add back command filter modification via sysfs

[Paolo Bonzini]

This adds two new sysfs attributes to the queue kobject.  The attributes
allow reading and writing the whitelist of unprivileged commands.

This is again a bit different from what was removed in commit 018e044
(block: get rid of queue-private command filter, 2009-06-26), but the idea
is the same.  One difference is that it does not use a separate kobject.
Also, the supported sysfs syntax is a bit more expressive: it includes
ranges, the ability to replace all of the filter with a single command,
and does not force usage of hexadecimal.

Since the names are different, and the old ones were anyway never really
enabled, the different userspace API is not a problem.

Cc: linux-scsi@vger.kernel.org
Cc: Hannes Reinecke <hare@suse.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Documentation/block/queue-sysfs.txt |  19 ++++++
 block/Kconfig                       |  10 +++
 block/blk-sysfs.c                   |  41 +++++++++++++
 block/scsi_ioctl.c                  | 117 ++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h              |   9 +++
 5 files changed, 196 insertions(+)

diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
index 2c1e67058fd3..1fe5fe5fd80a 100644
--- a/Documentation/block/queue-sysfs.txt
+++ b/Documentation/block/queue-sysfs.txt
@@ -162,6 +162,25 @@ control of this block device to that new IO scheduler. Note that writing
 an IO scheduler name to this file will attempt to load that IO scheduler
 module, if it isn't already present in the system.
 
+sgio_read_filter (RW)
+---------------------
+When read, this file will display a list of SCSI commands (i.e. values of
+the first byte of a CDB) that are always available for unprivileged users
+via /dev/bsg, /dev/sgNN, or ioctls such as SG_IO and CDROM_SEND_PACKET.
+When written, the list of commands will be modified.  The default filter
+can be restored by writing "default" to the file; otherwise the input should
+be a list of byte values or ranges such as "0x00-0xff".  In the latter case,
+instead of replacing the filter completely you can add to the commands,
+by writing a string that begins with '+', or remove them by writing a
+string that begins with '-'.
+
+sgio_write_filter (RW)
+----------------------
+When read, this file will display a list of SCSI commands (i.e. values of
+the first byte of a CDB) that are available for unprivileged users
+when the block device is open for writing.  Writing to this file behaves
+as for sgio_read_filter.
+
 write_cache (RW)
 ----------------
 When read, this file will display whether the device has write back
diff --git a/block/Kconfig b/block/Kconfig
index 1f2469a0123c..a5bc37d50e07 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -71,6 +71,16 @@ config BLK_DEV_BSG
 	  access device serial numbers, etc.
 
 	  If unsure, say Y.
+ 
+config BLK_DEV_SG_FILTER_SYSFS
+	bool "Customizable SG_IO filters in sysfs"
+	default y
+	help
+	  Saying Y here will let you use sysfs to customize the list
+	  of SCSI commands that are available (via /dev/sg, /dev/bsg or
+	  ioctls such as SG_IO) to unprivileged users.
+
+	  If unsure, say Y.
 
 config BLK_DEV_BSGLIB
 	bool "Block layer SG support v4 helper lib"
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index d1ec150a3478..ea2a47272bcf 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -703,6 +703,43 @@ static ssize_t queue_dax_show(struct request_queue *q, char *page)
 };
 #endif
 
+#ifdef CONFIG_BLK_DEV_SG_FILTER_SYSFS
+static ssize_t queue_sgio_filter_read_show(struct request_queue *q, char *page)
+{
+	return blk_filter_show(q, page, READ);
+}
+
+static ssize_t queue_sgio_filter_write_show(struct request_queue *q,
+					    char *page)
+{
+	return blk_filter_show(q, page, WRITE);
+}
+
+static ssize_t queue_sgio_filter_read_store(struct request_queue *q,
+					    const char *page, size_t count)
+{
+	return blk_filter_store(q, page, count, READ);
+}
+
+static ssize_t queue_sgio_filter_write_store(struct request_queue *q,
+					     const char *page, size_t count)
+{
+	return blk_filter_store(q, page, count, WRITE);
+}
+
+static struct queue_sysfs_entry queue_sgio_filter_read_entry = {
+	.attr = { .name = "sgio_filter_read", .mode = S_IRUGO | S_IWUSR },
+	.show = queue_sgio_filter_read_show,
+	.store = queue_sgio_filter_read_store,
+};
+
+static struct queue_sysfs_entry queue_sgio_filter_write_entry = {
+	.attr = {.name = "sgio_filter_write", .mode = S_IRUGO | S_IWUSR },
+	.show = queue_sgio_filter_write_show,
+	.store = queue_sgio_filter_write_store,
+};
+#endif
+
 static struct attribute *default_attrs[] = {
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
@@ -740,6 +777,10 @@ static ssize_t queue_dax_show(struct request_queue *q, char *page)
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	&throtl_sample_time_entry.attr,
 #endif
+#ifdef CONFIG_BLK_DEV_SG_FILTER_SYSFS
+	&queue_sgio_filter_read_entry.attr,
+	&queue_sgio_filter_write_entry.attr,
+#endif
 	NULL,
 };
 
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 5d577c89f9e6..c5f089e7d869 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 #include <linux/module.h>
 #include <linux/blkdev.h>
 #include <linux/capability.h>
@@ -725,6 +726,125 @@ void scsi_req_init(struct scsi_request *req)
 }
 EXPORT_SYMBOL(scsi_req_init);
 
+
+#ifdef CONFIG_BLK_DEV_SG_FILTER_SYSFS
+ssize_t blk_filter_show(struct request_queue *q, char *page, int rw)
+{
+	struct blk_cmd_filter *filter;
+	char *p = page;
+	unsigned long *okbits;
+	int i;
+
+	filter = q->cmd_filter;
+	if (!filter)
+		filter = &blk_default_cmd_filter;
+
+	if (rw == READ)
+		okbits = filter->read_ok;
+	else
+		okbits = filter->write_ok;
+
+	for (i = 0; i < BLK_SCSI_MAX_CMDS; i++)
+		if (test_bit(i, okbits))
+			p += sprintf(p, "0x%02x ", i);
+
+	if (p > page)
+		p[-1] = '\n';
+
+	return p - page;
+}
+EXPORT_SYMBOL_GPL(blk_filter_show);
+
+ssize_t blk_filter_store(struct request_queue *q,
+			 const char *page, size_t count, int rw)
+{
+	unsigned long bits[BLK_SCSI_CMD_PER_LONG], *target_bits;
+	bool set;
+	const char *p = page;
+	char *endp;
+	int start = -1, cmd;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (sysfs_streq(p, "default")) {
+		if (!q->cmd_filter)
+			return 0;
+		if (rw == READ)
+			memcpy(q->cmd_filter->read_ok,
+			       blk_default_cmd_filter.read_ok,
+			       sizeof(blk_default_cmd_filter.read_ok));
+		else
+			memcpy(q->cmd_filter->write_ok,
+			       blk_default_cmd_filter.write_ok,
+			       sizeof(blk_default_cmd_filter.write_ok));
+		return count;
+	}
+
+	if (!q->cmd_filter) {
+		q->cmd_filter = kmemdup(&blk_default_cmd_filter,
+					sizeof(struct blk_cmd_filter),
+					GFP_KERNEL);
+		if (!q->cmd_filter)
+			return -ENOMEM;
+	}
+
+	if (rw == READ)
+		target_bits = q->cmd_filter->read_ok;
+	else
+		target_bits = q->cmd_filter->write_ok;
+
+	set = (p[0] != '-');
+	if (p[0] != '-' && p[0] != '+')
+		memset(bits, 0, sizeof(bits));
+	else {
+		memcpy(bits, target_bits, sizeof(bits));
+		p++;
+	}
+
+	while (p < page + count) {
+		if (start == -1 && isspace(*p)) {
+			p++;
+			continue;
+		}
+
+		cmd = simple_strtol(p, &endp, 0);
+		if (endp == p || cmd < 0 || cmd >= BLK_SCSI_MAX_CMDS)
+			return -EINVAL;
+
+		/* Verify the character immediately after the number, if any */
+		p = endp;
+		if (p < page + count) {
+			if (start == -1 && *p == '-') {
+				start = cmd;
+				p++;
+				continue;
+			}
+			if (!isspace(*p))
+				return -EINVAL;
+		}
+
+		if (start == -1)
+			start = cmd;
+
+		for (; start <= cmd; start++)
+			if (set)
+				__set_bit(start, bits);
+			else
+				__clear_bit(start, bits);
+		start = -1;
+	}
+
+	/* Did the string end with a dash?  */
+	if (start != -1)
+		return -EINVAL;
+
+	memcpy(target_bits, bits, sizeof(bits));
+	return count;
+}
+EXPORT_SYMBOL_GPL(blk_filter_store);
+#endif
+
 static int __init blk_scsi_ioctl_init(void)
 {
 	blk_set_cmd_filter_defaults(&blk_default_cmd_filter);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index df46a36c9467..5110cd6d0d16 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1430,9 +1430,18 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
 				    gfp_mask, 0);
 }
 
+/*
+ * Command filter functions
+ */
 extern int blk_verify_command(struct request_queue *q, unsigned char *cmd,
 			      fmode_t mode);
 
+#ifdef CONFIG_BLK_DEV_SG_FILTER_SYSFS
+ssize_t blk_filter_show(struct request_queue *q, char *page, int rw);
+ssize_t blk_filter_store(struct request_queue *q,
+			 const char *page, size_t count, int rw);
+#endif /* CONFIG_BLK_DEV_SG_FILTER_SYSFS */
+
 enum blk_default_limits {
 	BLK_MAX_SEGMENTS	= 128,
 	BLK_SAFE_MAX_SECTORS	= 255,
-- 
1.8.3.1

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Theodore Y. Ts'o]

I wonder if a better way of adding SG_IO command filtering is via
eBPF?  We are currently carrying a inside Google a patch which allows
a specific of SCSI commands to non-root processes --- if the process
belonged to a particular Unix group id.

It's pretty specific to our use case, in terms of the specific SCSI
commands we want to allow through.  I can imagine people wanting
different filters based on the type of the SCSI device, or a HDD's
WWID, not just a group id.  For example, this might be useful for
people wanting to do crazy things with containers --- maybe you'd
want to allow container root to send a SANITIZE ERASE command to one
of its exclusively assigned disks, but not to other HDD's.

So having something that's more general than a flat file in sysfs
might be preferable to resurrecting an interface which we would then
after to support forever, even if we come up with a more general
interface.

					- Ted

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Christoph Hellwig]

I think this goes in the wrong way.  There isn't really any point
in filtering at all if we have access to the whole device by the
file persmissions, and we generally should not allow any access for
partitions.

I think we need to simplify the selection, not add crazy amounts of
special case code.

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

On 10/11/2018 20:05, Theodore Y. Ts'o wrote:
> I wonder if a better way of adding SG_IO command filtering is via
> eBPF?  We are currently carrying a inside Google a patch which allows
> a specific of SCSI commands to non-root processes --- if the process
> belonged to a particular Unix group id.
> 
> It's pretty specific to our use case, in terms of the specific SCSI
> commands we want to allow through.  I can imagine people wanting
> different filters based on the type of the SCSI device, or a HDD's
> WWID, not just a group id.  For example, this might be useful for
> people wanting to do crazy things with containers --- maybe you'd
> want to allow container root to send a SANITIZE ERASE command to one
> of its exclusively assigned disks, but not to other HDD's.
> 
> So having something that's more general than a flat file in sysfs
> might be preferable to resurrecting an interface which we would then
> after to support forever, even if we come up with a more general
> interface.

Heh, this was exactly the answer I dreaded, because I can't deny it
makes sense. :)

My main argument against it is that while superseding an interface and
still having to support it forever sucks, having a super-complex
interface is also bad (back in 2012 I wrote
https://lwn.net/Articles/501742/ which I'm not particularly enthusiastic
about).  In many cases a combination of MAC policies, ACLs, etc. can be
just as effective.

I'm not very eBPF savvy, the question I have is: what kind of
information about the running process is available in an eBPF program?
For example, even considering only the examples you make, would it be
able to access the CDB, the capabilities and uid/gid of the task, the
SCSI device type, the WWN?  Of course you also need the mode of the file
descriptor in order to allow SANITIZE ERASE if the disk is opened for write.

Paolo

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Theodore Y. Ts'o]

On Sun, Nov 11, 2018 at 05:14:45AM -0800, Christoph Hellwig wrote:
> I think this goes in the wrong way.  There isn't really any point
> in filtering at all if we have access to the whole device by the
> file persmissions, and we generally should not allow any access for
> partitions.

It really depends on the security model being used on a particular
system.  I can easily imagine scenarios where userspace is allowed
full access to the device with respect to read/write/open, but the
security model doesn't want to allow access to various SCSI commands
such as firmware upload commands, TCG commads, the
soon-to-be-standardized Zone Activation Commands (which allow dynamic
conversion of HDD recording modes between CMR and SMR), etc.

And this is before we get to crazy container / namespace scenarios.
And *no*, let's not have a SG_IO namespace!  :-)

> I think we need to simplify the selection, not add crazy amounts of
> special case code.

I have the opposite opinions in terms of wanting more complex
filtering rules, but I also agree that special case C code is not the
answer --- and why I suggested that eBPF filtering rules is the right
way to go.

					- Ted

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Theodore Y. Ts'o]

On Sun, Nov 11, 2018 at 02:26:45PM +0100, Paolo Bonzini wrote:
> 
> I'm not very eBPF savvy, the question I have is: what kind of
> information about the running process is available in an eBPF program?
> For example, even considering only the examples you make, would it be
> able to access the CDB, the capabilities and uid/gid of the task, the
> SCSI device type, the WWN?  Of course you also need the mode of the file
> descriptor in order to allow SANITIZE ERASE if the disk is opened for write.

The basic uid/gid of the task is certainly available.  For storage
stack specific things, it's a matter of what we make available to the
eBPF function.  For example, there is an experimental netfilter
replacement which uses eBPF; obviously that requires making the packet
which is being inspecting so it can be given a thumbs up or thumbs
down result.  That's going to require letting the eBPF function to
have access to the network header, access to the connection tracking
tables, etc.

You can basically think of it as passing arguments to a function
except it's it's written in eBPF instead of C.  You can also define
eBPF functions implemented in C which can be called from eBPF code.

						- Ted

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Christoph Hellwig]

On Sun, Nov 11, 2018 at 08:42:42AM -0500, Theodore Y. Ts'o wrote:
> It really depends on the security model being used on a particular
> system.  I can easily imagine scenarios where userspace is allowed
> full access to the device with respect to read/write/open, but the
> security model doesn't want to allow access to various SCSI commands
> such as firmware upload commands, TCG commads, the
> soon-to-be-standardized Zone Activation Commands (which allow dynamic
> conversion of HDD recording modes between CMR and SMR), etc.

Well, that's what we have the security_file_ioctl() LSM hook for so that
your security model can arbitrate access to ioctls.

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

On 12/11/2018 09:20, Christoph Hellwig wrote:
> On Sun, Nov 11, 2018 at 08:42:42AM -0500, Theodore Y. Ts'o wrote:
>> It really depends on the security model being used on a particular
>> system.  I can easily imagine scenarios where userspace is allowed
>> full access to the device with respect to read/write/open, but the
>> security model doesn't want to allow access to various SCSI commands
>> such as firmware upload commands, TCG commads, the
>> soon-to-be-standardized Zone Activation Commands (which allow dynamic
>> conversion of HDD recording modes between CMR and SMR), etc.
> 
> Well, that's what we have the security_file_ioctl() LSM hook for so that
> your security model can arbitrate access to ioctls.

Doesn't that have TOC-TOU races by design?

Also, what about SG_IO giving write access to files that are only opened
read-only (and only have read permissions)?

Paolo

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

On 11/11/2018 15:14, Theodore Y. Ts'o wrote:
> On Sun, Nov 11, 2018 at 02:26:45PM +0100, Paolo Bonzini wrote:
>>
>> I'm not very eBPF savvy, the question I have is: what kind of
>> information about the running process is available in an eBPF program?
>> For example, even considering only the examples you make, would it be
>> able to access the CDB, the capabilities and uid/gid of the task, the
>> SCSI device type, the WWN?  Of course you also need the mode of the file
>> descriptor in order to allow SANITIZE ERASE if the disk is opened for write.
> 
> The basic uid/gid of the task is certainly available.  For storage
> stack specific things, it's a matter of what we make available to the
> eBPF function.  For example, there is an experimental netfilter
> replacement which uses eBPF; obviously that requires making the packet
> which is being inspecting so it can be given a thumbs up or thumbs
> down result.  That's going to require letting the eBPF function to
> have access to the network header, access to the connection tracking
> tables, etc.

Yeah, and there are even already helpers such as
bpf_get_current_uid_gid.  So that part can be done in a sort-of generic way.

I can try and do the work, but I'd like some agreement on the design
first...  For example a more important question is how would the BPF
filter be attached?  Two possibilities that come to mind are:

- add it to the /dev/sg* or /dev/sd* struct file(*) via a ioctl, and use
pass the file descriptor to the unprivileged QEMU after setting up the
BPF filter, via either fork() or SCM_RIGHTS.  This would be a very nice
model for privilege separation, but I'm afraid it would not work for
your use case

- add BPF programs to cgroups, in the form of a new
BPF_PROG_TYPE_CGROUP_CDB_FILTER or something like that.  That would also
work for my usecase, and it seems to be in line with how the network
guys are doing things.  So it would seem like the way to go.

Some other details...  Registering the first cgroup-based filter would
disable the default filter; if multiple filters are attached, the
outcomes of all filters would be AND-ed, also similarly to how socket
and sockaddr cgroup BPF works.  Finally, filters would be applied also
to processes with CAP_SYS_RAWIO, unlike the current filter.

Needless to say, this would not add special case code, but it would
still add a substantial amount of code, probably comparable to this series.

Christoph?

Paolo

(*) that's not immediate for /dev/sd*, because it would require using
the block device file's private_data; that's not possible yet via struct
block_device_operations, but as far as I can tell block devices
themselves do not need the private_data, so it is at least doable.

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Bart Van Assche]

On Fri, 2018-11-16 at 01:26 +0100, Paolo Bonzini wrote:
> Yeah, and there are even already helpers such as
> bpf_get_current_uid_gid.  So that part can be done in a sort-of generic way.
> 
> I can try and do the work, but I'd like some agreement on the design
> first...  For example a more important question is how would the BPF
> filter be attached?  Two possibilities that come to mind are:
> 
> - add it to the /dev/sg* or /dev/sd* struct file(*) via a ioctl, and use
> pass the file descriptor to the unprivileged QEMU after setting up the
> BPF filter, via either fork() or SCM_RIGHTS.  This would be a very nice
> model for privilege separation, but I'm afraid it would not work for
> your use case
> 
> - add BPF programs to cgroups, in the form of a new
> BPF_PROG_TYPE_CGROUP_CDB_FILTER or something like that.  That would also
> work for my usecase, and it seems to be in line with how the network
> guys are doing things.  So it would seem like the way to go.
> 
> Some other details...  Registering the first cgroup-based filter would
> disable the default filter; if multiple filters are attached, the
> outcomes of all filters would be AND-ed, also similarly to how socket
> and sockaddr cgroup BPF works.  Finally, filters would be applied also
> to processes with CAP_SYS_RAWIO, unlike the current filter.
> 
> Needless to say, this would not add special case code, but it would
> still add a substantial amount of code, probably comparable to this series.

All user space interfaces in the Linux kernel for storage that I'm familiar
with not only allow configuration of parameters but also make it easy to
query which parameters have been configured. The existing sysfs and configfs
interfaces demonstrate this. Using BPF to configure SG/IO access has a
significant disadvantage, namely that it is very hard to figure out what has
been configured. Figuring out what has been configured namely requires
disassembling BPF. I'm not sure anyone will be enthusiast about this.

Bart.

Re: [PATCH 3/3] block: add back command filter modification via sysfs

[Bart Van Assche]

On Sat, 2018-11-10 at 17:35 +0100, Paolo Bonzini wrote:
> +sgio_read_filter (RW)
> +---------------------
> +When read, this file will display a list of SCSI commands (i.e. values of
> +the first byte of a CDB) that are always available for unprivileged users
> +via /dev/bsg, /dev/sgNN, or ioctls such as SG_IO and CDROM_SEND_PACKET.
> +When written, the list of commands will be modified.  The default filter
> +can be restored by writing "default" to the file; otherwise the input should
> +be a list of byte values or ranges such as "0x00-0xff".  In the latter case,
> +instead of replacing the filter completely you can add to the commands,
> +by writing a string that begins with '+', or remove them by writing a
> +string that begins with '-'.
> +
> +sgio_write_filter (RW)
> +----------------------
> +When read, this file will display a list of SCSI commands (i.e. values of
> +the first byte of a CDB) that are available for unprivileged users
> +when the block device is open for writing.  Writing to this file behaves
> +as for sgio_read_filter.

This seems like an unfortunate API choice to me. Let's have a look at the
following SBC commands:
* READ(6);  opcode 08h.
* READ(10); opcode 28h.
* READ(12); opcode A8h.
* READ(16); opcode 88h.
* READ(32); opcode 7fh; service action 0009h.

I do not know any application for which it would be useful to allow some but
not all of these commands. With the proposed interface however users will
have to examine all SCSI opcodes and for each opcode they will have to decide
whether or not it should be allowed. Additionally, for opcodes like 7fh that
represent multiple commands, users will have to decide whether they want to
allow all these commands or none. That's why I think that filtering SCSI
commands based on their CDB is an unfortunate choice. Would it be sufficient
for the use cases you are looking at to group SCSI commands as follows and to
enable/disable these commands per group:
* SCSI command that read information from the medium (e.g. READ) or from the
  controller (e.g. READ CAPACITY).
* SCSI commands that modify information on the medium (e.g. WRITE).
* SCSI commands that modify controller settings (e.g. MODE SELECT or SET
  TARGET PORT GROUPS).

Bart.

Re: [PATCH 3/3] block: add back command filter modification via sysfs

[Paolo Bonzini]

On 16/11/18 06:46, Bart Van Assche wrote:
> I do not know any application for which it would be useful to allow some but
> not all of these commands. With the proposed interface however users will
> have to examine all SCSI opcodes and for each opcode they will have to decide
> whether or not it should be allowed. Additionally, for opcodes like 7fh that
> represent multiple commands, users will have to decide whether they want to
> allow all these commands or none. That's why I think that filtering SCSI
> commands based on their CDB is an unfortunate choice. Would it be sufficient
> for the use cases you are looking at to group SCSI commands as follows and to
> enable/disable these commands per group:
> * SCSI command that read information from the medium (e.g. READ) or from the
>   controller (e.g. READ CAPACITY).
> * SCSI commands that modify information on the medium (e.g. WRITE).
> * SCSI commands that modify controller settings (e.g. MODE SELECT or SET
>   TARGET PORT GROUPS).

And also:

* all SCSI commands (e.g. write microcode, vendor specific commands).

It would.  However, it would be impossible to do this without making the
filter depend on the SCSI device type.  This has been rejected in 2012.

Paolo

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

On 16/11/18 01:37, Bart Van Assche wrote:
> All user space interfaces in the Linux kernel for storage that I'm familiar
> with not only allow configuration of parameters but also make it easy to
> query which parameters have been configured. The existing sysfs and configfs
> interfaces demonstrate this. Using BPF to configure SG/IO access has a
> significant disadvantage, namely that it is very hard to figure out what has
> been configured. Figuring out what has been configured namely requires
> disassembling BPF. I'm not sure anyone will be enthusiast about this.

Well, that's a problem with BPF in general.  With great power comes
great obscurability.

Paolo

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Christoph Hellwig]

On Mon, Nov 12, 2018 at 11:17:29AM +0100, Paolo Bonzini wrote:
> > Well, that's what we have the security_file_ioctl() LSM hook for so that
> > your security model can arbitrate access to ioctls.
> 
> Doesn't that have TOC-TOU races by design?

If you want to look at the command - yes.  If you just want to filter
read vs write vs ioctl, no.

> Also, what about SG_IO giving write access to files that are only opened
> read-only (and only have read permissions)?

Allowing SG_IO on read-only permissions sounds like a reall bad idea,
filtering or not.

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

On 16/11/18 10:32, Christoph Hellwig wrote:
> On Mon, Nov 12, 2018 at 11:17:29AM +0100, Paolo Bonzini wrote:
>>> Well, that's what we have the security_file_ioctl() LSM hook for so that
>>> your security model can arbitrate access to ioctls.
>>
>> Doesn't that have TOC-TOU races by design?
> 
> If you want to look at the command - yes.  If you just want to filter
> read vs write vs ioctl, no.

Yeah, but looking at the command is what Ted wants.  The thing that we
did in RHEL was a single sysfs bool that allows unfiltered access,
because it was sort of enough and made the delta very small.  But for
upstream I want to do it right, even if that means learning all that
new-fangled BPF stuff. :)

>> Also, what about SG_IO giving write access to files that are only opened
>> read-only (and only have read permissions)?
> 
> Allowing SG_IO on read-only permissions sounds like a reall bad idea,
> filtering or not.

I would even agree, however it's allowed right now and I would be
surprised if no one was relying on it in good faith ("I'm just doing an
INQUIRY, why do I need to open O_RDWR").  And indeed:

$ sudo chmod a+r /dev/sda
$ strace -e openat sg_inq /dev/sda
openat(AT_FDCWD, "/dev/sda", O_RDONLY|O_NONBLOCK) = 3
                             ^^^^^^^^

So it would be a regression.

Paolo

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Christoph Hellwig]

On Fri, Nov 16, 2018 at 10:45:11AM +0100, Paolo Bonzini wrote:
> Yeah, but looking at the command is what Ted wants.  The thing that we
> did in RHEL was a single sysfs bool that allows unfiltered access,
> because it was sort of enough and made the delta very small.  But for
> upstream I want to do it right, even if that means learning all that
> new-fangled BPF stuff. :)

So what is this magic command?

> I would even agree, however it's allowed right now and I would be
> surprised if no one was relying on it in good faith ("I'm just doing an
> INQUIRY, why do I need to open O_RDWR").  And indeed:
> 
> $ sudo chmod a+r /dev/sda
> $ strace -e openat sg_inq /dev/sda
> openat(AT_FDCWD, "/dev/sda", O_RDONLY|O_NONBLOCK) = 3

Well, not if we only did that for unprivileged opens.

Re: [PATCH 3/3] block: add back command filter modification via sysfs

[Bart Van Assche]

On Fri, 2018-11-16 at 08:00 +0100, Paolo Bonzini wrote:
> On 16/11/18 06:46, Bart Van Assche wrote:
> > I do not know any application for which it would be useful to allow some but
> > not all of these commands. With the proposed interface however users will
> > have to examine all SCSI opcodes and for each opcode they will have to decide
> > whether or not it should be allowed. Additionally, for opcodes like 7fh that
> > represent multiple commands, users will have to decide whether they want to
> > allow all these commands or none. That's why I think that filtering SCSI
> > commands based on their CDB is an unfortunate choice. Would it be sufficient
> > for the use cases you are looking at to group SCSI commands as follows and to
> > enable/disable these commands per group:
> > * SCSI command that read information from the medium (e.g. READ) or from the
> >   controller (e.g. READ CAPACITY).
> > * SCSI commands that modify information on the medium (e.g. WRITE).
> > * SCSI commands that modify controller settings (e.g. MODE SELECT or SET
> >   TARGET PORT GROUPS).
> 
> And also:
> 
> * all SCSI commands (e.g. write microcode, vendor specific commands).
> 
> It would.  However, it would be impossible to do this without making the
> filter depend on the SCSI device type.  This has been rejected in 2012.

Do you have a link available to that discussion? Since the meaning of several
SCSI opcodes depends on the SCSI device type, I don't see how we can avoid
making filtering dependent on the SCSI device type.

Bart.

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Theodore Y. Ts'o]

On Fri, Nov 16, 2018 at 08:01:29AM +0100, Paolo Bonzini wrote:
> On 16/11/18 01:37, Bart Van Assche wrote:
> > All user space interfaces in the Linux kernel for storage that I'm familiar
> > with not only allow configuration of parameters but also make it easy to
> > query which parameters have been configured. The existing sysfs and configfs
> > interfaces demonstrate this. Using BPF to configure SG/IO access has a
> > significant disadvantage, namely that it is very hard to figure out what has
> > been configured. Figuring out what has been configured namely requires
> > disassembling BPF. I'm not sure anyone will be enthusiast about this.
> 
> Well, that's a problem with BPF in general.  With great power comes
> great obscurability.

You can also make the same complaint about kernel modules; that it's
impossible to figure exactly what a kernel modules does without
disassembling them.  However, you can a one-line description of what
it does using modinfo:

% modinfo async_pq
filename:       /lib/modules/4.19.0-00022-g831156939ae8/kernel/crypto/async_tx/async_pq.ko
license:        GPL
description:    asynchronous raid6 syndrome generation/validation
srcversion:     529102C736C4FED181C15A8
depends:        raid6_pq,async_tx,async_xor
retpoline:      Y
intree:         Y
name:           async_pq
vermagic:       4.19.0-00022-g831156939ae8 SMP mod_unload modversions 

					       - Ted

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Theodore Y. Ts'o]

On Fri, Nov 16, 2018 at 10:45:11AM +0100, Paolo Bonzini wrote:
> On 16/11/18 10:32, Christoph Hellwig wrote:
> > On Mon, Nov 12, 2018 at 11:17:29AM +0100, Paolo Bonzini wrote:
> >>> Well, that's what we have the security_file_ioctl() LSM hook for so that
> >>> your security model can arbitrate access to ioctls.
> >>
> >> Doesn't that have TOC-TOU races by design?
> > 
> > If you want to look at the command - yes.  If you just want to filter
> > read vs write vs ioctl, no.
> 
> Yeah, but looking at the command is what Ted wants.  The thing that we
> did in RHEL was a single sysfs bool that allows unfiltered access,
> because it was sort of enough and made the delta very small.  But for
> upstream I want to do it right, even if that means learning all that
> new-fangled BPF stuff. :)

I'd argue that a purpose-built eBPF access control facility is
superior to the security_file_ioctl() LSM hook because it can make
available to the authorization function access to the cached results
of the SCSI INQUIRY command, and it avoids needing to duplicate
knowledge of how to parse the parameters of the SG_IO ioctl in the LSM
module as well as in the SCSI stack.

Just because you *could* implement anything in terms of a turing
machine tape doesn't mean that it is good idea.  Similarly, just
because you *can* implement something as an LSM hook doesn't mean that
it's the best design.

> >> Also, what about SG_IO giving write access to files that are only opened
> >> read-only (and only have read permissions)?
> > 
> > Allowing SG_IO on read-only permissions sounds like a reall bad idea,
> > filtering or not.
> 
> I would even agree, however it's allowed right now and I would be
> surprised if no one was relying on it in good faith ("I'm just doing an
> INQUIRY, why do I need to open O_RDWR").  And indeed:
> 
> $ sudo chmod a+r /dev/sda
> $ strace -e openat sg_inq /dev/sda
> openat(AT_FDCWD, "/dev/sda", O_RDONLY|O_NONBLOCK) = 3
>                              ^^^^^^^^
> 
> So it would be a regression.

Ugh, that's... unfortunate.  I suppose we could try to figure out all
of the SCSI commands that would have to be white-listed to be allowed
using O_RDONLY from historical usage, but that would be a huge job,
and it's highly likely we would miss some anyway.  OTOH, this could be
called a security botch that should be fixed, and if we make a best
effort to white list all of the innocuous cases such as SCSI INQUIRY,
maybe that would be acceptible.

					- Ted

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Bart Van Assche]

On Fri, 2018-11-16 at 12:43 -0500, Theodore Y. Ts'o wrote:
> I'd argue that a purpose-built eBPF access control facility is
> superior to the security_file_ioctl() LSM hook because it can make
> available to the authorization function access to the cached results
> of the SCSI INQUIRY command, and it avoids needing to duplicate
> knowledge of how to parse the parameters of the SG_IO ioctl in the LSM
> module as well as in the SCSI stack.

If an eBPF program would decide which SG_IO commands will be executed
and which ones not, does that mean that a SCSI parser would have to be
implemented in eBPF? If so, does that mean that both the eBPF and the
LSM approach share the disadvantage of requiring to do SCSI CDB parsing
outside the SCSI core?

Bart.

Re: [PATCH 0/3] SG_IO command filtering via sysfs

[Paolo Bonzini]

On 16/11/18 19:17, Bart Van Assche wrote:
> On Fri, 2018-11-16 at 12:43 -0500, Theodore Y. Ts'o wrote:
>> I'd argue that a purpose-built eBPF access control facility is
>> superior to the security_file_ioctl() LSM hook because it can make
>> available to the authorization function access to the cached results
>> of the SCSI INQUIRY command, and it avoids needing to duplicate
>> knowledge of how to parse the parameters of the SG_IO ioctl in the LSM
>> module as well as in the SCSI stack.
> 
> If an eBPF program would decide which SG_IO commands will be executed
> and which ones not, does that mean that a SCSI parser would have to be
> implemented in eBPF? If so, does that mean that both the eBPF and the
> LSM approach share the disadvantage of requiring to do SCSI CDB parsing
> outside the SCSI core?

The LSM approach cannot do SCSI CDB parsing, unless you add a special
SCSI-specific hook called after parsing the SG_IO argument, due to race
conditions.  I'd rather not do that, however it would have that
disadvantage indeed.

The eBPF approach pushes the policy and the parsing entirely to
userspace, so I'm not sure you can say it's a disadvantage.  It's just a
different design.  If you use SG_IO you're already in for writing
userspace code that handles CDBs, sense data etc.

Paolo