* [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542)
@ 2013-01-24 15:00 Paolo Bonzini
2013-01-24 15:00 ` [PATCH 01/13] sg_io: pass request_queue to blk_verify_command Paolo Bonzini
` (12 more replies)
0 siblings, 13 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, Jens Axboe, linux-scsi
This series regards the whitelist that is used for the SG_IO ioctl. This
whitelist has three problems:
* the bitmap of allowed commands is designed for MMC devices (roughly,
"play/burn CDs without requiring root") but some opcodes overlap across SCSI
device classes and have different meanings for different classes.
* also because the bitmap of allowed commands is designed for MMC devices
only, some commands are missing even though they are generally useful and
not insecure. At least not more insecure than anything else you can
do if you have access to /dev/sdX or /dev/stX nodes.
* the whitelist can be disabled per-process but not per-disk. In addition,
the required capability (CAP_SYS_RAWIO) gives access to a range of other
resources, enough to make it insecure.
The series corrects these problems. Patches 1-4 solve the first problem,
which also has an assigned CVE, by using different bitmaps for the various
device classes. Patches 5-11 solve the second by adding more commands
to the bitmaps. Patches 12 and 13 solve the third, and were already
posted but ignored by the maintainers despite multiple pings.
Note: checkpatch hates the formatting of the command table. I know about this,
and ensured that there are no errors in the rest of the code.
Ok for the next merge window?
Paolo Bonzini (13):
sg_io: pass request_queue to blk_verify_command
sg_io: reorganize list of allowed commands
sg_io: use different default filters for each device class
sg_io: resolve conflicts between commands assigned to multiple
classes (CVE-2012-4542)
sg_io: whitelist a few more commands for rare & obsolete device types
sg_io: whitelist a few more commands for multimedia devices
sg_io: whitelist a few more commands for media changers
sg_io: whitelist a few more commands for tapes
sg_io: whitelist a few more commands for disks
sg_io: whitelist a few obsolete commands
sg_io: add list of commands that were in the consulted list but are
disabled
sg_io: remove remnants of sysfs SG_IO filters
sg_io: introduce unpriv_sgio queue flag
Documentation/block/queue-sysfs.txt | 8 +
block/blk-sysfs.c | 33 +++
block/bsg.c | 2 +-
block/scsi_ioctl.c | 440 ++++++++++++++++++++++++++++-------
drivers/scsi/scsi_scan.c | 2 +
drivers/scsi/sg.c | 3 +-
include/linux/blkdev.h | 8 +-
include/linux/genhd.h | 9 -
include/scsi/scsi.h | 1 +
9 files changed, 403 insertions(+), 103 deletions(-)
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 01/13] sg_io: pass request_queue to blk_verify_command
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 22:34 ` Tejun Heo
2013-01-24 15:00 ` [PATCH 02/13] sg_io: reorganize list of allowed commands Paolo Bonzini
` (11 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel
Cc: tj, pmatouse, FUJITA Tomonori, Doug Gilbert,
James E.J. Bottomley, linux-scsi, Jens Axboe
Adjust the blk_verify_command function to let it look at per-queue
data. This will be done in the next patch.
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Doug Gilbert <dgilbert@interlog.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/bsg.c | 2 +-
block/scsi_ioctl.c | 7 ++++---
drivers/scsi/sg.c | 3 ++-
include/linux/blkdev.h | 3 ++-
4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index ff64ae3..eb5ad59 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -187,7 +187,7 @@ static int blk_fill_sgv4_hdr_rq(struct request_queue *q, struct request *rq,
return -EFAULT;
if (hdr->subprotocol == BSG_SUB_PROTOCOL_SCSI_CMD) {
- if (blk_verify_command(rq->cmd, has_write_perm))
+ if (blk_verify_command(q, rq->cmd, has_write_perm))
return -EPERM;
} else if (!capable(CAP_SYS_RAWIO))
return -EPERM;
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 9a87daa..a737562 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -196,7 +196,8 @@ 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 has_write_perm)
+int blk_verify_command(struct request_queue *q,
+ unsigned char *cmd, fmode_t has_write_perm)
{
struct blk_cmd_filter *filter = &blk_default_cmd_filter;
@@ -225,7 +226,7 @@ static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
{
if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
return -EFAULT;
- if (blk_verify_command(rq->cmd, mode & FMODE_WRITE))
+ if (blk_verify_command(q, rq->cmd, mode & FMODE_WRITE))
return -EPERM;
/*
@@ -472,7 +473,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(rq->cmd, mode & FMODE_WRITE);
+ err = blk_verify_command(q, rq->cmd, mode & FMODE_WRITE);
if (err)
goto error;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index be2c9a6..cab816f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -217,11 +217,12 @@ static void sg_put_dev(Sg_device *sdp);
static int sg_allow_access(struct file *filp, unsigned char *cmd)
{
struct sg_fd *sfp = filp->private_data;
+ struct request_queue *q = sfp->parentdp->device->request_queue;
if (sfp->parentdp->device->type == TYPE_SCANNER)
return 0;
- return blk_verify_command(cmd, filp->f_mode & FMODE_WRITE);
+ return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
}
static int get_exclude(Sg_device *sdp)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f94bc83..0782336 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1061,7 +1061,8 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
gfp_mask);
}
-extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
+extern int blk_verify_command(struct request_queue *q,
+ unsigned char *cmd, fmode_t has_write_perm);
enum blk_default_limits {
BLK_MAX_SEGMENTS = 128,
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
2013-01-24 15:00 ` [PATCH 01/13] sg_io: pass request_queue to blk_verify_command Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 22:42 ` Tejun Heo
2013-01-24 15:00 ` [PATCH 03/13] sg_io: use different default filters for each device class Paolo Bonzini
` (10 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
To prepare for the next patches, reorganize the list of commands into
a two-way table of command numbers and device types.
One command (READ CAPACITY) was listed twice in the old table, hence
the new table has one entry less than the old one.
Right now, there is still just one bitmap and the mask is ignored,
so there is no semantic change yet.
Of course, checkpatch hates this table. It has long lines and
non-standard spacing. IMO the improved readability trumps the problems
reported by checkpatch.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 209 ++++++++++++++++++++++++++++++++--------------------
1 files changed, 130 insertions(+), 79 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index a737562..75533bd 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -115,85 +115,136 @@ static int sg_emulated_host(struct request_queue *q, int __user *p)
static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
{
- /* Basic read-only commands */
- __set_bit(TEST_UNIT_READY, filter->read_ok);
- __set_bit(REQUEST_SENSE, filter->read_ok);
- __set_bit(READ_6, filter->read_ok);
- __set_bit(READ_10, filter->read_ok);
- __set_bit(READ_12, filter->read_ok);
- __set_bit(READ_16, filter->read_ok);
- __set_bit(READ_BUFFER, filter->read_ok);
- __set_bit(READ_DEFECT_DATA, filter->read_ok);
- __set_bit(READ_CAPACITY, filter->read_ok);
- __set_bit(READ_LONG, filter->read_ok);
- __set_bit(INQUIRY, filter->read_ok);
- __set_bit(MODE_SENSE, filter->read_ok);
- __set_bit(MODE_SENSE_10, filter->read_ok);
- __set_bit(LOG_SENSE, filter->read_ok);
- __set_bit(START_STOP, filter->read_ok);
- __set_bit(GPCMD_VERIFY_10, filter->read_ok);
- __set_bit(VERIFY_16, filter->read_ok);
- __set_bit(REPORT_LUNS, filter->read_ok);
- __set_bit(SERVICE_ACTION_IN, filter->read_ok);
- __set_bit(RECEIVE_DIAGNOSTIC, filter->read_ok);
- __set_bit(MAINTENANCE_IN, filter->read_ok);
- __set_bit(GPCMD_READ_BUFFER_CAPACITY, filter->read_ok);
-
- /* Audio CD commands */
- __set_bit(GPCMD_PLAY_CD, filter->read_ok);
- __set_bit(GPCMD_PLAY_AUDIO_10, filter->read_ok);
- __set_bit(GPCMD_PLAY_AUDIO_MSF, filter->read_ok);
- __set_bit(GPCMD_PLAY_AUDIO_TI, filter->read_ok);
- __set_bit(GPCMD_PAUSE_RESUME, filter->read_ok);
-
- /* CD/DVD data reading */
- __set_bit(GPCMD_READ_CD, filter->read_ok);
- __set_bit(GPCMD_READ_CD_MSF, filter->read_ok);
- __set_bit(GPCMD_READ_DISC_INFO, filter->read_ok);
- __set_bit(GPCMD_READ_CDVD_CAPACITY, filter->read_ok);
- __set_bit(GPCMD_READ_DVD_STRUCTURE, filter->read_ok);
- __set_bit(GPCMD_READ_HEADER, filter->read_ok);
- __set_bit(GPCMD_READ_TRACK_RZONE_INFO, filter->read_ok);
- __set_bit(GPCMD_READ_SUBCHANNEL, filter->read_ok);
- __set_bit(GPCMD_READ_TOC_PMA_ATIP, filter->read_ok);
- __set_bit(GPCMD_REPORT_KEY, filter->read_ok);
- __set_bit(GPCMD_SCAN, filter->read_ok);
- __set_bit(GPCMD_GET_CONFIGURATION, filter->read_ok);
- __set_bit(GPCMD_READ_FORMAT_CAPACITIES, filter->read_ok);
- __set_bit(GPCMD_GET_EVENT_STATUS_NOTIFICATION, filter->read_ok);
- __set_bit(GPCMD_GET_PERFORMANCE, filter->read_ok);
- __set_bit(GPCMD_SEEK, filter->read_ok);
- __set_bit(GPCMD_STOP_PLAY_SCAN, filter->read_ok);
-
- /* Basic writing commands */
- __set_bit(WRITE_6, filter->write_ok);
- __set_bit(WRITE_10, filter->write_ok);
- __set_bit(WRITE_VERIFY, filter->write_ok);
- __set_bit(WRITE_12, filter->write_ok);
- __set_bit(WRITE_VERIFY_12, filter->write_ok);
- __set_bit(WRITE_16, filter->write_ok);
- __set_bit(WRITE_LONG, filter->write_ok);
- __set_bit(WRITE_LONG_2, filter->write_ok);
- __set_bit(ERASE, filter->write_ok);
- __set_bit(GPCMD_MODE_SELECT_10, filter->write_ok);
- __set_bit(MODE_SELECT, filter->write_ok);
- __set_bit(LOG_SELECT, filter->write_ok);
- __set_bit(GPCMD_BLANK, filter->write_ok);
- __set_bit(GPCMD_CLOSE_TRACK, filter->write_ok);
- __set_bit(GPCMD_FLUSH_CACHE, filter->write_ok);
- __set_bit(GPCMD_FORMAT_UNIT, filter->write_ok);
- __set_bit(GPCMD_REPAIR_RZONE_TRACK, filter->write_ok);
- __set_bit(GPCMD_RESERVE_RZONE_TRACK, filter->write_ok);
- __set_bit(GPCMD_SEND_DVD_STRUCTURE, filter->write_ok);
- __set_bit(GPCMD_SEND_EVENT, filter->write_ok);
- __set_bit(GPCMD_SEND_KEY, filter->write_ok);
- __set_bit(GPCMD_SEND_OPC, filter->write_ok);
- __set_bit(GPCMD_SEND_CUE_SHEET, filter->write_ok);
- __set_bit(GPCMD_SET_SPEED, filter->write_ok);
- __set_bit(GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL, filter->write_ok);
- __set_bit(GPCMD_LOAD_UNLOAD, filter->write_ok);
- __set_bit(GPCMD_SET_STREAMING, filter->write_ok);
- __set_bit(GPCMD_SET_READ_AHEAD, filter->write_ok);
+#define sgio_bitmap_set(cmd, mask, rw) \
+ if ((mask) != 0) __set_bit((cmd), filter->rw##_ok)
+
+#define D (1u << TYPE_DISK) /* Direct Access Block Device (SBC-3) */
+#define T (1u << TYPE_TAPE) /* Sequential Access Device (SSC-3) */
+#define L (1u << TYPE_PRINTER) /* Printer Device (SSC) */
+#define P (1u << TYPE_PROCESSOR) /* Processor Device (SPC-2) */
+#define W (1u << TYPE_WORM) /* Write Once Block Device (SBC) */
+#define R (1u << TYPE_ROM) /* C/DVD Device (MMC-6) */
+#define S (1u << TYPE_SCANNER) /* Scanner device (obsolete) */
+#define O (1u << TYPE_MOD) /* Optical Memory Block Device (SBC) */
+#define M (1u << TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */
+#define C (1u << TYPE_COMM) /* Communication devices (obsolete) */
+#define A (1u << TYPE_RAID) /* Storage Array Device (SCC-2) */
+#define E (1u << TYPE_ENCLOSURE) /* SCSI Enclosure Services device (SES-2) */
+#define B (1u << TYPE_RBC) /* Simplified Direct-Access (Reduced Block) device (RBC) */
+#define K (1u << 0x0f) /* Optical Card Reader/Writer device (OCRW) */
+#define V (1u << 0x10) /* Automation/Device Interface device (ADC-2) */
+#define F (1u << TYPE_OSD) /* Object-based Storage Device (OSD-2) */
+
+ /* control, universal except possibly RBC, read */
+
+ sgio_bitmap_set(0x00, -1 , read); // TEST UNIT READY
+ sgio_bitmap_set(0x03, -1 , read); // REQUEST SENSE
+ sgio_bitmap_set(0x12, -1 , read); // INQUIRY
+ sgio_bitmap_set(0x1A, -1 , read); // MODE SENSE(6)
+ sgio_bitmap_set(0x1B, D|T|L| W|R|O|M|A| B|K|V|F| S, read); // START STOP UNIT
+ sgio_bitmap_set(0x1C, ~B , read); // RECEIVE DIAGNOSTIC RESULTS
+ sgio_bitmap_set(0x2B, D|T| W|R|O|M| K , read); // SEEK(10)
+ sgio_bitmap_set(0x3C, ~B , read); // READ BUFFER
+ sgio_bitmap_set(0x4D, -1 , read); // LOG SENSE
+ sgio_bitmap_set(0x5A, -1 , read); // MODE SENSE(10)
+ sgio_bitmap_set(0x9E, -1 , read); // SERVICE ACTION IN(16)
+ sgio_bitmap_set(0xA0, -1 , read); // REPORT LUNS
+ sgio_bitmap_set(0xA3, D|T|L| W| O|M|A|E|B|K|V , read); // MAINTENANCE IN
+
+ /* control, universal, write */
+
+ sgio_bitmap_set(0x15, -1 , write); // MODE SELECT(6)
+ sgio_bitmap_set(0x4C, -1 , write); // LOG SELECT
+ sgio_bitmap_set(0x55, -1 , write); // MODE SELECT(10)
+
+ /* control, write */
+
+ sgio_bitmap_set(0x1E, D|T| W|R|O|M| K| F , write); // PREVENT ALLOW MEDIUM REMOVAL
+
+ /* input */
+
+ sgio_bitmap_set(0x08, D|T| P|W| O| C , read); // READ(6)
+ sgio_bitmap_set(0x25, D| W|R|O| B|K| S, read); // READ CAPACITY(10)
+ sgio_bitmap_set(0x28, D| W|R|O| B|K| C , read); // READ(10)
+ sgio_bitmap_set(0x2F, D| W|R|O , read); // VERIFY(10)
+ sgio_bitmap_set(0x37, D| O|M , read); // READ DEFECT DATA(10)
+ sgio_bitmap_set(0x3E, D| W| O , read); // READ LONG(10)
+ sgio_bitmap_set(0x88, D|T| W| O| B , read); // READ(16)
+ sgio_bitmap_set(0x8F, D|T| W| O| B , read); // VERIFY(16)
+ sgio_bitmap_set(0xA8, D| W|R|O| C , read); // READ(12)
+
+ /* write */
+
+ sgio_bitmap_set(0x04, D|T|L| R|O , write); // FORMAT UNIT
+ sgio_bitmap_set(0x0A, D|T|L|P|W| O| C , write); // WRITE(6)
+ sgio_bitmap_set(0x2A, D| W|R|O| B|K| C|S, write); // WRITE(10)
+ sgio_bitmap_set(0x2E, D| W|R|O| B|K , write); // WRITE AND VERIFY(10)
+ sgio_bitmap_set(0x35, D| W|R|O| B|K , write); // SYNCHRONIZE CACHE(10)
+ sgio_bitmap_set(0x3F, D| W| O , write); // WRITE LONG(10)
+ sgio_bitmap_set(0x8A, D|T| W| O| B , write); // WRITE(16)
+ sgio_bitmap_set(0xAA, D| W|R|O| C , write); // WRITE(12)
+ sgio_bitmap_set(0xAE, D| W| O , write); // WRITE AND VERIFY(12)
+ sgio_bitmap_set(0xEA, D| W| O , write); // WRITE_LONG_2 ??
+
+ /* (mostly) MMC */
+
+ sgio_bitmap_set(0x23, R , read); // READ FORMAT CAPACITIES
+ sgio_bitmap_set(0x42, D| R , read); // READ SUB-CHANNEL / UNMAP !!
+ sgio_bitmap_set(0x43, R , read); // READ TOC/PMA/ATIP
+ sgio_bitmap_set(0x44, T| R| V , read); // READ HEADER
+ sgio_bitmap_set(0x45, R , read); // PLAY AUDIO(10)
+ sgio_bitmap_set(0x46, R , read); // GET CONFIGURATION
+ sgio_bitmap_set(0x47, R , read); // PLAY AUDIO MSF
+ sgio_bitmap_set(0x48, D| R| B , read); // PLAY AUDIO TI / SANITIZE !!
+ sgio_bitmap_set(0x4A, R , read); // GET EVENT STATUS NOTIFICATION
+ sgio_bitmap_set(0x4B, R , read); // PAUSE/RESUME
+ sgio_bitmap_set(0x4E, R , read); // STOP PLAY/SCAN
+ sgio_bitmap_set(0x51, D| R , read); // READ DISC INFORMATION / XPWRITE(10) !!
+ sgio_bitmap_set(0x52, R , read); // READ TRACK INFORMATION
+ sgio_bitmap_set(0x5C, R , read); // READ BUFFER CAPACITY
+ sgio_bitmap_set(0xA4, R , read); // REPORT KEY
+ sgio_bitmap_set(0xAC, R|O , read); // GET PERFORMANCE / ERASE !!
+ sgio_bitmap_set(0xAD, R , read); // READ DVD STRUCTURE
+ sgio_bitmap_set(0xB9, R , read); // READ CD MSF
+ sgio_bitmap_set(0xBA, R , read); // SCAN
+ sgio_bitmap_set(0xBC, R , read); // PLAY CD
+ sgio_bitmap_set(0xBE, R , read); // READ CD
+
+ sgio_bitmap_set(0x53, D| R , write); // RESERVE TRACK / XDWRITEREAD(10)
+ sgio_bitmap_set(0x54, R , write); // SEND OPC INFORMATION
+ sgio_bitmap_set(0x58, R , write); // REPAIR TRACK
+ sgio_bitmap_set(0x5B, R , write); // CLOSE TRACK/SESSION
+ sgio_bitmap_set(0x5D, R , write); // SEND CUE SHEET
+ sgio_bitmap_set(0xA1, D| R| B , write); // BLANK / ATA PASS-THROUGH(12)
+ sgio_bitmap_set(0xA2, R , write); // SEND EVENT
+ sgio_bitmap_set(0xA3, R , write); // SEND KEY
+ sgio_bitmap_set(0xA6, R| M , write); // LOAD/UNLOAD C/DVD
+ sgio_bitmap_set(0xA7, R , write); // SET READ AHEAD
+ sgio_bitmap_set(0xB6, R| M , write); // SET STREAMING
+ sgio_bitmap_set(0xBB, R , write); // SET CD SPEED
+ sgio_bitmap_set(0xBF, R , write); // SEND DVD STRUCTURE
+
+ /* (mostly) tape */
+
+ sgio_bitmap_set(0x19, T , write); // ERASE(6)
+
+#undef D
+#undef T
+#undef L
+#undef P
+#undef W
+#undef R
+#undef S
+#undef O
+#undef M
+#undef C
+#undef A
+#undef E
+#undef B
+#undef K
+#undef V
+#undef F
+#undef sgio_bitmap_set
}
int blk_verify_command(struct request_queue *q,
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 03/13] sg_io: use different default filters for each device class
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
2013-01-24 15:00 ` [PATCH 01/13] sg_io: pass request_queue to blk_verify_command Paolo Bonzini
2013-01-24 15:00 ` [PATCH 02/13] sg_io: reorganize list of allowed commands Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 04/13] sg_io: resolve conflicts between commands assigned to multiple classes (CVE-2012-4542) Paolo Bonzini
` (9 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Store the filters in a 256-entry array, and pick an appropriate filter
for SCSI devices. Apart from SCSI disks, SG_IO is supported for CCISS,
ide-floppy and virtio-blk devices; TYPE_DISK (which is zero, i.e. the
default) is more appropriate for these devices than TYPE_ROM.
This patch already introduces some semantic change, albeit very limited;
in addition to the above change for CCISS/ide-floppy/virtio-blk, a few
commands are now forbidden for devices of type other than TYPE_ROM,
where they are reserved or vendor-specific.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 14 +++++---------
drivers/scsi/scsi_scan.c | 2 ++
include/linux/blkdev.h | 2 +-
include/scsi/scsi.h | 1 +
4 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 75533bd..e68add2 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -34,8 +34,8 @@
#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];
+ u32 read_ok[BLK_SCSI_MAX_CMDS];
+ u32 write_ok[BLK_SCSI_MAX_CMDS];
};
static struct blk_cmd_filter blk_default_cmd_filter;
@@ -116,7 +116,7 @@ static int sg_emulated_host(struct request_queue *q, int __user *p)
static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
{
#define sgio_bitmap_set(cmd, mask, rw) \
- if ((mask) != 0) __set_bit((cmd), filter->rw##_ok)
+ filter->rw##_ok[(cmd)] |= (mask);
#define D (1u << TYPE_DISK) /* Direct Access Block Device (SBC-3) */
#define T (1u << TYPE_TAPE) /* Sequential Access Device (SSC-3) */
@@ -256,16 +256,12 @@ int blk_verify_command(struct request_queue *q,
if (capable(CAP_SYS_RAWIO))
return 0;
- /* if there's no filter set, assume we're filtering everything out */
- if (!filter)
- return -EPERM;
-
/* Anybody who can open the device can do a read-safe command */
- if (test_bit(cmd[0], filter->read_ok))
+ if (filter->read_ok[cmd[0]] & (1 << q->sgio_type))
return 0;
/* Write-safe commands require a writable open */
- if (test_bit(cmd[0], filter->write_ok) && has_write_perm)
+ if (has_write_perm && filter->write_ok[cmd[0]] & (1 << q->sgio_type))
return 0;
return -EPERM;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3e58b22..86940f3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -782,6 +782,8 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
sdev->removable = (inq_result[1] & 0x80) >> 7;
}
+ sdev->request_queue->sgio_type = sdev->type;
+
switch (sdev->type) {
case TYPE_RBC:
case TYPE_TAPE:
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 0782336..b376d37 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -256,7 +256,6 @@ struct blk_queue_tag {
};
#define BLK_SCSI_MAX_CMDS (256)
-#define BLK_SCSI_CMD_PER_LONG (BLK_SCSI_MAX_CMDS / (sizeof(long) * 8))
struct queue_limits {
unsigned long bounce_pfn;
@@ -403,6 +402,7 @@ struct request_queue {
*/
unsigned int sg_timeout;
unsigned int sg_reserved_size;
+ unsigned char sgio_type;
int node;
#ifdef CONFIG_BLK_DEV_IO_TRACE
struct blk_trace *blk_trace;
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 66216c1..1a26291 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -325,6 +325,7 @@ static inline int scsi_status_is_good(int status)
#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
#define TYPE_RBC 0x0e
#define TYPE_OSD 0x11
+#define TYPE_MAX 0x1f
#define TYPE_NO_LUN 0x7f
/* SCSI protocols; these are taken from SPC-3 section 7.5 */
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 04/13] sg_io: resolve conflicts between commands assigned to multiple classes (CVE-2012-4542)
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (2 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 03/13] sg_io: use different default filters for each device class Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 05/13] sg_io: whitelist a few more commands for rare & obsolete device types Paolo Bonzini
` (8 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Some SCSI commands can be sent to disks via SG_IO even by unprivileged
users. Unfortunately, some opcodes overlap across SCSI device classes
and have different meanings for different classes. Four of them can
be used for read-only file descriptors on MMC, but should be limited to
descriptors opened for read-write on SBC:
The current bitmap of allowed commands is designed for MMC devices
(roughly, "play/burn CDs without requiring root").
- READ SUBCHANNEL <-> UNMAP (destructive, but no control on written
data)
- GET PERFORMANCE <-> ERASE (not really a problem, no one supports
ERASE anyway)
- READ DISC INFORMATION <-> XPWRITE (not commonly implemented but
most dangerous)
- PLAY AUDIO TI <-> SANITIZE (a very new command)
To fix this, the series splits the bitmap entries for these four
commands into two entries, one read-only for MMC and one read-write
for the other device classes.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e68add2..c266546 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -181,29 +181,33 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x2E, D| W|R|O| B|K , write); // WRITE AND VERIFY(10)
sgio_bitmap_set(0x35, D| W|R|O| B|K , write); // SYNCHRONIZE CACHE(10)
sgio_bitmap_set(0x3F, D| W| O , write); // WRITE LONG(10)
+ sgio_bitmap_set(0x42, D , write); // UNMAP
+ sgio_bitmap_set(0x48, D| B , write); // SANITIZE
+ sgio_bitmap_set(0x51, D , write); // XPWRITE(10)
sgio_bitmap_set(0x8A, D|T| W| O| B , write); // WRITE(16)
sgio_bitmap_set(0xAA, D| W|R|O| C , write); // WRITE(12)
+ sgio_bitmap_set(0xAC, O , write); // ERASE(12)
sgio_bitmap_set(0xAE, D| W| O , write); // WRITE AND VERIFY(12)
sgio_bitmap_set(0xEA, D| W| O , write); // WRITE_LONG_2 ??
/* (mostly) MMC */
sgio_bitmap_set(0x23, R , read); // READ FORMAT CAPACITIES
- sgio_bitmap_set(0x42, D| R , read); // READ SUB-CHANNEL / UNMAP !!
+ sgio_bitmap_set(0x42, R , read); // READ SUB-CHANNEL
sgio_bitmap_set(0x43, R , read); // READ TOC/PMA/ATIP
sgio_bitmap_set(0x44, T| R| V , read); // READ HEADER
sgio_bitmap_set(0x45, R , read); // PLAY AUDIO(10)
sgio_bitmap_set(0x46, R , read); // GET CONFIGURATION
sgio_bitmap_set(0x47, R , read); // PLAY AUDIO MSF
- sgio_bitmap_set(0x48, D| R| B , read); // PLAY AUDIO TI / SANITIZE !!
+ sgio_bitmap_set(0x48, R , read); // PLAY AUDIO TI
sgio_bitmap_set(0x4A, R , read); // GET EVENT STATUS NOTIFICATION
sgio_bitmap_set(0x4B, R , read); // PAUSE/RESUME
sgio_bitmap_set(0x4E, R , read); // STOP PLAY/SCAN
- sgio_bitmap_set(0x51, D| R , read); // READ DISC INFORMATION / XPWRITE(10) !!
+ sgio_bitmap_set(0x51, R , read); // READ DISC INFORMATION
sgio_bitmap_set(0x52, R , read); // READ TRACK INFORMATION
sgio_bitmap_set(0x5C, R , read); // READ BUFFER CAPACITY
sgio_bitmap_set(0xA4, R , read); // REPORT KEY
- sgio_bitmap_set(0xAC, R|O , read); // GET PERFORMANCE / ERASE !!
+ sgio_bitmap_set(0xAC, R , read); // GET PERFORMANCE
sgio_bitmap_set(0xAD, R , read); // READ DVD STRUCTURE
sgio_bitmap_set(0xB9, R , read); // READ CD MSF
sgio_bitmap_set(0xBA, R , read); // SCAN
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 05/13] sg_io: whitelist a few more commands for rare & obsolete device types
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (3 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 04/13] sg_io: resolve conflicts between commands assigned to multiple classes (CVE-2012-4542) Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices Paolo Bonzini
` (7 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Start cleaning up the table, moving out of the way four
rare & obsolete device types: "processor devices", printers,
communication devices (network cards) and scanners. Add
missing commands for these four device types.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
1 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index c266546..084b943 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -141,7 +141,7 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x03, -1 , read); // REQUEST SENSE
sgio_bitmap_set(0x12, -1 , read); // INQUIRY
sgio_bitmap_set(0x1A, -1 , read); // MODE SENSE(6)
- sgio_bitmap_set(0x1B, D|T|L| W|R|O|M|A| B|K|V|F| S, read); // START STOP UNIT
+ sgio_bitmap_set(0x1B, D|T| W|R|O|M|A| B|K|V|F , read); // START STOP UNIT
sgio_bitmap_set(0x1C, ~B , read); // RECEIVE DIAGNOSTIC RESULTS
sgio_bitmap_set(0x2B, D|T| W|R|O|M| K , read); // SEEK(10)
sgio_bitmap_set(0x3C, ~B , read); // READ BUFFER
@@ -163,21 +163,21 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
/* input */
- sgio_bitmap_set(0x08, D|T| P|W| O| C , read); // READ(6)
- sgio_bitmap_set(0x25, D| W|R|O| B|K| S, read); // READ CAPACITY(10)
- sgio_bitmap_set(0x28, D| W|R|O| B|K| C , read); // READ(10)
+ sgio_bitmap_set(0x08, D|T| W| O , read); // READ(6)
+ sgio_bitmap_set(0x25, D| W|R|O| B|K , read); // READ CAPACITY(10)
+ sgio_bitmap_set(0x28, D| W|R|O| B|K , read); // READ(10)
sgio_bitmap_set(0x2F, D| W|R|O , read); // VERIFY(10)
sgio_bitmap_set(0x37, D| O|M , read); // READ DEFECT DATA(10)
sgio_bitmap_set(0x3E, D| W| O , read); // READ LONG(10)
sgio_bitmap_set(0x88, D|T| W| O| B , read); // READ(16)
sgio_bitmap_set(0x8F, D|T| W| O| B , read); // VERIFY(16)
- sgio_bitmap_set(0xA8, D| W|R|O| C , read); // READ(12)
+ sgio_bitmap_set(0xA8, D| W|R|O , read); // READ(12)
/* write */
- sgio_bitmap_set(0x04, D|T|L| R|O , write); // FORMAT UNIT
- sgio_bitmap_set(0x0A, D|T|L|P|W| O| C , write); // WRITE(6)
- sgio_bitmap_set(0x2A, D| W|R|O| B|K| C|S, write); // WRITE(10)
+ sgio_bitmap_set(0x04, D|T| R|O , write); // FORMAT UNIT
+ sgio_bitmap_set(0x0A, D|T| W| O , write); // WRITE(6)
+ sgio_bitmap_set(0x2A, D| W|R|O| B|K , write); // WRITE(10)
sgio_bitmap_set(0x2E, D| W|R|O| B|K , write); // WRITE AND VERIFY(10)
sgio_bitmap_set(0x35, D| W|R|O| B|K , write); // SYNCHRONIZE CACHE(10)
sgio_bitmap_set(0x3F, D| W| O , write); // WRITE LONG(10)
@@ -185,11 +185,24 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x48, D| B , write); // SANITIZE
sgio_bitmap_set(0x51, D , write); // XPWRITE(10)
sgio_bitmap_set(0x8A, D|T| W| O| B , write); // WRITE(16)
- sgio_bitmap_set(0xAA, D| W|R|O| C , write); // WRITE(12)
+ sgio_bitmap_set(0xAA, D| W|R|O , write); // WRITE(12)
sgio_bitmap_set(0xAC, O , write); // ERASE(12)
sgio_bitmap_set(0xAE, D| W| O , write); // WRITE AND VERIFY(12)
sgio_bitmap_set(0xEA, D| W| O , write); // WRITE_LONG_2 ??
+ /* processor device */
+
+ sgio_bitmap_set(0x08, P , read); // RECEIVE
+ sgio_bitmap_set(0x0A, P , write); // SEND(6)
+
+ /* printer */
+
+ sgio_bitmap_set(0x04, L , write); // FORMAT
+ sgio_bitmap_set(0x0A, L , write); // PRINT
+ sgio_bitmap_set(0x0B, L , write); // SLEW AND PRINT
+ sgio_bitmap_set(0x10, L , write); // SYNCHRONIZE BUFFER
+ sgio_bitmap_set(0x1B, L , write); // STOP PRINT
+
/* (mostly) MMC */
sgio_bitmap_set(0x23, R , read); // READ FORMAT CAPACITIES
@@ -232,6 +245,24 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x19, T , write); // ERASE(6)
+ /* communication devices (obsolete) */
+
+ sgio_bitmap_set(0x08, C , write); // GET MESSAGE(6)
+ sgio_bitmap_set(0x0A, C , write); // SEND MESSAGE(6)
+ sgio_bitmap_set(0x28, C , write); // GET MESSAGE(10)
+ sgio_bitmap_set(0x2A, C , write); // SEND MESSAGE(10)
+ sgio_bitmap_set(0xA8, C , write); // GET MESSAGE(12)
+ sgio_bitmap_set(0xAA, C , write); // SEND MESSAGE(12)
+
+ /* scanners (obsolete) */
+
+ sgio_bitmap_set(0x1B, S, write); // SCAN
+ sgio_bitmap_set(0x24, S, write); // SET WINDOW
+ sgio_bitmap_set(0x25, S, write); // GET WINDOW
+ sgio_bitmap_set(0x2A, S, write); // SEND(10)
+ sgio_bitmap_set(0x31, S, write); // OBJECT POSITION
+ sgio_bitmap_set(0x34, S, write); // GET DATA BUFFER STATUS
+
#undef D
#undef T
#undef L
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (4 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 05/13] sg_io: whitelist a few more commands for rare & obsolete device types Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 22:55 ` Tejun Heo
2013-01-24 15:00 ` [PATCH 07/13] sg_io: whitelist a few more commands for media changers Paolo Bonzini
` (6 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Strangely, a couple of MMC commands were never included. Add them too.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 084b943..ec930f0 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -220,11 +220,14 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x52, R , read); // READ TRACK INFORMATION
sgio_bitmap_set(0x5C, R , read); // READ BUFFER CAPACITY
sgio_bitmap_set(0xA4, R , read); // REPORT KEY
+ sgio_bitmap_set(0xA5, R , read); // PLAY AUDIO(12)
+ sgio_bitmap_set(0xAB, R| V , read); // SERVICE ACTION IN(12)
sgio_bitmap_set(0xAC, R , read); // GET PERFORMANCE
sgio_bitmap_set(0xAD, R , read); // READ DVD STRUCTURE
sgio_bitmap_set(0xB9, R , read); // READ CD MSF
sgio_bitmap_set(0xBA, R , read); // SCAN
sgio_bitmap_set(0xBC, R , read); // PLAY CD
+ sgio_bitmap_set(0xBD, R , read); // MECHANISM STATUS
sgio_bitmap_set(0xBE, R , read); // READ CD
sgio_bitmap_set(0x53, D| R , write); // RESERVE TRACK / XDWRITEREAD(10)
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 07/13] sg_io: whitelist a few more commands for media changers
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (5 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 08/13] sg_io: whitelist a few more commands for tapes Paolo Bonzini
` (5 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Besides CD-ROMs, three more device types are interesting for SG_IO:
media changers, tapes and of course disks.
Starting with this patch, we will whitelist a few more commands for
these devices. For media changers, enable "INITIALIZE ELEMENT STATUS"
and "REQUEST VOLUME ELEMENT ADDRESS". A few changer-specific commands
were already enabled by chance because they overlapped commands that
are valid for other classes: "EXCHANGE MEDIUM" and "SEND VOLUME TAG",
"INITIALIZE ELEMENT STATUS WITH RANGE".
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index ec930f0..6af330a 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -141,9 +141,9 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x03, -1 , read); // REQUEST SENSE
sgio_bitmap_set(0x12, -1 , read); // INQUIRY
sgio_bitmap_set(0x1A, -1 , read); // MODE SENSE(6)
- sgio_bitmap_set(0x1B, D|T| W|R|O|M|A| B|K|V|F , read); // START STOP UNIT
+ sgio_bitmap_set(0x1B, D|T| W|R|O| A| B|K|V|F , read); // START STOP UNIT
sgio_bitmap_set(0x1C, ~B , read); // RECEIVE DIAGNOSTIC RESULTS
- sgio_bitmap_set(0x2B, D|T| W|R|O|M| K , read); // SEEK(10)
+ sgio_bitmap_set(0x2B, D|T| W|R|O| K , read); // SEEK(10)
sgio_bitmap_set(0x3C, ~B , read); // READ BUFFER
sgio_bitmap_set(0x4D, -1 , read); // LOG SENSE
sgio_bitmap_set(0x5A, -1 , read); // MODE SENSE(10)
@@ -167,7 +167,7 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x25, D| W|R|O| B|K , read); // READ CAPACITY(10)
sgio_bitmap_set(0x28, D| W|R|O| B|K , read); // READ(10)
sgio_bitmap_set(0x2F, D| W|R|O , read); // VERIFY(10)
- sgio_bitmap_set(0x37, D| O|M , read); // READ DEFECT DATA(10)
+ sgio_bitmap_set(0x37, D| O , read); // READ DEFECT DATA(10)
sgio_bitmap_set(0x3E, D| W| O , read); // READ LONG(10)
sgio_bitmap_set(0x88, D|T| W| O| B , read); // READ(16)
sgio_bitmap_set(0x8F, D|T| W| O| B , read); // VERIFY(16)
@@ -203,6 +203,17 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x10, L , write); // SYNCHRONIZE BUFFER
sgio_bitmap_set(0x1B, L , write); // STOP PRINT
+ /* media changer */
+
+ sgio_bitmap_set(0x07, M , read); // INITIALIZE ELEMENT STATUS
+ sgio_bitmap_set(0x1B, M , read); // OPEN/CLOSE IMPORT/EXPORT ELEMENT
+ sgio_bitmap_set(0x2B, M , read); // POSITION TO ELEMENT
+ sgio_bitmap_set(0x37, M , read); // INITIALIZE ELEMENT STATUS WITH RANGE
+ sgio_bitmap_set(0xB5, M , read); // REQUEST VOLUME ELEMENT ADDRESS
+
+ sgio_bitmap_set(0xA6, M , write); // EXCHANGE MEDIUM
+ sgio_bitmap_set(0xB6, M , write); // SEND VOLUME TAG
+
/* (mostly) MMC */
sgio_bitmap_set(0x23, R , read); // READ FORMAT CAPACITIES
@@ -238,9 +249,9 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0xA1, D| R| B , write); // BLANK / ATA PASS-THROUGH(12)
sgio_bitmap_set(0xA2, R , write); // SEND EVENT
sgio_bitmap_set(0xA3, R , write); // SEND KEY
- sgio_bitmap_set(0xA6, R| M , write); // LOAD/UNLOAD C/DVD
+ sgio_bitmap_set(0xA6, R , write); // LOAD/UNLOAD C/DVD
sgio_bitmap_set(0xA7, R , write); // SET READ AHEAD
- sgio_bitmap_set(0xB6, R| M , write); // SET STREAMING
+ sgio_bitmap_set(0xB6, R , write); // SET STREAMING
sgio_bitmap_set(0xBB, R , write); // SET CD SPEED
sgio_bitmap_set(0xBF, R , write); // SEND DVD STRUCTURE
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 08/13] sg_io: whitelist a few more commands for tapes
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (6 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 07/13] sg_io: whitelist a few more commands for media changers Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 09/13] sg_io: whitelist a few more commands for disks Paolo Bonzini
` (4 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Tapes have no problematic overlap, but quite a few commands
are missing that are useful when operating tapes with /dev/sg.
This patch adds them.
START STOP UNIT, FORMAT UNIT and SEEK(10) have similar meanings
but different names for tapes, so move them to the tape section
of the whitelist.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 28 ++++++++++++++++++++++++----
1 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 6af330a..49cd98a 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -141,9 +141,9 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x03, -1 , read); // REQUEST SENSE
sgio_bitmap_set(0x12, -1 , read); // INQUIRY
sgio_bitmap_set(0x1A, -1 , read); // MODE SENSE(6)
- sgio_bitmap_set(0x1B, D|T| W|R|O| A| B|K|V|F , read); // START STOP UNIT
+ sgio_bitmap_set(0x1B, D| W|R|O| A| B|K| F , read); // START STOP UNIT
sgio_bitmap_set(0x1C, ~B , read); // RECEIVE DIAGNOSTIC RESULTS
- sgio_bitmap_set(0x2B, D|T| W|R|O| K , read); // SEEK(10)
+ sgio_bitmap_set(0x2B, D| W|R|O| K , read); // SEEK(10)
sgio_bitmap_set(0x3C, ~B , read); // READ BUFFER
sgio_bitmap_set(0x4D, -1 , read); // LOG SENSE
sgio_bitmap_set(0x5A, -1 , read); // MODE SENSE(10)
@@ -175,7 +175,7 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
/* write */
- sgio_bitmap_set(0x04, D|T| R|O , write); // FORMAT UNIT
+ sgio_bitmap_set(0x04, D| R|O , write); // FORMAT UNIT
sgio_bitmap_set(0x0A, D|T| W| O , write); // WRITE(6)
sgio_bitmap_set(0x2A, D| W|R|O| B|K , write); // WRITE(10)
sgio_bitmap_set(0x2E, D| W|R|O| B|K , write); // WRITE AND VERIFY(10)
@@ -219,7 +219,7 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x23, R , read); // READ FORMAT CAPACITIES
sgio_bitmap_set(0x42, R , read); // READ SUB-CHANNEL
sgio_bitmap_set(0x43, R , read); // READ TOC/PMA/ATIP
- sgio_bitmap_set(0x44, T| R| V , read); // READ HEADER
+ sgio_bitmap_set(0x44, R , read); // READ HEADER
sgio_bitmap_set(0x45, R , read); // PLAY AUDIO(10)
sgio_bitmap_set(0x46, R , read); // GET CONFIGURATION
sgio_bitmap_set(0x47, R , read); // PLAY AUDIO MSF
@@ -257,7 +257,27 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
/* (mostly) tape */
+ sgio_bitmap_set(0x01, T , read); // REWIND
+ sgio_bitmap_set(0x05, T , read); // READ BLOCK LIMITS
+ sgio_bitmap_set(0x0F, T , read); // READ REVERSE(6)
+ sgio_bitmap_set(0x13, T , read); // VERIFY(6)
+ sgio_bitmap_set(0x1B, T| V , read); // LOAD UNLOAD
+ sgio_bitmap_set(0x2B, T , read); // LOCATE(10)
+ sgio_bitmap_set(0x34, T , read); // READ POSITION
+ sgio_bitmap_set(0x44, T| V , read); // REPORT DENSITY SUPPORT
+ sgio_bitmap_set(0x81, T , read); // READ REVERSE(16)
+ sgio_bitmap_set(0x92, T , read); // LOCATE(16)
+
+ sgio_bitmap_set(0x04, T , write); // FORMAT MEDIUM
+ sgio_bitmap_set(0x0B, T , write); // SET CAPACITY
+ sgio_bitmap_set(0x10, T , write); // WRITE FILEMARKS(6)
+ sgio_bitmap_set(0x11, T , write); // SPACE(6)
+ sgio_bitmap_set(0x14, T|L , write); // RECOVER BUFFERED DATA
sgio_bitmap_set(0x19, T , write); // ERASE(6)
+ sgio_bitmap_set(0x80, T , write); // WRITE FILEMARKS(16)
+ sgio_bitmap_set(0x82, T , write); // ALLOW OVERWRITE
+ sgio_bitmap_set(0x91, T , write); // SPACE(16)
+ sgio_bitmap_set(0x93, T , write); // ERASE(16)
/* communication devices (obsolete) */
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 09/13] sg_io: whitelist a few more commands for disks
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (7 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 08/13] sg_io: whitelist a few more commands for tapes Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 10/13] sg_io: whitelist a few obsolete commands Paolo Bonzini
` (3 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
This adds missing commands to the table from SBC and related standards.
Only commands that affect the medium are added. Commands that affect
other state of the LUN are all privileged, with the sole exception of START
STOP UNIT (which has always been allowed for all file descriptors. I do not
really agree with that and it's probably an artifact of when /dev/cdrom had
r--r--r-- permissions, but I'm not trying to change that.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 23 +++++++++++++++++++++--
1 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 49cd98a..74f3678 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -166,25 +166,44 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x08, D|T| W| O , read); // READ(6)
sgio_bitmap_set(0x25, D| W|R|O| B|K , read); // READ CAPACITY(10)
sgio_bitmap_set(0x28, D| W|R|O| B|K , read); // READ(10)
+ sgio_bitmap_set(0x29, D| W|R|O , read); // READ GENERATION
+ sgio_bitmap_set(0x2D, O , read); // READ UPDATED BLOCK
sgio_bitmap_set(0x2F, D| W|R|O , read); // VERIFY(10)
+ sgio_bitmap_set(0x34, D| W| O| K , read); // PRE-FETCH(10)
sgio_bitmap_set(0x37, D| O , read); // READ DEFECT DATA(10)
sgio_bitmap_set(0x3E, D| W| O , read); // READ LONG(10)
sgio_bitmap_set(0x88, D|T| W| O| B , read); // READ(16)
sgio_bitmap_set(0x8F, D|T| W| O| B , read); // VERIFY(16)
+ sgio_bitmap_set(0x90, D| W| O| B , read); // PRE-FETCH(16)
sgio_bitmap_set(0xA8, D| W|R|O , read); // READ(12)
+ sgio_bitmap_set(0xAF, D| W| O , read); // VERIFY(12)
+ sgio_bitmap_set(0xB7, D| O , read); // READ DEFECT DATA(12)
/* write */
sgio_bitmap_set(0x04, D| R|O , write); // FORMAT UNIT
+ sgio_bitmap_set(0x07, D| W| O , write); // REASSIGN BLOCKS
sgio_bitmap_set(0x0A, D|T| W| O , write); // WRITE(6)
sgio_bitmap_set(0x2A, D| W|R|O| B|K , write); // WRITE(10)
+ sgio_bitmap_set(0x2C, D| R|O , write); // ERASE(10)
sgio_bitmap_set(0x2E, D| W|R|O| B|K , write); // WRITE AND VERIFY(10)
sgio_bitmap_set(0x35, D| W|R|O| B|K , write); // SYNCHRONIZE CACHE(10)
+ sgio_bitmap_set(0x38, W| O| K , write); // MEDIUM SCAN
+ sgio_bitmap_set(0x3D, O , write); // UPDATE BLOCK
sgio_bitmap_set(0x3F, D| W| O , write); // WRITE LONG(10)
+ sgio_bitmap_set(0x41, D , write); // WRITE SAME(10)
sgio_bitmap_set(0x42, D , write); // UNMAP
sgio_bitmap_set(0x48, D| B , write); // SANITIZE
sgio_bitmap_set(0x51, D , write); // XPWRITE(10)
+ sgio_bitmap_set(0x53, D , write); // XDWRITEREAD(10)
+ sgio_bitmap_set(0x85, D| B , write); // ATA PASS-THROUGH(16)
+ sgio_bitmap_set(0x89, D , write); // COMPARE AND WRITE
+ sgio_bitmap_set(0x8B, D , write); // ORWRITE
sgio_bitmap_set(0x8A, D|T| W| O| B , write); // WRITE(16)
+ sgio_bitmap_set(0x8E, D| W| O| B , write); // WRITE AND VERIFY(16)
+ sgio_bitmap_set(0x91, D| W| O| B , write); // SYNCHRONIZE CACHE(16)
+ sgio_bitmap_set(0x93, D , write); // WRITE SAME(16)
+ sgio_bitmap_set(0xA1, D| B , write); // ATA PASS-THROUGH(12)
sgio_bitmap_set(0xAA, D| W|R|O , write); // WRITE(12)
sgio_bitmap_set(0xAC, O , write); // ERASE(12)
sgio_bitmap_set(0xAE, D| W| O , write); // WRITE AND VERIFY(12)
@@ -241,12 +260,12 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0xBD, R , read); // MECHANISM STATUS
sgio_bitmap_set(0xBE, R , read); // READ CD
- sgio_bitmap_set(0x53, D| R , write); // RESERVE TRACK / XDWRITEREAD(10)
+ sgio_bitmap_set(0x53, R , write); // RESERVE TRACK
sgio_bitmap_set(0x54, R , write); // SEND OPC INFORMATION
sgio_bitmap_set(0x58, R , write); // REPAIR TRACK
sgio_bitmap_set(0x5B, R , write); // CLOSE TRACK/SESSION
sgio_bitmap_set(0x5D, R , write); // SEND CUE SHEET
- sgio_bitmap_set(0xA1, D| R| B , write); // BLANK / ATA PASS-THROUGH(12)
+ sgio_bitmap_set(0xA1, R , write); // BLANK
sgio_bitmap_set(0xA2, R , write); // SEND EVENT
sgio_bitmap_set(0xA3, R , write); // SEND KEY
sgio_bitmap_set(0xA6, R , write); // LOAD/UNLOAD C/DVD
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 10/13] sg_io: whitelist a few obsolete commands
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (8 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 09/13] sg_io: whitelist a few more commands for disks Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 11/13] sg_io: add list of commands that were in the consulted list but are disabled Paolo Bonzini
` (2 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
These are added to their own section of the table, together with SEEK(10)
which has always been permitted.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 22 +++++++++++++++++++++-
1 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 74f3678..fea0c5d 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -143,7 +143,6 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x1A, -1 , read); // MODE SENSE(6)
sgio_bitmap_set(0x1B, D| W|R|O| A| B|K| F , read); // START STOP UNIT
sgio_bitmap_set(0x1C, ~B , read); // RECEIVE DIAGNOSTIC RESULTS
- sgio_bitmap_set(0x2B, D| W|R|O| K , read); // SEEK(10)
sgio_bitmap_set(0x3C, ~B , read); // READ BUFFER
sgio_bitmap_set(0x4D, -1 , read); // LOG SENSE
sgio_bitmap_set(0x5A, -1 , read); // MODE SENSE(10)
@@ -298,6 +297,27 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x91, T , write); // SPACE(16)
sgio_bitmap_set(0x93, T , write); // ERASE(16)
+ /* various obsolete */
+
+ sgio_bitmap_set(0x0B, D| W|R|O , read); // SEEK(6)
+ sgio_bitmap_set(0x2B, D| W|R|O| K , read); // SEEK(10)
+ sgio_bitmap_set(0x30, D| W|R|O , read); // SEARCH DATA HIGH(10)
+ sgio_bitmap_set(0x31, D| W|R|O , read); // SEARCH DATA EQUAL(10)
+ sgio_bitmap_set(0x32, D| W|R|O , read); // SEARCH DATA LOW(10)
+ sgio_bitmap_set(0x39, D|T|L|P|W|R|O| K , read); // COMPARE
+ sgio_bitmap_set(0x52, D , read); // XDREAD(10)
+ sgio_bitmap_set(0xB0, W|R|O , read); // SEARCH DATA HIGH(12)
+ sgio_bitmap_set(0xB1, W|R|O , read); // SEARCH DATA EQUAL(12)
+ sgio_bitmap_set(0xB2, W|R|O , read); // SEARCH DATA LOW(12)
+ sgio_bitmap_set(0xB4, D|T| W|R|O , read); // READ ELEMENT STATUS ATTACHED
+ sgio_bitmap_set(0xB8, T| W|R|O|M , read); // READ ELEMENT STATUS
+
+ sgio_bitmap_set(0x01, D| W|R|O|M , write); // REZERO UNIT
+ sgio_bitmap_set(0x18, D|T|L|P|W|R|O| K , write); // COPY
+ sgio_bitmap_set(0x3A, D|T|L|P|W|R|O| K , write); // COPY AND VERIFY
+ sgio_bitmap_set(0x50, D , write); // XDWRITE(10)
+ sgio_bitmap_set(0x80, D , write); // XDWRITE EXTENDED(16)
+
/* communication devices (obsolete) */
sgio_bitmap_set(0x08, C , write); // GET MESSAGE(6)
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 11/13] sg_io: add list of commands that were in the consulted list but are disabled
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (9 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 10/13] sg_io: whitelist a few obsolete commands Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 12/13] sg_io: remove remnants of sysfs SG_IO filters Paolo Bonzini
2013-01-24 15:00 ` [PATCH 13/13] sg_io: introduce unpriv_sgio queue flag Paolo Bonzini
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
To aid future modifications of the list, add a list of commands
that were in the version of the SCSI commands list I consulted,
but I considered too dangerous to enable by default for unprivileged
users.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/scsi_ioctl.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 102 insertions(+), 0 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index fea0c5d..27b844c 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -336,6 +336,108 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
sgio_bitmap_set(0x31, S, write); // OBJECT POSITION
sgio_bitmap_set(0x34, S, write); // GET DATA BUFFER STATUS
+#if 0
+ /*
+ * Starting from here are commands that are always privileged.
+ * I'm listing them anyway, as a reference to the version of
+ * the command list that I used.
+ */
+
+ /* control, privileged, universal except possibly RBC */
+
+ sgio_bitmap_set(0x1D, ~B , write); // SEND DIAGNOSTIC
+ sgio_bitmap_set(0x3B, -1 , write); // WRITE BUFFER
+
+ /* control, privileged */
+
+ sgio_bitmap_set(0x5E, D|T|L|P|W| O|M|A|E| F , write); // PERSISTENT RESERVE IN
+ sgio_bitmap_set(0x5F, D|T|L|P|W| O|M|A|E| F , write); // PERSISTENT RESERVE OUT
+ sgio_bitmap_set(0x83, D|T|L|P|W| O| K|V , write); // Third-party Copy OUT
+ sgio_bitmap_set(0x84, D|T|L|P|W| O| K|V , write); // Third-party Copy IN
+ sgio_bitmap_set(0x86, D|T| P|W| O|M|A|E|B|K|V , write); // ACCESS CONTROL IN
+ sgio_bitmap_set(0x87, D|T| P|W| O|M|A|E|B|K|V , write); // ACCESS CONTROL OUT
+ sgio_bitmap_set(0x8C, D|T| W| O|M| B| V , write); // READ ATTRIBUTE
+ sgio_bitmap_set(0x8D, D|T| W| O|M| B| V , write); // WRITE ATTRIBUTE
+ sgio_bitmap_set(0xA2, D|T| R| V , write); // SECURITY PROTOCOL IN
+ sgio_bitmap_set(0xA4, D|T|L| W| O|M|A|E|B|K|V , write); // MAINTENANCE OUT
+ sgio_bitmap_set(0xA9, V , write); // SERVICE ACTION OUT(12)
+ sgio_bitmap_set(0xB5, D|T| R| V , write); // SECURITY PROTOCOL OUT
+ sgio_bitmap_set(0xBA, D| W| O|M|A|E , write); // REDUNDANCY GROUP (IN)
+ sgio_bitmap_set(0xBB, D| W| O|M|A|E , write); // REDUNDANCY GROUP (OUT)
+ sgio_bitmap_set(0xBC, D| W| O|M|A|E , write); // SPARE (IN)
+ sgio_bitmap_set(0xBD, D| W| O|M|A|E , write); // SPARE (OUT)
+ sgio_bitmap_set(0xBE, D| W| O|M|A|E , write); // VOLUME SET (IN)
+ sgio_bitmap_set(0xBF, D| W| O|M|A|E , write); // VOLUME SET (OUT)
+
+ /* control, privileged, obsolete */
+
+ sgio_bitmap_set(0x16, D|T|L|P|W| O|M|A|E| K , write); // RESERVE(6)
+ sgio_bitmap_set(0x16, M , write); // RESERVE ELEMENT(6)
+ sgio_bitmap_set(0x17, D|T|L|P|W| O|M|A|E| K , write); // RELEASE(6)
+ sgio_bitmap_set(0x17, M , write); // RELEASE ELEMENT(6)
+ sgio_bitmap_set(0x33, D| W|R|O , write); // SET LIMITS(10)
+ sgio_bitmap_set(0x36, D| W| O| K , write); // LOCK UNLOCK CACHE(10)
+ sgio_bitmap_set(0x40, D|T|L|P|W|R|O|M , write); // CHANGE DEFINITION
+ sgio_bitmap_set(0x56, D|T|L|P|W| O|M|A|E , write); // RESERVE(10)
+ sgio_bitmap_set(0x56, M , write); // RESERVE ELEMENT(10)
+ sgio_bitmap_set(0x57, D|T|L|P|W| O|M|A|E , write); // RELEASE(10)
+ sgio_bitmap_set(0x57, M , write); // RELEASE ELEMENT(10)
+ sgio_bitmap_set(0x81, D , write); // REBUILD(16)
+ sgio_bitmap_set(0x82, D , write); // REGENERATE(16)
+ sgio_bitmap_set(0x92, D| W| O , write); // LOCK UNLOCK CACHE(16)
+ sgio_bitmap_set(0xA5, T| W| O|M , write); // MOVE MEDIUM
+ sgio_bitmap_set(0xA7, D|T| W| O , write); // MOVE MEDIUM ATTACHED
+ sgio_bitmap_set(0xB3, D| W|R|O , write); // SET LIMITS(12)
+
+ /* others: multiplexed */
+
+ sgio_bitmap_set(0x7E, D|T| R| M|A|E|B| V , write); // extended CDB
+ sgio_bitmap_set(0x7F, D| F , write); // variable length CDB
+ sgio_bitmap_set(0x9F, V , write); // SERVICE ACTION OUT(16)
+
+ /* others: vendor specific */
+
+ sgio_bitmap_set(0x01, L , write);
+ sgio_bitmap_set(0x02, D|T|L|P|W|R| M , write);
+ sgio_bitmap_set(0x05, D| L|P|W|R| M , write);
+ sgio_bitmap_set(0x06, D|T|L|P|W|R| M , write);
+ sgio_bitmap_set(0x07, T|L , write);
+ sgio_bitmap_set(0x08, L| M , write);
+ sgio_bitmap_set(0x09, D|T|L|P|W|R| M , write);
+ sgio_bitmap_set(0x0A, M , write);
+ sgio_bitmap_set(0x0B, M , write);
+ sgio_bitmap_set(0x0C, D|T|L|P|W|R| M , write);
+ sgio_bitmap_set(0x0D, D|T|L|P|W|R| M , write);
+ sgio_bitmap_set(0x0E, D|T|L|P|W|R| M , write);
+ sgio_bitmap_set(0x0F, D| L|P|W|R| M , write);
+ sgio_bitmap_set(0x10, D| P|W|R , write);
+ sgio_bitmap_set(0x11, D| L|P|W|R , write);
+ sgio_bitmap_set(0x13, D| L|P|W|R , write);
+ sgio_bitmap_set(0x14, D| P|W|R , write);
+ sgio_bitmap_set(0x19, D| L|P|W|R , write);
+ sgio_bitmap_set(0x20, D| W|R|O| K , write);
+ sgio_bitmap_set(0x21, D| W|R|O| K , write);
+ sgio_bitmap_set(0x22, D| W|R|O| K , write);
+ sgio_bitmap_set(0x23, D| W| O| K , write);
+ sgio_bitmap_set(0x24, D| W|R , write);
+ sgio_bitmap_set(0x26, D| W|R , write);
+ sgio_bitmap_set(0x27, D| W|R , write);
+ sgio_bitmap_set(0x2D, D , write);
+
+ /* others: reserved */
+
+ sgio_bitmap_set(0x1F, 0 , write);
+ sgio_bitmap_set(0x49, 0 , write);
+ sgio_bitmap_set(0x4F, 0 , write);
+ sgio_bitmap_set(0x59, 0 , write);
+ sgio_bitmap_set(0x98, 0 , write);
+ sgio_bitmap_set(0x99, 0 , write);
+ sgio_bitmap_set(0x9A, 0 , write);
+ sgio_bitmap_set(0x9B, 0 , write);
+ sgio_bitmap_set(0x9C, 0 , write);
+ sgio_bitmap_set(0x9D, 0 , write); // SERVICE ACTION BIDIRECTIONAL
+#endif
+
#undef D
#undef T
#undef L
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 12/13] sg_io: remove remnants of sysfs SG_IO filters
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (10 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 11/13] sg_io: add list of commands that were in the consulted list but are disabled Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 13/13] sg_io: introduce unpriv_sgio queue flag Paolo Bonzini
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Some defines and structs remained when support was removed for SG_IO
filters in sysfs. Remove them.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/linux/genhd.h | 9 ---------
1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 79b8bba..d3bc249 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -144,15 +144,6 @@ enum {
DISK_EVENT_EJECT_REQUEST = 1 << 1, /* eject requested */
};
-#define BLK_SCSI_MAX_CMDS (256)
-#define BLK_SCSI_CMD_PER_LONG (BLK_SCSI_MAX_CMDS / (sizeof(long) * 8))
-
-struct blk_scsi_cmd_filter {
- unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
- unsigned long write_ok[BLK_SCSI_CMD_PER_LONG];
- struct kobject kobj;
-};
-
struct disk_part_tbl {
struct rcu_head rcu_head;
int len;
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 13/13] sg_io: introduce unpriv_sgio queue flag
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
` (11 preceding siblings ...)
2013-01-24 15:00 ` [PATCH 12/13] sg_io: remove remnants of sysfs SG_IO filters Paolo Bonzini
@ 2013-01-24 15:00 ` Paolo Bonzini
12 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-24 15:00 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
This queue flag will let unprivileged users send any SG_IO command to
the device, without any filtering.
This is useful for virtualization, where some trusted guests would like
to send commands such as persistent reservations, but still the virtual
machine monitor should run with restricted permissions.
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Documentation/block/queue-sysfs.txt | 8 ++++++++
block/blk-sysfs.c | 33 +++++++++++++++++++++++++++++++++
block/scsi_ioctl.c | 4 +++-
include/linux/blkdev.h | 3 +++
4 files changed, 47 insertions(+), 1 deletions(-)
diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
index e54ac1d..341e781 100644
--- a/Documentation/block/queue-sysfs.txt
+++ b/Documentation/block/queue-sysfs.txt
@@ -133,6 +133,14 @@ 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.
+unpriv_sgio (RW)
+----------------
+When a process runs without CAP_SYS_RAWIO, access to some SCSI commands
+with the SG_IO ioctl is restricted. If this option is '0', the whitelist
+is applied for all file descriptors belonging to unprivileged processes.
+If this option is '1', the whitelist is only applied for file descriptors
+that are opened read-only; other file descriptors can send all SCSI commands,
+and no restrictions are applied by the kernel.
Jens Axboe <jens.axboe@oracle.com>, February 2009
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 7881477..ab2947d 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -215,6 +215,32 @@ static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page)
return queue_var_show(max_hw_sectors_kb, (page));
}
+static ssize_t
+queue_show_unpriv_sgio(struct request_queue *q, char *page)
+{
+ int bit;
+ bit = test_bit(QUEUE_FLAG_UNPRIV_SGIO, &q->queue_flags);
+ return queue_var_show(bit, page);
+}
+static ssize_t
+queue_store_unpriv_sgio(struct request_queue *q, const char *page, size_t count)
+{
+ unsigned long val;
+ ssize_t ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ ret = queue_var_store(&val, page, count);
+ spin_lock_irq(q->queue_lock);
+ if (val)
+ queue_flag_set(QUEUE_FLAG_UNPRIV_SGIO, q);
+ else
+ queue_flag_clear(QUEUE_FLAG_UNPRIV_SGIO, q);
+ spin_unlock_irq(q->queue_lock);
+ return ret;
+}
+
#define QUEUE_SYSFS_BIT_FNS(name, flag, neg) \
static ssize_t \
queue_show_##name(struct request_queue *q, char *page) \
@@ -403,6 +429,12 @@ static struct queue_sysfs_entry queue_nonrot_entry = {
.store = queue_store_nonrot,
};
+static struct queue_sysfs_entry queue_unpriv_sgio_entry = {
+ .attr = {.name = "unpriv_sgio", .mode = S_IRUGO | S_IWUSR },
+ .show = queue_show_unpriv_sgio,
+ .store = queue_store_unpriv_sgio,
+};
+
static struct queue_sysfs_entry queue_nomerges_entry = {
.attr = {.name = "nomerges", .mode = S_IRUGO | S_IWUSR },
.show = queue_nomerges_show,
@@ -445,6 +477,7 @@ static struct attribute *default_attrs[] = {
&queue_discard_max_entry.attr,
&queue_discard_zeroes_data_entry.attr,
&queue_write_same_max_entry.attr,
+ &queue_unpriv_sgio_entry.attr,
&queue_nonrot_entry.attr,
&queue_nomerges_entry.attr,
&queue_rq_affinity_entry.attr,
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 27b844c..e0d7792 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -471,7 +471,9 @@ int blk_verify_command(struct request_queue *q,
return 0;
/* Write-safe commands require a writable open */
- if (has_write_perm && filter->write_ok[cmd[0]] & (1 << q->sgio_type))
+ if (has_write_perm &&
+ (blk_queue_unpriv_sgio(q) ||
+ (filter->write_ok[cmd[0]] & (1 << q->sgio_type))))
return 0;
return -EPERM;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b376d37..71312ec 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -459,6 +459,7 @@ struct request_queue {
#define QUEUE_FLAG_SECDISCARD 17 /* supports SECDISCARD */
#define QUEUE_FLAG_SAME_FORCE 18 /* force complete on same CPU */
#define QUEUE_FLAG_DEAD 19 /* queue tear-down finished */
+#define QUEUE_FLAG_UNPRIV_SGIO 20 /* SG_IO free for unprivileged users */
#define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
(1 << QUEUE_FLAG_STACKABLE) | \
@@ -534,6 +535,8 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
#define blk_queue_nomerges(q) test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
#define blk_queue_noxmerges(q) \
test_bit(QUEUE_FLAG_NOXMERGES, &(q)->queue_flags)
+#define blk_queue_unpriv_sgio(q) \
+ test_bit(QUEUE_FLAG_UNPRIV_SGIO, &(q)->queue_flags)
#define blk_queue_nonrot(q) test_bit(QUEUE_FLAG_NONROT, &(q)->queue_flags)
#define blk_queue_io_stat(q) test_bit(QUEUE_FLAG_IO_STAT, &(q)->queue_flags)
#define blk_queue_add_random(q) test_bit(QUEUE_FLAG_ADD_RANDOM, &(q)->queue_flags)
--
1.7.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 01/13] sg_io: pass request_queue to blk_verify_command
2013-01-24 15:00 ` [PATCH 01/13] sg_io: pass request_queue to blk_verify_command Paolo Bonzini
@ 2013-01-24 22:34 ` Tejun Heo
0 siblings, 0 replies; 35+ messages in thread
From: Tejun Heo @ 2013-01-24 22:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, FUJITA Tomonori, Doug Gilbert,
James E.J. Bottomley, linux-scsi, Jens Axboe
On Thu, Jan 24, 2013 at 04:00:37PM +0100, Paolo Bonzini wrote:
> Adjust the blk_verify_command function to let it look at per-queue
> data. This will be done in the next patch.
>
> Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Doug Gilbert <dgilbert@interlog.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-scsi@kernel.org
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-24 15:00 ` [PATCH 02/13] sg_io: reorganize list of allowed commands Paolo Bonzini
@ 2013-01-24 22:42 ` Tejun Heo
2013-01-24 22:49 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-24 22:42 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
On Thu, Jan 24, 2013 at 04:00:38PM +0100, Paolo Bonzini wrote:
> +#define sgio_bitmap_set(cmd, mask, rw) \
> + if ((mask) != 0) __set_bit((cmd), filter->rw##_ok)
> +
> +#define D (1u << TYPE_DISK) /* Direct Access Block Device (SBC-3) */
> +#define T (1u << TYPE_TAPE) /* Sequential Access Device (SSC-3) */
> +#define L (1u << TYPE_PRINTER) /* Printer Device (SSC) */
> +#define P (1u << TYPE_PROCESSOR) /* Processor Device (SPC-2) */
> +#define W (1u << TYPE_WORM) /* Write Once Block Device (SBC) */
> +#define R (1u << TYPE_ROM) /* C/DVD Device (MMC-6) */
> +#define S (1u << TYPE_SCANNER) /* Scanner device (obsolete) */
> +#define O (1u << TYPE_MOD) /* Optical Memory Block Device (SBC) */
> +#define M (1u << TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */
> +#define C (1u << TYPE_COMM) /* Communication devices (obsolete) */
> +#define A (1u << TYPE_RAID) /* Storage Array Device (SCC-2) */
> +#define E (1u << TYPE_ENCLOSURE) /* SCSI Enclosure Services device (SES-2) */
> +#define B (1u << TYPE_RBC) /* Simplified Direct-Access (Reduced Block) device (RBC) */
> +#define K (1u << 0x0f) /* Optical Card Reader/Writer device (OCRW) */
> +#define V (1u << 0x10) /* Automation/Device Interface device (ADC-2) */
Can we please add TYPE_* constants for these two too?
> +#define F (1u << TYPE_OSD) /* Object-based Storage Device (OSD-2) */
> +
> + /* control, universal except possibly RBC, read */
> +
> + sgio_bitmap_set(0x00, -1 , read); // TEST UNIT READY
Hmmm... why are we not using symbolic constants for commands? If it's
because of horizontal real estate, we can abbreviate
sgio_bitmap_set(), no? Also, wouldn't it be better to have ALL
instead of -1? Also, the custom formatting is nice but can we at
least not use //?
One other thing is I would much prefer if the table was made static
const first. As we only allow compile-time defined tables, there's no
point in dynamically initializing these and the above can be static
initializers.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-24 22:42 ` Tejun Heo
@ 2013-01-24 22:49 ` Tejun Heo
2013-01-24 22:58 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-24 22:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
On Thu, Jan 24, 2013 at 02:42:03PM -0800, Tejun Heo wrote:
> One other thing is I would much prefer if the table was made static
> const first. As we only allow compile-time defined tables, there's no
> point in dynamically initializing these and the above can be static
> initializers.
On the similar line of thoughts, wouldn't it be better to have the
table organized by the device type first? It would be much easier to
comprehend which commands are allowed for each device type that way
and FWIW it would be more cacheline friendly. e.g. something like,
#define M(opcode) (1 << opcode)
#define COMMON \
M(READ_6) | M(WRITE_6) | ....
static const whatever_type blk_cmd_filter_disk = {
COMMON |
M(CMD_SPECIFIC_TO_THIS_TYPE0) |
M(CMD_SPECIFIC_TO_THIS_TYPE2) |
...
};
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-24 15:00 ` [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices Paolo Bonzini
@ 2013-01-24 22:55 ` Tejun Heo
2013-01-25 9:26 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-24 22:55 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
On Thu, Jan 24, 2013 at 04:00:42PM +0100, Paolo Bonzini wrote:
> Strangely, a couple of MMC commands were never included. Add them too.
What are the justifications for adding these commands to the filter?
Are there users requesting these?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-24 22:49 ` Tejun Heo
@ 2013-01-24 22:58 ` Tejun Heo
2013-01-25 10:01 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-24 22:58 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
On Thu, Jan 24, 2013 at 02:49:21PM -0800, Tejun Heo wrote:
> On Thu, Jan 24, 2013 at 02:42:03PM -0800, Tejun Heo wrote:
> > One other thing is I would much prefer if the table was made static
> > const first. As we only allow compile-time defined tables, there's no
> > point in dynamically initializing these and the above can be static
> > initializers.
>
> On the similar line of thoughts, wouldn't it be better to have the
> table organized by the device type first? It would be much easier to
> comprehend which commands are allowed for each device type that way
> and FWIW it would be more cacheline friendly. e.g. something like,
>
> #define M(opcode) (1 << opcode)
>
> #define COMMON \
> M(READ_6) | M(WRITE_6) | ....
>
> static const whatever_type blk_cmd_filter_disk = {
> COMMON |
> M(CMD_SPECIFIC_TO_THIS_TYPE0) |
> M(CMD_SPECIFIC_TO_THIS_TYPE2) |
> ...
> };
Oops, there are way more bits than in the longest integer, so you
can't statically initialize them in pretty way (maybe it's possible
but I can't think of anything pretty). We can still initialize the
table once during boot and throw away the init code, I guess. Also, I
still think device -> command organization would be better than
commmand -> device.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-24 22:55 ` Tejun Heo
@ 2013-01-25 9:26 ` Paolo Bonzini
2013-01-25 17:04 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 9:26 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 24/01/2013 23:55, Tejun Heo ha scritto:
> On Thu, Jan 24, 2013 at 04:00:42PM +0100, Paolo Bonzini wrote:
>> Strangely, a couple of MMC commands were never included. Add them too.
>
> What are the justifications for adding these commands to the filter?
> Are there users requesting these?
I think it's the other way round. What's the justification for leaving
them out, if they are in the standard? Since we're touching the
commands for other standards, it's better to be complete for MMC as well.
At least one of them (MECHANISM STATUS) is implemented in both QEMU and
Bochs, so someone is using it. If that someone were run virtualized,
with the host /dev/sr0 passed directly to the guest rather than
emulated, it would break.
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-24 22:58 ` Tejun Heo
@ 2013-01-25 10:01 ` Paolo Bonzini
2013-01-25 17:13 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 10:01 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 24/01/2013 23:58, Tejun Heo ha scritto:
>
>> +#define sgio_bitmap_set(cmd, mask, rw) \
>> + if ((mask) != 0) __set_bit((cmd), filter->rw##_ok)
>> +
>> +#define D (1u << TYPE_DISK) /* Direct Access Block Device (SBC-3) */
>> +#define T (1u << TYPE_TAPE) /* Sequential Access Device (SSC-3) */
>> +#define L (1u << TYPE_PRINTER) /* Printer Device (SSC) */
>> +#define P (1u << TYPE_PROCESSOR) /* Processor Device (SPC-2) */
>> +#define W (1u << TYPE_WORM) /* Write Once Block Device (SBC) */
>> +#define R (1u << TYPE_ROM) /* C/DVD Device (MMC-6) */
>> +#define S (1u << TYPE_SCANNER) /* Scanner device (obsolete) */
>> +#define O (1u << TYPE_MOD) /* Optical Memory Block Device (SBC) */
>> +#define M (1u << TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */
>> +#define C (1u << TYPE_COMM) /* Communication devices (obsolete) */
>> +#define A (1u << TYPE_RAID) /* Storage Array Device (SCC-2) */
>> +#define E (1u << TYPE_ENCLOSURE) /* SCSI Enclosure Services device (SES-2) */
>> +#define B (1u << TYPE_RBC) /* Simplified Direct-Access (Reduced Block) device (RBC) */
>> +#define K (1u << 0x0f) /* Optical Card Reader/Writer device (OCRW) */
>> +#define V (1u << 0x10) /* Automation/Device Interface device (ADC-2) */
>
> Can we please add TYPE_* constants for these two too?
Ok.
>> +#define F (1u << TYPE_OSD) /* Object-based Storage Device (OSD-2) */
>> +
>> + /* control, universal except possibly RBC, read */
>> +
>> + sgio_bitmap_set(0x00, -1 , read); // TEST UNIT READY
>
> Hmmm... why are we not using symbolic constants for commands?
First, because the table is based on
http://www.t10.org/lists/op-num.txt. Entries in that file look like this:
OP DTLPWROMAEBKVF Description
-- -------------- ----------------------------------------------------
00 MMMMMMMMMMMMMM TEST UNIT READY
01 M REWIND
01 Z V ZZZZ REZERO UNIT
which explains a bit the formatting.
Second, because many symbolic constants do not exist in
include/scsi/scsi.h or include/linux/uapi/cdrom.h, and I don't think it
would make sense to add them for a one-off use, especially for obsolete
commands or device types.
> If it's because of horizontal real estate, we can abbreviate
> sgio_bitmap_set(), no?
No, it's not that. In fact using the symbolic constants would save a
few characters (for the 0xNN and the comment start). I really prefer to
keep the opcode visible so that you can easily match the code to op-num.txt.
> Also, wouldn't it be better to have ALL
> instead of -1? Also, the custom formatting is nice but can we at
> least not use //?
ALL instead of -1 is a good idea, or I can just spell it out if it's
okay for you.
// is nicer in my opinion (for this case where we're throwing away all
the rules anyway) because it avoids the misaligned */ but I can change
it of course.
>>> One other thing is I would much prefer if the table was made static
>>> const first. As we only allow compile-time defined tables, there's no
>>> point in dynamically initializing these and the above can be static
>>> initializers.
>>
>> On the similar line of thoughts, wouldn't it be better to have the
>> table organized by the device type first? It would be much easier to
>> comprehend which commands are allowed for each device type that way
>> and FWIW it would be more cacheline friendly. e.g. something like,
It is actually more cacheline friendly like this. The vast majority of
commands will be READ and WRITE, which are 0x?8 and 0x?A. So in
practice one cacheline will be shared by all device types, maybe two if
you use READ/WRITE(12) for some devices and READ/WRITE(16) for others.
>> #define M(opcode) (1 << opcode)
>>
>> #define COMMON \
>> M(READ_6) | M(WRITE_6) | ....
>>
>> static const whatever_type blk_cmd_filter_disk = {
>> COMMON |
>> M(CMD_SPECIFIC_TO_THIS_TYPE0) |
>> M(CMD_SPECIFIC_TO_THIS_TYPE2) |
>> ...
>> };
>
> Oops, there are way more bits than in the longest integer, so you
> can't statically initialize them in pretty way (maybe it's possible
> but I can't think of anything pretty). We can still initialize the
> table once during boot and throw away the init code, I guess.
Yeah, that's what happens because GCC will inline
blk_set_cmd_filter_defaults into its sole caller which is __init.
There's no difference from before this patch, but I can add an explicit
__init to blk_set_cmd_filter_defaults too.
> Also, I
> still think device -> command organization would be better than
> commmand -> device.
This patch ties to keep the list as similar as possible to the old one,
to ease review. There is usually a 1:1 matching between "old" and "new"
lines, and this limits the freedom I have in organizing the list.
In the next patches I gradually move towards a more structured
organization, usually something like standard->command (one standard may
correspond to more than one device type, but all of them are described
in the same document).
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 9:26 ` Paolo Bonzini
@ 2013-01-25 17:04 ` Tejun Heo
2013-01-25 17:16 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 17:04 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Hello, Paolo.
On Fri, Jan 25, 2013 at 10:26:09AM +0100, Paolo Bonzini wrote:
> > What are the justifications for adding these commands to the filter?
> > Are there users requesting these?
>
> I think it's the other way round. What's the justification for leaving
> them out, if they are in the standard? Since we're touching the
> commands for other standards, it's better to be complete for MMC as well.
Maybe my experience with ATA left me bitter with the standards, but
opening gate to everything described in standard sounds like a pretty
bad idea to me. If there are users and devices which make use of them
in sane way, sure. If not, what's the point of risking it?
> At least one of them (MECHANISM STATUS) is implemented in both QEMU and
> Bochs, so someone is using it. If that someone were run virtualized,
> with the host /dev/sr0 passed directly to the guest rather than
> emulated, it would break.
Sure, then, enable MECHANISM_STATUS.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-25 10:01 ` Paolo Bonzini
@ 2013-01-25 17:13 ` Tejun Heo
2013-01-25 17:26 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 17:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Hello, Paolo.
On Fri, Jan 25, 2013 at 11:01:38AM +0100, Paolo Bonzini wrote:
> First, because the table is based on
> http://www.t10.org/lists/op-num.txt. Entries in that file look like this:
>
> OP DTLPWROMAEBKVF Description
> -- -------------- ----------------------------------------------------
> 00 MMMMMMMMMMMMMM TEST UNIT READY
> 01 M REWIND
> 01 Z V ZZZZ REZERO UNIT
>
> which explains a bit the formatting.
Ah, okay, if it's something already established, please go ahead.
> Second, because many symbolic constants do not exist in
> include/scsi/scsi.h or include/linux/uapi/cdrom.h, and I don't think it
> would make sense to add them for a one-off use, especially for obsolete
> commands or device types.
It's kinda nice to be able to search for the constants for usages in
kernel. It's not complete but does help from time to time.
> > If it's because of horizontal real estate, we can abbreviate
> > sgio_bitmap_set(), no?
>
> No, it's not that. In fact using the symbolic constants would save a
> few characters (for the 0xNN and the comment start). I really prefer to
> keep the opcode visible so that you can easily match the code to op-num.txt.
How many constants need to be added? I guess this ties to the other
thread on why it's desirable to add all listed commands. If you're
just gonna add several, there's no point in not using the constants,
right? Most are already there. If you want opcodes visible, you can
make them the comments, right?
> > Also, wouldn't it be better to have ALL
> > instead of -1? Also, the custom formatting is nice but can we at
> > least not use //?
>
> ALL instead of -1 is a good idea, or I can just spell it out if it's
> okay for you.
Yeah, both sound fine to me.
> // is nicer in my opinion (for this case where we're throwing away all
> the rules anyway) because it avoids the misaligned */ but I can change
> it of course.
Let's please not do //.
> >> On the similar line of thoughts, wouldn't it be better to have the
> >> table organized by the device type first? It would be much easier to
> >> comprehend which commands are allowed for each device type that way
> >> and FWIW it would be more cacheline friendly. e.g. something like,
>
> It is actually more cacheline friendly like this. The vast majority of
> commands will be READ and WRITE, which are 0x?8 and 0x?A. So in
> practice one cacheline will be shared by all device types, maybe two if
> you use READ/WRITE(12) for some devices and READ/WRITE(16) for others.
The cacheline thing was me being confused about how the tables are
used. The tables are per-device after initialized so it should be
fine.
> >> #define M(opcode) (1 << opcode)
> >>
> >> #define COMMON \
> >> M(READ_6) | M(WRITE_6) | ....
> >>
> >> static const whatever_type blk_cmd_filter_disk = {
> >> COMMON |
> >> M(CMD_SPECIFIC_TO_THIS_TYPE0) |
> >> M(CMD_SPECIFIC_TO_THIS_TYPE2) |
> >> ...
> >> };
> >
> > Oops, there are way more bits than in the longest integer, so you
> > can't statically initialize them in pretty way (maybe it's possible
> > but I can't think of anything pretty). We can still initialize the
> > table once during boot and throw away the init code, I guess.
>
> Yeah, that's what happens because GCC will inline
> blk_set_cmd_filter_defaults into its sole caller which is __init.
> There's no difference from before this patch, but I can add an explicit
> __init to blk_set_cmd_filter_defaults too.
Maybe I'm misreading the code but we're still initializing table per
queue, right? Can't we just have per-type tables which are populated
during boot (or on-demand)?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 17:04 ` Tejun Heo
@ 2013-01-25 17:16 ` Paolo Bonzini
2013-01-25 17:28 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 17:16 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 25/01/2013 18:04, Tejun Heo ha scritto:
>> >
>> > I think it's the other way round. What's the justification for leaving
>> > them out, if they are in the standard? Since we're touching the
>> > commands for other standards, it's better to be complete for MMC as well.
> Maybe my experience with ATA left me bitter with the standards, but
> opening gate to everything described in standard sounds like a pretty
> bad idea to me. If there are users and devices which make use of them
> in sane way, sure. If not, what's the point of risking it?
Well, you can find broken devices for pretty much every command in the
list. Anyhow, the other two commands are obsolete so I'm okay with
leaving them out, if only for the sake of avoiding useless email threads.
Paolo
>> > At least one of them (MECHANISM STATUS) is implemented in both QEMU and
>> > Bochs, so someone is using it. If that someone were run virtualized,
>> > with the host /dev/sr0 passed directly to the guest rather than
>> > emulated, it would break.
> Sure, then, enable MECHANISM_STATUS.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
2013-01-25 17:13 ` Tejun Heo
@ 2013-01-25 17:26 ` Paolo Bonzini
0 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 17:26 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 25/01/2013 18:13, Tejun Heo ha scritto:
> Hello, Paolo.
>
> On Fri, Jan 25, 2013 at 11:01:38AM +0100, Paolo Bonzini wrote:
>> First, because the table is based on
>> http://www.t10.org/lists/op-num.txt. Entries in that file look like this:
>>
>> OP DTLPWROMAEBKVF Description
>> -- -------------- ----------------------------------------------------
>> 00 MMMMMMMMMMMMMM TEST UNIT READY
>> 01 M REWIND
>> 01 Z V ZZZZ REZERO UNIT
>>
>> which explains a bit the formatting.
>
> Ah, okay, if it's something already established, please go ahead.
>
>> Second, because many symbolic constants do not exist in
>> include/scsi/scsi.h or include/linux/uapi/cdrom.h, and I don't think it
>> would make sense to add them for a one-off use, especially for obsolete
>> commands or device types.
>
> It's kinda nice to be able to search for the constants for usages in
> kernel. It's not complete but does help from time to time.
Yeah, that's true. On the other hand, all the constants that are
missing are really just for userspace tools in general.
>>> If it's because of horizontal real estate, we can abbreviate
>>> sgio_bitmap_set(), no?
>>
>> No, it's not that. In fact using the symbolic constants would save a
>> few characters (for the 0xNN and the comment start). I really prefer to
>> keep the opcode visible so that you can easily match the code to op-num.txt.
>
> How many constants need to be added?
I'd guesstimate 40-50.
> If you're
> just gonna add several, there's no point in not using the constants,
> right? Most are already there. If you want opcodes visible, you can
> make them the comments, right?
Yes, like "/* 0x00 */ CONSTANT, MASK". I still have a slight preference
for the opcodes because if the constant ends up wrong, the
head-scratching would be higher than if the opcode is wrong (the opcode
is what you see in the dumps).
>>> Also, wouldn't it be better to have ALL
>>> instead of -1? Also, the custom formatting is nice but can we at
>>> least not use //?
>>
>> ALL instead of -1 is a good idea, or I can just spell it out if it's
>> okay for you.
>
> Yeah, both sound fine to me.
>
>> // is nicer in my opinion (for this case where we're throwing away all
>> the rules anyway) because it avoids the misaligned */ but I can change
>> it of course.
>
> Let's please not do //.
Ok, // comments replaced with C comments.
>>>> On the similar line of thoughts, wouldn't it be better to have the
>>>> table organized by the device type first? It would be much easier to
>>>> comprehend which commands are allowed for each device type that way
>>>> and FWIW it would be more cacheline friendly. e.g. something like,
>>
>> It is actually more cacheline friendly like this. The vast majority of
>> commands will be READ and WRITE, which are 0x?8 and 0x?A. So in
>> practice one cacheline will be shared by all device types, maybe two if
>> you use READ/WRITE(12) for some devices and READ/WRITE(16) for others.
>
> The cacheline thing was me being confused about how the tables are
> used. The tables are per-device after initialized so it should be
> fine.
No, they are not, but it is fine anyway. :) You'll really use very
little of this table (and of the old one as well) in the hot paths.
>>>> #define M(opcode) (1 << opcode)
>>>>
>>>> #define COMMON \
>>>> M(READ_6) | M(WRITE_6) | ....
>>>>
>>>> static const whatever_type blk_cmd_filter_disk = {
>>>> COMMON |
>>>> M(CMD_SPECIFIC_TO_THIS_TYPE0) |
>>>> M(CMD_SPECIFIC_TO_THIS_TYPE2) |
>>>> ...
>>>> };
>>>
>>> Oops, there are way more bits than in the longest integer, so you
>>> can't statically initialize them in pretty way (maybe it's possible
>>> but I can't think of anything pretty). We can still initialize the
>>> table once during boot and throw away the init code, I guess.
>>
>> Yeah, that's what happens because GCC will inline
>> blk_set_cmd_filter_defaults into its sole caller which is __init.
>> There's no difference from before this patch, but I can add an explicit
>> __init to blk_set_cmd_filter_defaults too.
>
> Maybe I'm misreading the code but we're still initializing table per
> queue, right? Can't we just have per-type tables which are populated
> during boot (or on-demand)?
No, the queue just stores the device type in an unsigned char. The
device type is then used to pick a bit in each word. I think you are
confusing with an earlier version you saw on Red Hat mailing lists, but
you NACKed it because you didn't like the lazy allocation.
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 17:16 ` Paolo Bonzini
@ 2013-01-25 17:28 ` Tejun Heo
2013-01-25 17:57 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 17:28 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
On Fri, Jan 25, 2013 at 06:16:04PM +0100, Paolo Bonzini wrote:
> Well, you can find broken devices for pretty much every command in the
> list. Anyhow, the other two commands are obsolete so I'm okay with
> leaving them out, if only for the sake of avoiding useless email threads.
Once we open the commands to userland this way, it's difficult to
throttle it back again. I just fail to see the point of allowing
everything possible. There's a way to override it (is that in yet?)
and we can always extend the list later, so please do the minimal set
with justification and can you please stop labeling reviews as
"useless"? For Pete's sake, if you disagree, just disagree and
explain your point. How is discussion about which commands should be
allowed via SG_IO "useless"? Gees...
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 17:28 ` Tejun Heo
@ 2013-01-25 17:57 ` Paolo Bonzini
2013-01-25 18:13 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 17:57 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 25/01/2013 18:28, Tejun Heo ha scritto:
> On Fri, Jan 25, 2013 at 06:16:04PM +0100, Paolo Bonzini wrote:
>> > Well, you can find broken devices for pretty much every command in the
>> > list. Anyhow, the other two commands are obsolete so I'm okay with
>> > leaving them out, if only for the sake of avoiding useless email threads.
> Once we open the commands to userland this way, it's difficult to
> throttle it back again.
I think the right place to throttle them back would be with a per-device
quirk, but I understand being conservative since we're talking about two
obsolete commands.
> I just fail to see the point of allowing
> everything possible. There's a way to override it (is that in yet?)
No, it's not. I made it patch 13/13 in this series.
> and we can always extend the list later, so please do the minimal set
> with justification
I cannot really give justification for single commands. What this is
going to be used for is virt, but I cannot know of all OSes and all
proprietary software in the wild. You have to some extent accept that
if it is in the standard, somebody had the need for it, usually a big
database vendor.
And because someone _will_ use it from my point of view, I can only give
justification to leave stuff _out_. In general I did so if the command
can disrupt someone else, as would be the case for persistent
reservations. It was not really always respected for the existing
table, for example I'd have left out LOG SELECT, but the series is
already controversial enough, so one thing at a time.
I put these three commands in because I wanted to include somewhere all
the commands that were in my list, and I had no reason to leave them
out. Two of them are obsolete, so if you prefer to keep them out I'll
move them, end of the story. :)
> and can you please stop labeling reviews as "useless"?
Reviews are not useless, but championing the inclusion of two obsolete
commands in a list is not a good use of bandwidth. Because the choice
is arbitrary, the discussion can degenerate too easily into repeatedly
saying the same thing over and over. All I'm trying to do is avoid it.
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 17:57 ` Paolo Bonzini
@ 2013-01-25 18:13 ` Tejun Heo
2013-01-25 18:47 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 18:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Hey, Paolo.
On Fri, Jan 25, 2013 at 06:57:20PM +0100, Paolo Bonzini wrote:
> And because someone _will_ use it from my point of view, I can only give
> justification to leave stuff _out_. In general I did so if the command
> can disrupt someone else, as would be the case for persistent
> reservations. It was not really always respected for the existing
> table, for example I'd have left out LOG SELECT, but the series is
> already controversial enough, so one thing at a time.
I'm not sure how well I can explain this but something being in the
spec and something in wide use are two different things, and people,
including the ones in hw vendors, tend not to pay too much attention /
resources to stuff which aren't used widely and commands which aren't
in wide use are much more likely to have errors in their
implementation or be abused - e.g. overridden for vendor specific
command as part of weird init sequence to select device mode or
whatnot.
Another aspect is that sometimes opcodes are recycled and people of
course don't try to recycle opcodes which are in wide use. They
choose the ones which failed to gain much traction and became
obsolete. The recycled command might not be suitable for exposing to
userland.
So, there are actual costs coming from exposing them, which is fine if
the benefit can offset them enough, but what would be the benefit of
exposing commands which aren't being used? The only thing I can think
of is that of avoiding the annoyance of expanding the list later?
But, as you said, exposing commands by default mean that we're more
likely to need blacklists in the future, so that doesn't seem like
that much of winning to me. Discovering use cases is actually much
easier than finding broken devices for some obscure commands.
> > and can you please stop labeling reviews as "useless"?
>
> Reviews are not useless, but championing the inclusion of two obsolete
> commands in a list is not a good use of bandwidth. Because the choice
> is arbitrary, the discussion can degenerate too easily into repeatedly
> saying the same thing over and over. All I'm trying to do is avoid it.
Seems like I was too twitchy. My apologies.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 18:13 ` Tejun Heo
@ 2013-01-25 18:47 ` Paolo Bonzini
2013-01-25 19:01 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 18:47 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 25/01/2013 19:13, Tejun Heo ha scritto:
> I'm not sure how well I can explain this but something being in the
> spec and something in wide use are two different things, and people,
> including the ones in hw vendors, tend not to pay too much attention /
> resources to stuff which aren't used widely and commands which aren't
> in wide use are much more likely to have errors in their
> implementation or be abused - e.g. overridden for vendor specific
> command as part of weird init sequence to select device mode or
> whatnot.
That doesn't happen. There's plenty of vendor-specific opcodes and mode
pages, not to mention out-of-band channels, e.g. work directly at the
USB level like the mode select stuff.
IIRC IOMEGA ZIP drives had some egregious abuse in READ and WRITE of the
boot sector, presenting fake partition tables or something like that.
But it wasn't using strange commands for that.
> Another aspect is that sometimes opcodes are recycled and people of
> course don't try to recycle opcodes which are in wide use. They
> choose the ones which failed to gain much traction and became
> obsolete. The recycled command might not be suitable for exposing to
> userland.
No, that doesn't happen either. There's plenty of reserved opcodes, and
this time you can count on the standards people which are more
reasonable than vendors.
> So, there are actual costs coming from exposing them, which is fine if
> the benefit can offset them enough, but what would be the benefit of
> exposing commands which aren't being used?
If it is not used, nobody will use it and nothing will break.
If it is used in a case we don't know, you'll have to add it later.
If it is little used and broken somewhere, you'll have to add it later
to the whitelist and implement the quirk. Both of them. Adding it now
saves a step.
The only case in which you save later work, is if someone bitches that X
doesn't work but nobody tells us. Patch economy by obscurity. :)
> The only thing I can think
> of is that of avoiding the annoyance of expanding the list later?
> But, as you said, exposing commands by default mean that we're more
> likely to need blacklists in the future
I disagree. It does not change anything.
If it is something really serious (i.e. it crashes the firmware), we'd
need a quirk anyway. If it is something only slightly less stupid,
userspace can deal with broken hardware itself. It is somewhat expected
with SG_IO anyway. For virt, the guest would treat it the same as on
bare metal; for anything else, the program would already have to deal
with it if run as root.
> , so that doesn't seem like
> that much of winning to me. Discovering use cases is actually much
> easier than finding broken devices for some obscure commands.
Not if many of your uses are proprietary (Windows, AIX on PPC, z/OS on
s390, Oracle, SAP, you name it).
>>> and can you please stop labeling reviews as "useless"?
>>
>> Reviews are not useless, but championing the inclusion of two obsolete
>> commands in a list is not a good use of bandwidth. Because the choice
>> is arbitrary, the discussion can degenerate too easily into repeatedly
>> saying the same thing over and over. All I'm trying to do is avoid it.
>
> Seems like I was too twitchy. My apologies.
No problem.
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 18:47 ` Paolo Bonzini
@ 2013-01-25 19:01 ` Tejun Heo
2013-01-25 22:32 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 19:01 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Hey,
On Fri, Jan 25, 2013 at 07:47:47PM +0100, Paolo Bonzini wrote:
> That doesn't happen. There's plenty of vendor-specific opcodes and mode
> pages, not to mention out-of-band channels, e.g. work directly at the
> USB level like the mode select stuff.
>
> IIRC IOMEGA ZIP drives had some egregious abuse in READ and WRITE of the
> boot sector, presenting fake partition tables or something like that.
> But it wasn't using strange commands for that.
Some did that at least in ATA. It was mapping some command to
firmware write. Given how many USB devices implement SCSI these days,
I wouldn't be too firm with "that doesn't happen". How can such
all-covering statement be asserted?
> > Another aspect is that sometimes opcodes are recycled and people of
> > course don't try to recycle opcodes which are in wide use. They
> > choose the ones which failed to gain much traction and became
> > obsolete. The recycled command might not be suitable for exposing to
> > userland.
>
> No, that doesn't happen either. There's plenty of reserved opcodes, and
> this time you can count on the standards people which are more
> reasonable than vendors.
ISTR recycle. Maybe it was ATA too and SCSI people are better. I
don't know.
> > So, there are actual costs coming from exposing them, which is fine if
> > the benefit can offset them enough, but what would be the benefit of
> > exposing commands which aren't being used?
>
> If it is not used, nobody will use it and nothing will break.
I have to say that's an overly optimistic view of the world. There
are even devices puking on receiving SCSI commands in a slightly
different sequence than windows sends them out and devices which lock
up if you send them enough TURs.
> > , so that doesn't seem like
> > that much of winning to me. Discovering use cases is actually much
> > easier than finding broken devices for some obscure commands.
>
> Not if many of your uses are proprietary (Windows, AIX on PPC, z/OS on
> s390, Oracle, SAP, you name it).
You would get -EPERM/ACCES whatnot from base OS and you would know the
command in use, no?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 19:01 ` Tejun Heo
@ 2013-01-25 22:32 ` Paolo Bonzini
2013-01-25 22:41 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 22:32 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 25/01/2013 20:01, Tejun Heo ha scritto:
> Some did that at least in ATA. It was mapping some command to
> firmware write. Given how many USB devices implement SCSI these days,
> I wouldn't be too firm with "that doesn't happen". How can such
> all-covering statement be asserted?
Of course it cannot be asserted. In the same way as it cannot be
asserted that "no program will try to use a command". It is just as
unsafe to keep a command in, as it is unsafe to keep a command out.
All you can do is use common sense, which says that if a command has
been in a standard, someone has likely used it. Of course someone
_might_ have misused it too, but how likely is that?
To try and give an answer, I grepped the "unusual" drivers in
drivers/usb/storage. All of them seem to use non-standard packets (i.e.
basically are not SCSI) for their strange stuff, except two:
initializers.c has one function that sends SCSI command 0xEC,
realtek_cr.c uses 0xF0, both vendor specific. So USB firmware writers,
despite being known for tweaking SCSI standards in many ways, seem to be
sensible in this respect.
What about the likelihood that someone _used_ particular commands? For
example, I know that REZERO UNIT (which has been obsolete for several
releases of the standard) is used in the wild because a patch to support
it was submitted to QEMU not long ago. Some commands are indeed no-ops
or almost no-ops, or just too obscure to care (such as the two MMC
commands), so I wouldn't spend too much time to discuss them, but it
should be unnecessary if we stick to a principle that is logical and
generally applicable. "Someone may have misused it" is not one.
>>> So, there are actual costs coming from exposing them, which is fine if
>>> the benefit can offset them enough, but what would be the benefit of
>>> exposing commands which aren't being used?
>>
>> If it is not used, nobody will use it and nothing will break.
>
> I have to say that's an overly optimistic view of the world. There
> are even devices puking on receiving SCSI commands in a slightly
> different sequence than windows sends them out and devices which lock
> up if you send them enough TURs.
If it is not used, nobody will use it and setting the bit is effectively
useless (but harmless too).
>>> , so that doesn't seem like
>>> that much of winning to me. Discovering use cases is actually much
>>> easier than finding broken devices for some obscure commands.
>>
>> Not if many of your uses are proprietary (Windows, AIX on PPC, z/OS on
>> s390, Oracle, SAP, you name it).
>
> You would get -EPERM/ACCES whatnot from base OS and you would know the
> command in use, no?
Yes, but I would like to avoid many iterations. I don't have access
myself to all that software except Windows.
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 22:32 ` Paolo Bonzini
@ 2013-01-25 22:41 ` Tejun Heo
2013-01-25 23:32 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 22:41 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Hello, Paolo.
On Fri, Jan 25, 2013 at 11:32:42PM +0100, Paolo Bonzini wrote:
> All you can do is use common sense, which says that if a command has
> been in a standard, someone has likely used it. Of course someone
> _might_ have misused it too, but how likely is that?
So, that's where we differ, I guess. When it comes to hardware
devices, especially the ones as widely and cheaply deployed as SCSI or
ATA, my common sense screams to stick to what's known to work and
don't venture outside unnecessarily.
> To try and give an answer, I grepped the "unusual" drivers in
> drivers/usb/storage. All of them seem to use non-standard packets (i.e.
> basically are not SCSI) for their strange stuff, except two:
> initializers.c has one function that sends SCSI command 0xEC,
> realtek_cr.c uses 0xF0, both vendor specific. So USB firmware writers,
> despite being known for tweaking SCSI standards in many ways, seem to be
> sensible in this respect.
You're grepping for known irregularities which need to be used to make
the device work. Just read through, say, sd driver code and see how
hard it tries to stick to what's known to work rather than venturing
out to the space allowed by the spec.
The default approach in this area definitely is and should be
conservative in general.
> > I have to say that's an overly optimistic view of the world. There
> > are even devices puking on receiving SCSI commands in a slightly
> > different sequence than windows sends them out and devices which lock
> > up if you send them enough TURs.
>
> If it is not used, nobody will use it and setting the bit is effectively
> useless (but harmless too).
No, we don't know it's gonna be harmless. How do we *know* that
outside of the naive expectation that devices are gonna safely and
sanely ignore things that they don't understand?
> > You would get -EPERM/ACCES whatnot from base OS and you would know the
> > command in use, no?
>
> Yes, but I would like to avoid many iterations. I don't have access
> myself to all that software except Windows.
I don't think it's gonna be that many iterations and would much prefer
doing several iterations and having the backing data for each command
added.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 22:41 ` Tejun Heo
@ 2013-01-25 23:32 ` Paolo Bonzini
2013-01-25 23:47 ` Tejun Heo
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-25 23:32 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 25/01/2013 23:41, Tejun Heo ha scritto:
> Hello, Paolo.
>
> On Fri, Jan 25, 2013 at 11:32:42PM +0100, Paolo Bonzini wrote:
>> All you can do is use common sense, which says that if a command has
>> been in a standard, someone has likely used it. Of course someone
>> _might_ have misused it too, but how likely is that?
>
> So, that's where we differ, I guess. When it comes to hardware
> devices, especially the ones as widely and cheaply deployed as SCSI or
> ATA, my common sense screams to stick to what's known to work and
> don't venture outside unnecessarily.
I understand, but you should also consider the likelihood of the various
choices. We do have a list of quirks, and they all seem to be about
regular stuff like off-by-one errors with weird outcomes, not about "the
firmware writer gave random meanings to known commands".
You're also missing another point I made. Userspace is explicitly
asking for trouble in using SG_IO, and must deal with the consequences.
If the trouble is big, we should work around it anyway. If the trouble
is small, we need not worry about it.
Check how many "quirks" are in a typical burning program. Does that
mean that we should remove burning commands from the whitelist? Of
course not.
>> To try and give an answer, I grepped the "unusual" drivers in
>> drivers/usb/storage. All of them seem to use non-standard packets (i.e.
>> basically are not SCSI) for their strange stuff, except two:
>> initializers.c has one function that sends SCSI command 0xEC,
>> realtek_cr.c uses 0xF0, both vendor specific. So USB firmware writers,
>> despite being known for tweaking SCSI standards in many ways, seem to be
>> sensible in this respect.
>
> You're grepping for known irregularities which need to be used to make
> the device work.
Of course I did, you were talking about "weird init code to select some
mode" and I went looking for how they look like in the real world.
> Just read through, say, sd driver code and see how
> hard it tries to stick to what's known to work rather than venturing
> out to the space allowed by the spec.
That's a different story, once userspace uses SG_IO it takes the burden
of doing this.
> The default approach in this area definitely is and should be
> conservative in general.
Userspace of course must be written with care, too.
>> If it is not used, nobody will use it and setting the bit is effectively
>> useless (but harmless too).
>
> No, we don't know it's gonna be harmless. How do we *know* that
> outside of the naive expectation that devices are gonna safely and
> sanely ignore things that they don't understand?
First of all, we have a misunderstanding. I wrote "it is not used", and
I included _everything_ in that, even malware. In that case, it's a
tautology that it's useless and harmless.
Now, nobody asked about sane error handling. I have a USB pen drive
that returns a Unit Attention code for any command it does not
recognize. Does the existence of that USB pen drive mean that we should
remove MODE SENSE from the whitelist? Of course not. We just pass the
errors to userspace, and userspace deals with them.
In fact, even for safety we can only go so far. Other firmwares crash
if you read/write a block that is out of range. Should we remove
READ/WRITE from the whitelist? Of course not. We handle that as a
quirk in the USB driver.
I think the latter case can be generalized. Let's assume that there is
a bad guy that goes around and wants to brick your disk. It would not
be a very smart thing to do, just like Ebola is not a very smart virus,
but the world is full of strange people and he is just one of them.
If a particular firmware can brick a device from userspace with a
particular sequence of innocuous-looking (and non-vendor-specific)
commands, I believe that the kernel should have a workaround to protect
against that, independent of the privileges needed to send the commands
to begin with. This means that it is irrelevant to use these cases to
exclude commands from a whitelist. Independent of the fact that neither
the weird guy nor the weird firmwares have been shown to exist.
>>> You would get -EPERM/ACCES whatnot from base OS and you would know the
>>> command in use, no?
>>
>> Yes, but I would like to avoid many iterations. I don't have access
>> myself to all that software except Windows.
>
> I don't think it's gonna be that many iterations and would much prefer
> doing several iterations and having the backing data for each command
> added.
If I make a whitelist with all the commands that Linux sends, I'll have
many new commands in the whitelist and no old commands. The new
commands didn't exist when old drives were sold, so they are "dangerous"
in your opinion. At that point I might as well keep the whitelist
empty, no?
(Which BTW is what James proposed and I wrote the patch for, but you
rejected: make the whitelist entirely configurable in userspace, and add
a Kconfig option that makes the default whitelist very very small).
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 23:32 ` Paolo Bonzini
@ 2013-01-25 23:47 ` Tejun Heo
2013-01-26 10:18 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Tejun Heo @ 2013-01-25 23:47 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Hello, Paolo.
On Sat, Jan 26, 2013 at 12:32:36AM +0100, Paolo Bonzini wrote:
> If I make a whitelist with all the commands that Linux sends, I'll have
> many new commands in the whitelist and no old commands. The new
> commands didn't exist when old drives were sold, so they are "dangerous"
> in your opinion. At that point I might as well keep the whitelist
> empty, no?
Let's not go to extremes. It's not about theoretic correctness. It's
about how to appraoch a possibly messy practical problem. To me, it
seems natural to be conservative on this and add what's being acitvely
used, which as a bonus will also give us at least some chance of
evaluating what we have and why later on if it ever needs to be
changed.
I'm just not comfortable with adding a bunch of commands by simply
scanning the specs. Let's at least have some backing data and
justification for exposing new ones. I really don't think that's too
much to ask. Start with minimal set. Grow it as needed. We can
always grow but the other direction is much harder.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices
2013-01-25 23:47 ` Tejun Heo
@ 2013-01-26 10:18 ` Paolo Bonzini
0 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-01-26 10:18 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, pmatouse, James E.J. Bottomley, linux-scsi, Jens Axboe
Il 26/01/2013 00:47, Tejun Heo ha scritto:
>> > If I make a whitelist with all the commands that Linux sends, I'll have
>> > many new commands in the whitelist and no old commands. The new
>> > commands didn't exist when old drives were sold, so they are "dangerous"
>> > in your opinion. At that point I might as well keep the whitelist
>> > empty, no?
> Let's not go to extremes. It's not about theoretic correctness.
Once you start looking at what is already in the list, you'll see that
your problem is entirely theoretical, and any choice is going to be arbitrary.
There are already plenty of enabled commands that are not implemented by
most SCSI devices (not just cheap USB enclosures), and I don't see what
the difference should be between say "LOG SELECT" and "ORWRITE".
> It's about how to appraoch a possibly messy practical problem. To me,
> it seems natural to be conservative on this and add what's being
> acitvely used, which as a bonus will also give us at least some
> chance of evaluating what we have and why later on if it ever needs
> to be changed.
>
> I'm just not comfortable with adding a bunch of commands by simply
> scanning the specs. Let's at least have some backing data and
> justification for exposing new ones. I really don't think that's too
> much to ask. Start with minimal set. Grow it as needed. We can
> always grow but the other direction is much harder.
Ok, so I looked at the list. The vast majority of the commands are
added because Linux itself is using them.
Considering only commands that have a "D" (i.e. are supported by
"normal" disks), these maybe can be removed from patch 9. Probably
no one would notice:
+ sgio_bitmap_set(0x07, D| W| O , write); // REASSIGN BLOCKS
+ sgio_bitmap_set(0x29, D| W|R|O , read); // READ GENERATION
+ sgio_bitmap_set(0x2C, D| R|O , write); // ERASE(10)
+ sgio_bitmap_set(0x8B, D , write); // ORWRITE
These two are no-ops so I won't cry too much for them:
+ sgio_bitmap_set(0x34, D| W| O| K , read); // PRE-FETCH(10)
+ sgio_bitmap_set(0x90, D| W| O| B , read); // PRE-FETCH(16)
Everything else has to stay.
For tapes and media changers, all of the commands have to stay.
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2013-01-26 10:18 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-24 15:00 [PATCH 00/13] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542) Paolo Bonzini
2013-01-24 15:00 ` [PATCH 01/13] sg_io: pass request_queue to blk_verify_command Paolo Bonzini
2013-01-24 22:34 ` Tejun Heo
2013-01-24 15:00 ` [PATCH 02/13] sg_io: reorganize list of allowed commands Paolo Bonzini
2013-01-24 22:42 ` Tejun Heo
2013-01-24 22:49 ` Tejun Heo
2013-01-24 22:58 ` Tejun Heo
2013-01-25 10:01 ` Paolo Bonzini
2013-01-25 17:13 ` Tejun Heo
2013-01-25 17:26 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 03/13] sg_io: use different default filters for each device class Paolo Bonzini
2013-01-24 15:00 ` [PATCH 04/13] sg_io: resolve conflicts between commands assigned to multiple classes (CVE-2012-4542) Paolo Bonzini
2013-01-24 15:00 ` [PATCH 05/13] sg_io: whitelist a few more commands for rare & obsolete device types Paolo Bonzini
2013-01-24 15:00 ` [PATCH 06/13] sg_io: whitelist a few more commands for multimedia devices Paolo Bonzini
2013-01-24 22:55 ` Tejun Heo
2013-01-25 9:26 ` Paolo Bonzini
2013-01-25 17:04 ` Tejun Heo
2013-01-25 17:16 ` Paolo Bonzini
2013-01-25 17:28 ` Tejun Heo
2013-01-25 17:57 ` Paolo Bonzini
2013-01-25 18:13 ` Tejun Heo
2013-01-25 18:47 ` Paolo Bonzini
2013-01-25 19:01 ` Tejun Heo
2013-01-25 22:32 ` Paolo Bonzini
2013-01-25 22:41 ` Tejun Heo
2013-01-25 23:32 ` Paolo Bonzini
2013-01-25 23:47 ` Tejun Heo
2013-01-26 10:18 ` Paolo Bonzini
2013-01-24 15:00 ` [PATCH 07/13] sg_io: whitelist a few more commands for media changers Paolo Bonzini
2013-01-24 15:00 ` [PATCH 08/13] sg_io: whitelist a few more commands for tapes Paolo Bonzini
2013-01-24 15:00 ` [PATCH 09/13] sg_io: whitelist a few more commands for disks Paolo Bonzini
2013-01-24 15:00 ` [PATCH 10/13] sg_io: whitelist a few obsolete commands Paolo Bonzini
2013-01-24 15:00 ` [PATCH 11/13] sg_io: add list of commands that were in the consulted list but are disabled Paolo Bonzini
2013-01-24 15:00 ` [PATCH 12/13] sg_io: remove remnants of sysfs SG_IO filters Paolo Bonzini
2013-01-24 15:00 ` [PATCH 13/13] sg_io: introduce unpriv_sgio queue flag Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).