SG_IO and security

SG_IO and security

[Alan Cox]

Since the entire thread seems to have died again unresolved I'd suggest
the following patch should get into 2.6.8 so that anyone with read
access to any block device cannot issue arbitary scsi commands to it
(like writes or firmware erase)

--- drivers/block/scsi_ioctl.c~	2004-08-12 14:14:38.078821640 +0100
+++ drivers/block/scsi_ioctl.c	2004-08-12 14:14:38.079821488 +0100
@@ -115,6 +115,8 @@
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	unsigned char cmd[BLK_MAX_CDB];
 
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
 	if (hdr->interface_id != 'S')
 		return -EINVAL;
 	if (hdr->cmd_len > BLK_MAX_CDB)

Re: SG_IO and security

[Linus Torvalds]

On Thu, 12 Aug 2004, Alan Cox wrote:
>
> Since the entire thread seems to have died again unresolved I'd suggest
> the following patch should get into 2.6.8 so that anyone with read
> access to any block device cannot issue arbitary scsi commands to it
> (like writes or firmware erase)

Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).

I'll add that too.

		Linus

Re: SG_IO and security

[Linus Torvalds]

On Thu, 12 Aug 2004, Linus Torvalds wrote:
> 
> Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).

Btw, I think the _right_ thing to check is the write access of the file 
descriptor. If you have write access to a block device, you can delete the 
data, so you might as well be able to do the raw commands. And that would 
allow things like "disk" groups etc to work and burn CD's.

However, right now we don't even pass down the "struct file" to this 
function. We probably should. Anybody willing to go through the callers? 
(just a few callers, but things like cdrom_command() doesn't even have the 
file, so it has to be recursive).

		Linus

Re: SG_IO and security

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Thu, 12 Aug 2004, Linus Torvalds wrote:
> 
>>Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> 
> 
> Btw, I think the _right_ thing to check is the write access of the file 
> descriptor. If you have write access to a block device, you can delete the 
> data, so you might as well be able to do the raw commands. And that would 
> allow things like "disk" groups etc to work and burn CD's.


Define raw commands.  I certainly don't want non-root users to be able 
to issue FORMAT UNIT on my hard drive.

	Jeff

Re: SG_IO and security

[Jeff Garzik]

Jeff Garzik wrote:
> Linus Torvalds wrote:
> 
>>
>> On Thu, 12 Aug 2004, Linus Torvalds wrote:
>>
>>> Hmm.. This still allows the old "junk" commands 
>>> (SCSI_IOCTL_SEND_COMMAND).
>>
>>
>>
>> Btw, I think the _right_ thing to check is the write access of the 
>> file descriptor. If you have write access to a block device, you can 
>> delete the data, so you might as well be able to do the raw commands. 
>> And that would allow things like "disk" groups etc to work and burn CD's.
> 
> 
> 
> Define raw commands.  I certainly don't want non-root users to be able 
> to issue FORMAT UNIT on my hard drive.

oh, to the block device, not the file.

nevermind.

	Jeff

Re: SG_IO and security

[Linus Torvalds]

On Thu, 12 Aug 2004, Jeff Garzik wrote:
>
> Linus Torvalds wrote:
> > 
> > On Thu, 12 Aug 2004, Linus Torvalds wrote:
> > 
> >>Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> > 
> > 
> > Btw, I think the _right_ thing to check is the write access of the file 
> > descriptor. If you have write access to a block device, you can delete the 
> > data, so you might as well be able to do the raw commands. And that would 
> > allow things like "disk" groups etc to work and burn CD's.
> 
> Define raw commands.  I certainly don't want non-root users to be able 
> to issue FORMAT UNIT on my hard drive.

Ehh? The same ones you allow to write all over the raw device?

Let's see now:

	brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda

would you put people you don't trust with your disk in the "disk" group?

Right. If you trust somebody enough that you give him write access to the 
disk, then you might as well trust him enough to do commands on it. 

Conversely, if you don't trust him enough to do things like that, you 
shouldn't give him write access in the first place.

It's a hell of a lot easier to destroy a disk with

	dd if=/dev/zero of=/dev/xxx bs=8k

than it is to do it with some special malicious command. 

And yes, there's clearly a difference, but in general I'd say it is the 
_data_ on the disk that is worth something to you. The disk itself? Do you 
really fundamentally care?

		Linus

Re: SG_IO and security

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]

On Thu, 2004-08-12 at 18:55, Jeff Garzik wrote:

> Define raw commands.  I certainly don't want non-root users to be able 
> to issue FORMAT UNIT on my hard drive.


then don't give them write access ;)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: SG_IO and security

[Jeff Garzik]

Linus Torvalds wrote:
> Let's see now:
> 
> 	brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
> 
> would you put people you don't trust with your disk in the "disk" group?
> 
> Right. If you trust somebody enough that you give him write access to the 
> disk, then you might as well trust him enough to do commands on it. 


Yeah, I agree.  I was thinking write access to files on a hard drive, 
not write access to the blkdev itself.

	Jeff

Re: SG_IO and security

[Jens Axboe]

On Thu, Aug 12 2004, Linus Torvalds wrote:
> 
> 
> On Thu, 12 Aug 2004, Linus Torvalds wrote:
> > 
> > Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> 
> Btw, I think the _right_ thing to check is the write access of the file 
> descriptor. If you have write access to a block device, you can delete the 
> data, so you might as well be able to do the raw commands. And that would 
> allow things like "disk" groups etc to work and burn CD's.
> 
> However, right now we don't even pass down the "struct file" to this 
> function. We probably should. Anybody willing to go through the callers? 
> (just a few callers, but things like cdrom_command() doesn't even have the 
> file, so it has to be recursive).

Precisely, I guess that's a 2.6.9 job then. CDROM_SEND_PACKET works
directly on top of SG_IO now, so should just work as long as sg_io() is
translated.

I have no problem going over this, if somebody beats me to it, fine.
I'll be gone on vacation next week.

-- 
Jens Axboe

Re: SG_IO and security

[Jens Axboe]

On Thu, Aug 12 2004, Jens Axboe wrote:
> On Thu, Aug 12 2004, Linus Torvalds wrote:
> > 
> > 
> > On Thu, 12 Aug 2004, Linus Torvalds wrote:
> > > 
> > > Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> > 
> > Btw, I think the _right_ thing to check is the write access of the file 
> > descriptor. If you have write access to a block device, you can delete the 
> > data, so you might as well be able to do the raw commands. And that would 
> > allow things like "disk" groups etc to work and burn CD's.
> > 
> > However, right now we don't even pass down the "struct file" to this 
> > function. We probably should. Anybody willing to go through the callers? 
> > (just a few callers, but things like cdrom_command() doesn't even have the 
> > file, so it has to be recursive).
> 
> Precisely, I guess that's a 2.6.9 job then. CDROM_SEND_PACKET works
> directly on top of SG_IO now, so should just work as long as sg_io() is
> translated.
> 
> I have no problem going over this, if somebody beats me to it, fine.
> I'll be gone on vacation next week.

Here's a much better and more complete patch, imho, and doing it was no
more than 20 minutes. It struck me while doing this that you could
possibly get around just checking the data direction given in the
sg_io_hdr. The advantage of that would be that you don't need a command
table at all, the disadvantage is that it would still be possible for a
given user with read-only access to a device to possibly confuse the
device and/or cause bus resets. So it probably wont work. I added it as
an extra check, though.

The table is as complete as cdrom.h is, plus the 6 and 16 byte variants
from scsi.h. There are still commands missing, they can be added along
the way though. Patch is compile tested.

===== drivers/block/scsi_ioctl.c 1.50 vs edited =====
--- 1.50/drivers/block/scsi_ioctl.c	2004-08-12 18:43:03 +02:00
+++ edited/drivers/block/scsi_ioctl.c	2004-08-12 20:22:08 +02:00
@@ -105,8 +105,56 @@
 	return put_user(1, p);
 }
 
+static int sg_allowed_cmd(unsigned char opcode, int may_write)
+{
+	if (capable(CAP_SYS_RAWIO))
+		return 1;
+	if (may_write)
+		return 1;
+
+	switch (opcode) {
+		case GPCMD_GET_CONFIGURATION:
+		case GPCMD_GET_EVENT_STATUS_NOTIFICATION:
+		case GPCMD_GET_PERFORMANCE:
+		case GPCMD_INQUIRY:
+		case GPCMD_MECHANISM_STATUS:
+		case MODE_SENSE:
+		case LOG_SENSE:
+		case GPCMD_MODE_SENSE_10:
+		case GPCMD_PLAY_AUDIO_10:
+		case GPCMD_PLAY_AUDIO_MSF:
+		case GPCMD_PLAY_AUDIO_TI:
+		case GPCMD_PLAY_CD:
+		case READ_6:
+		case GPCMD_READ_10:
+		case GPCMD_READ_12:
+		case READ_16:
+		case GPCMD_READ_CDVD_CAPACITY:
+		case GPCMD_READ_CD:
+		case GPCMD_READ_CD_MSF:
+		case GPCMD_READ_DISC_INFO:
+		case GPCMD_READ_DVD_STRUCTURE:
+		case GPCMD_READ_HEADER:
+		case GPCMD_READ_TRACK_RZONE_INFO:
+		case GPCMD_READ_SUBCHANNEL:
+		case GPCMD_READ_TOC_PMA_ATIP:
+		case GPCMD_REPORT_KEY:
+		case GPCMD_REQUEST_SENSE:
+		case GPCMD_SCAN:
+		case GPCMD_SEEK:
+		case GPCMD_START_STOP_UNIT:
+		case GPCMD_STOP_PLAY_SCAN:
+		case GPCMD_TEST_UNIT_READY:
+		case GPCMD_VERIFY_10:
+		case READ_BUFFER:
+			return 1;
+		default:
+			return 0;
+	}
+}
+
 static int sg_io(request_queue_t *q, struct gendisk *bd_disk,
-		 struct sg_io_hdr *hdr)
+		 struct sg_io_hdr *hdr, int may_write)
 {
 	unsigned long start_time;
 	int reading, writing;
@@ -115,14 +163,14 @@
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	unsigned char cmd[BLK_MAX_CDB];
 
-	if (!capable(CAP_SYS_RAWIO))
-		return -EPERM;
 	if (hdr->interface_id != 'S')
 		return -EINVAL;
 	if (hdr->cmd_len > BLK_MAX_CDB)
 		return -EINVAL;
 	if (copy_from_user(cmd, hdr->cmdp, hdr->cmd_len))
 		return -EFAULT;
+	if (!sg_allowed_cmd(cmd[0], may_write))
+		return -EPERM;
 
 	/*
 	 * we'll do that later
@@ -149,6 +197,9 @@
 			break;
 		}
 
+		if (writing && !may_write)
+			return -EPERM;
+
 		rq = blk_rq_map_user(q, writing ? WRITE : READ, hdr->dxferp,
 				     hdr->dxfer_len);
 
@@ -229,14 +280,12 @@
 #define OMAX_SB_LEN 16          /* For backward compatibility */
 
 static int sg_scsi_ioctl(request_queue_t *q, struct gendisk *bd_disk,
-			 Scsi_Ioctl_Command __user *sic)
+			 Scsi_Ioctl_Command __user *sic, int may_write)
 {
 	struct request *rq;
 	int err, in_len, out_len, bytes, opcode, cmdlen;
 	char *buffer = NULL, sense[SCSI_SENSE_BUFFERSIZE];
 
-	if (!capable(CAP_SYS_RAWIO))
-		return -EPERM;
 	/*
 	 * get in an out lengths, verify they don't exceed a page worth of data
 	 */
@@ -323,8 +372,10 @@
 	return err;
 }
 
-int scsi_cmd_ioctl(struct gendisk *bd_disk, unsigned int cmd, void __user *arg)
+int scsi_cmd_ioctl(struct gendisk *bd_disk, struct file *file, unsigned int cmd,
+		   void __user *arg)
 {
+	int may_write = file->f_flags & FMODE_WRITE;
 	request_queue_t *q;
 	struct request *rq;
 	int close = 0, err;
@@ -370,7 +421,7 @@
 			err = -EFAULT;
 			if (copy_from_user(&hdr, arg, sizeof(hdr)))
 				break;
-			err = sg_io(q, bd_disk, &hdr);
+			err = sg_io(q, bd_disk, &hdr, may_write);
 			if (err == -EFAULT)
 				break;
 
@@ -418,7 +469,7 @@
 			hdr.cmdp = ((struct cdrom_generic_command __user*) arg)->cmd;
 			hdr.cmd_len = sizeof(cgc.cmd);
 
-			err = sg_io(q, bd_disk, &hdr);
+			err = sg_io(q, bd_disk, &hdr, may_write);
 			if (err == -EFAULT)
 				break;
 
@@ -441,7 +492,7 @@
 			if (!arg)
 				break;
 
-			err = sg_scsi_ioctl(q, bd_disk, arg);
+			err = sg_scsi_ioctl(q, bd_disk, arg, may_write);
 			break;
 		case CDROMCLOSETRAY:
 			close = 1;
===== drivers/block/paride/pcd.c 1.38 vs edited =====
--- 1.38/drivers/block/paride/pcd.c	2004-07-12 10:01:05 +02:00
+++ edited/drivers/block/paride/pcd.c	2004-08-12 20:26:39 +02:00
@@ -259,7 +259,7 @@
 				unsigned cmd, unsigned long arg)
 {
 	struct pcd_unit *cd = inode->i_bdev->bd_disk->private_data;
-	return cdrom_ioctl(&cd->info, inode, cmd, arg);
+	return cdrom_ioctl(&cd->info, inode, file, cmd, arg);
 }
 
 static int pcd_block_media_changed(struct gendisk *disk)
===== drivers/cdrom/cdrom.c 1.65 vs edited =====
--- 1.65/drivers/cdrom/cdrom.c	2004-08-08 08:43:40 +02:00
+++ edited/drivers/cdrom/cdrom.c	2004-08-12 20:23:06 +02:00
@@ -2073,13 +2073,14 @@
  * mmc_ioct().
  */
 int cdrom_ioctl(struct cdrom_device_info *cdi, struct inode *ip,
-		unsigned int cmd, unsigned long arg)
+		struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct cdrom_device_ops *cdo = cdi->ops;
+	struct gendisk *disk = ip->i_bdev->bd_disk;
 	int ret;
 
 	/* Try the generic SCSI command ioctl's first.. */
-	ret = scsi_cmd_ioctl(ip->i_bdev->bd_disk, cmd, (void __user *)arg);
+	ret = scsi_cmd_ioctl(disk, file, cmd, (void __user *) arg);
 	if (ret != -ENOTTY)
 		return ret;
 
===== drivers/cdrom/cdu31a.c 1.42 vs edited =====
--- 1.42/drivers/cdrom/cdu31a.c	2004-06-04 01:05:29 +02:00
+++ edited/drivers/cdrom/cdu31a.c	2004-08-12 20:12:01 +02:00
@@ -3179,7 +3179,7 @@
 static int scd_block_ioctl(struct inode *inode, struct file *file,
 				unsigned cmd, unsigned long arg)
 {
-	return cdrom_ioctl(&scd_info, inode, cmd, arg);
+	return cdrom_ioctl(&scd_info, inode, file, cmd, arg);
 }
 
 static int scd_block_media_changed(struct gendisk *disk)
===== drivers/cdrom/cm206.c 1.37 vs edited =====
--- 1.37/drivers/cdrom/cm206.c	2004-03-16 11:29:43 +01:00
+++ edited/drivers/cdrom/cm206.c	2004-08-12 20:12:01 +02:00
@@ -1363,7 +1363,7 @@
 static int cm206_block_ioctl(struct inode *inode, struct file *file,
 				unsigned cmd, unsigned long arg)
 {
-	return cdrom_ioctl(&cm206_info, inode, cmd, arg);
+	return cdrom_ioctl(&cm206_info, inode, file, cmd, arg);
 }
 
 static int cm206_block_media_changed(struct gendisk *disk)
===== drivers/cdrom/mcd.c 1.35 vs edited =====
--- 1.35/drivers/cdrom/mcd.c	2003-09-04 08:40:13 +02:00
+++ edited/drivers/cdrom/mcd.c	2004-08-12 20:12:01 +02:00
@@ -227,7 +227,7 @@
 static int mcd_block_ioctl(struct inode *inode, struct file *file,
 				unsigned cmd, unsigned long arg)
 {
-	return cdrom_ioctl(&mcd_info, inode, cmd, arg);
+	return cdrom_ioctl(&mcd_info, inode, file, cmd, arg);
 }
 
 static int mcd_block_media_changed(struct gendisk *disk)
===== drivers/cdrom/mcdx.c 1.37 vs edited =====
--- 1.37/drivers/cdrom/mcdx.c	2004-07-14 14:18:48 +02:00
+++ edited/drivers/cdrom/mcdx.c	2004-08-12 20:12:01 +02:00
@@ -233,7 +233,7 @@
 				unsigned cmd, unsigned long arg)
 {
 	struct s_drive_stuff *p = inode->i_bdev->bd_disk->private_data;
-	return cdrom_ioctl(&p->info, inode, cmd, arg);
+	return cdrom_ioctl(&p->info, inode, file, cmd, arg);
 }
 
 static int mcdx_block_media_changed(struct gendisk *disk)
===== drivers/cdrom/sbpcd.c 1.52 vs edited =====
--- 1.52/drivers/cdrom/sbpcd.c	2004-08-02 10:00:45 +02:00
+++ edited/drivers/cdrom/sbpcd.c	2004-08-12 20:12:01 +02:00
@@ -5372,7 +5372,7 @@
 				unsigned cmd, unsigned long arg)
 {
 	struct sbpcd_drive *p = inode->i_bdev->bd_disk->private_data;
-	return cdrom_ioctl(p->sbpcd_infop, inode, cmd, arg);
+	return cdrom_ioctl(p->sbpcd_infop, inode, file, cmd, arg);
 }
 
 static int sbpcd_block_media_changed(struct gendisk *disk)
===== drivers/cdrom/viocd.c 1.6 vs edited =====
--- 1.6/drivers/cdrom/viocd.c	2004-06-29 16:44:46 +02:00
+++ edited/drivers/cdrom/viocd.c	2004-08-12 20:12:01 +02:00
@@ -199,7 +199,7 @@
 		unsigned cmd, unsigned long arg)
 {
 	struct disk_info *di = inode->i_bdev->bd_disk->private_data;
-	return cdrom_ioctl(&di->viocd_info, inode, cmd, arg);
+	return cdrom_ioctl(&di->viocd_info, inode, file, cmd, arg);
 }
 
 static int viocd_blk_media_changed(struct gendisk *disk)
===== drivers/ide/ide-cd.c 1.88 vs edited =====
--- 1.88/drivers/ide/ide-cd.c	2004-08-03 08:10:56 +02:00
+++ edited/drivers/ide/ide-cd.c	2004-08-12 20:12:01 +02:00
@@ -3395,10 +3395,10 @@
 {
 	struct block_device *bdev = inode->i_bdev;
 	ide_drive_t *drive = bdev->bd_disk->private_data;
-	int err = generic_ide_ioctl(bdev, cmd, arg);
+	int err = generic_ide_ioctl(bdev, file, cmd, arg);
 	if (err == -EINVAL) {
 		struct cdrom_info *info = drive->driver_data;
-		err = cdrom_ioctl(&info->devinfo, inode, cmd, arg);
+		err = cdrom_ioctl(&info->devinfo, inode, file, cmd, arg);
 	}
 	return err;
 }
===== drivers/ide/ide-disk.c 1.87 vs edited =====
--- 1.87/drivers/ide/ide-disk.c	2004-06-25 18:49:26 +02:00
+++ edited/drivers/ide/ide-disk.c	2004-08-12 20:12:01 +02:00
@@ -1668,7 +1668,7 @@
 			unsigned int cmd, unsigned long arg)
 {
 	struct block_device *bdev = inode->i_bdev;
-	return generic_ide_ioctl(bdev, cmd, arg);
+	return generic_ide_ioctl(bdev, file, cmd, arg);
 }
 
 static int idedisk_media_changed(struct gendisk *disk)
===== drivers/ide/ide-floppy.c 1.39 vs edited =====
--- 1.39/drivers/ide/ide-floppy.c	2004-06-04 06:12:07 +02:00
+++ edited/drivers/ide/ide-floppy.c	2004-08-12 20:12:01 +02:00
@@ -1946,7 +1946,7 @@
 	ide_drive_t *drive = bdev->bd_disk->private_data;
 	idefloppy_floppy_t *floppy = drive->driver_data;
 	void __user *argp = (void __user *)arg;
-	int err = generic_ide_ioctl(bdev, cmd, arg);
+	int err = generic_ide_ioctl(bdev, file, cmd, arg);
 	int prevent = (arg) ? 1 : 0;
 	idefloppy_pc_t pc;
 	if (err != -EINVAL)
===== drivers/ide/ide-tape.c 1.42 vs edited =====
--- 1.42/drivers/ide/ide-tape.c	2004-08-08 04:07:36 +02:00
+++ edited/drivers/ide/ide-tape.c	2004-08-12 20:12:01 +02:00
@@ -4807,7 +4807,7 @@
 {
 	struct block_device *bdev = inode->i_bdev;
 	ide_drive_t *drive = bdev->bd_disk->private_data;
-	int err = generic_ide_ioctl(bdev, cmd, arg);
+	int err = generic_ide_ioctl(bdev, file, cmd, arg);
 	if (err == -EINVAL)
 		err = idetape_blkdev_ioctl(drive, cmd, arg);
 	return err;
===== drivers/ide/ide.c 1.151 vs edited =====
--- 1.151/drivers/ide/ide.c	2004-06-30 18:22:19 +02:00
+++ edited/drivers/ide/ide.c	2004-08-12 20:23:35 +02:00
@@ -1453,8 +1453,8 @@
 	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
 }
 
-int generic_ide_ioctl(struct block_device *bdev, unsigned int cmd,
-			unsigned long arg)
+int generic_ide_ioctl(struct block_device *bdev, struct file *file,
+			unsigned int cmd, unsigned long arg)
 {
 	ide_drive_t *drive = bdev->bd_disk->private_data;
 	ide_settings_t *setting;
@@ -1603,9 +1603,15 @@
 			return 0;
 		}
 
+		/*
+		 * should just be moved on top and passed first for all
+		 * cmds (needs to be for ide-tape/floppy to work anyways),
+		 * would need to check that there are no overlapping ioctls
+		 * between scsi and ide first
+		 */
 		case CDROMEJECT:
 		case CDROMCLOSETRAY:
-			return scsi_cmd_ioctl(bdev->bd_disk, cmd, p);
+			return scsi_cmd_ioctl(bdev->bd_disk, file, cmd, p);
 
 		case HDIO_GET_BUSSTATE:
 			if (!capable(CAP_SYS_ADMIN))
===== drivers/scsi/ide-scsi.c 1.42 vs edited =====
--- 1.42/drivers/scsi/ide-scsi.c	2004-06-18 19:06:33 +02:00
+++ edited/drivers/scsi/ide-scsi.c	2004-08-12 20:20:33 +02:00
@@ -735,7 +735,7 @@
 			unsigned int cmd, unsigned long arg)
 {
 	struct block_device *bdev = inode->i_bdev;
-	return generic_ide_ioctl(bdev, cmd, arg);
+	return generic_ide_ioctl(bdev, file, cmd, arg);
 }
 
 static struct block_device_operations idescsi_ops = {
===== drivers/scsi/sd.c 1.154 vs edited =====
--- 1.154/drivers/scsi/sd.c	2004-06-19 16:38:40 +02:00
+++ edited/drivers/scsi/sd.c	2004-08-12 20:24:06 +02:00
@@ -594,7 +594,7 @@
 		case SCSI_IOCTL_GET_BUS_NUMBER:
 			return scsi_ioctl(sdp, cmd, p);
 		default:
-			error = scsi_cmd_ioctl(disk, cmd, p);
+			error = scsi_cmd_ioctl(disk, filp, cmd, p);
 			if (error != -ENOTTY)
 				return error;
 	}
===== drivers/scsi/sr.c 1.113 vs edited =====
--- 1.113/drivers/scsi/sr.c	2004-07-15 22:29:34 +02:00
+++ edited/drivers/scsi/sr.c	2004-08-12 20:21:09 +02:00
@@ -504,7 +504,7 @@
                 case SCSI_IOCTL_GET_BUS_NUMBER:
                         return scsi_ioctl(sdev, cmd, (void __user *)arg);
 	}
-	return cdrom_ioctl(&cd->cdi, inode, cmd, arg);
+	return cdrom_ioctl(&cd->cdi, inode, file, cmd, arg);
 }
 
 static int sr_block_media_changed(struct gendisk *disk)
===== drivers/scsi/st.c 1.92 vs edited =====
--- 1.92/drivers/scsi/st.c	2004-08-08 04:07:36 +02:00
+++ edited/drivers/scsi/st.c	2004-08-12 20:22:42 +02:00
@@ -3408,7 +3408,7 @@
 		case SCSI_IOCTL_GET_BUS_NUMBER:
 			break;
 		default:
-			i = scsi_cmd_ioctl(STp->disk, cmd_in, p);
+			i = scsi_cmd_ioctl(STp->disk, file, cmd_in, p);
 			if (i != -ENOTTY)
 				return i;
 			break;
===== include/linux/blkdev.h 1.149 vs edited =====
--- 1.149/include/linux/blkdev.h	2004-08-03 08:10:56 +02:00
+++ edited/include/linux/blkdev.h	2004-08-12 20:21:28 +02:00
@@ -517,7 +517,7 @@
 extern void blk_recount_segments(request_queue_t *, struct bio *);
 extern int blk_phys_contig_segment(request_queue_t *q, struct bio *, struct bio *);
 extern int blk_hw_contig_segment(request_queue_t *q, struct bio *, struct bio *);
-extern int scsi_cmd_ioctl(struct gendisk *, unsigned int, void __user *);
+extern int scsi_cmd_ioctl(struct gendisk *, struct file *, unsigned int, void __user *);
 extern void blk_start_queue(request_queue_t *q);
 extern void blk_stop_queue(request_queue_t *q);
 extern void __blk_stop_queue(request_queue_t *q);
===== include/linux/cdrom.h 1.20 vs edited =====
--- 1.20/include/linux/cdrom.h	2004-06-04 06:18:16 +02:00
+++ edited/include/linux/cdrom.h	2004-08-12 20:12:01 +02:00
@@ -985,7 +985,7 @@
 			struct file *fp);
 extern int cdrom_release(struct cdrom_device_info *cdi, struct file *fp);
 extern int cdrom_ioctl(struct cdrom_device_info *cdi, struct inode *ip,
-		unsigned int cmd, unsigned long arg);
+		struct file *file, unsigned int cmd, unsigned long arg);
 extern int cdrom_media_changed(struct cdrom_device_info *);
 
 extern int register_cdrom(struct cdrom_device_info *cdi);
===== include/linux/ide.h 1.127 vs edited =====
--- 1.127/include/linux/ide.h	2004-06-19 03:03:39 +02:00
+++ edited/include/linux/ide.h	2004-08-12 20:12:01 +02:00
@@ -1194,7 +1194,7 @@
 
 #define DRIVER(drive)		((drive)->driver)
 
-extern int generic_ide_ioctl(struct block_device *, unsigned, unsigned long);
+extern int generic_ide_ioctl(struct block_device *, struct file *, unsigned, unsigned long);
 
 /*
  * ide_hwifs[] is the master data structure used to keep track

-- 
Jens Axboe

Re: SG_IO and security

[Jeff Garzik]

On Thu, Aug 12, 2004 at 08:29:14PM +0200, Jens Axboe wrote:
>  			close = 1;
> ===== drivers/block/paride/pcd.c 1.38 vs edited =====
> --- 1.38/drivers/block/paride/pcd.c	2004-07-12 10:01:05 +02:00
> +++ edited/drivers/block/paride/pcd.c	2004-08-12 20:26:39 +02:00
> @@ -259,7 +259,7 @@
>  				unsigned cmd, unsigned long arg)
>  {
>  	struct pcd_unit *cd = inode->i_bdev->bd_disk->private_data;
> -	return cdrom_ioctl(&cd->info, inode, cmd, arg);
> +	return cdrom_ioctl(&cd->info, inode, file, cmd, arg);


If you have the struct file, can't you eliminate the inode argument?

	Jeff

Re: SG_IO and security

[Jens Axboe]

On Thu, Aug 12 2004, Jeff Garzik wrote:
> On Thu, Aug 12, 2004 at 08:29:14PM +0200, Jens Axboe wrote:
> >  			close = 1;
> > ===== drivers/block/paride/pcd.c 1.38 vs edited =====
> > --- 1.38/drivers/block/paride/pcd.c	2004-07-12 10:01:05 +02:00
> > +++ edited/drivers/block/paride/pcd.c	2004-08-12 20:26:39 +02:00
> > @@ -259,7 +259,7 @@
> >  				unsigned cmd, unsigned long arg)
> >  {
> >  	struct pcd_unit *cd = inode->i_bdev->bd_disk->private_data;
> > -	return cdrom_ioctl(&cd->info, inode, cmd, arg);
> > +	return cdrom_ioctl(&cd->info, inode, file, cmd, arg);
> 
> 
> If you have the struct file, can't you eliminate the inode argument?

How so?

-- 
Jens Axboe

Re: SG_IO and security

[Christoph Hellwig]

On Thu, Aug 12, 2004 at 08:43:42PM +0200, Jens Axboe wrote:
> > If you have the struct file, can't you eliminate the inode argument?
> 
> How so?

file->f_dentry->d_inode

Re: SG_IO and security

[Jens Axboe]

On Thu, Aug 12 2004, Christoph Hellwig wrote:
> On Thu, Aug 12, 2004 at 08:43:42PM +0200, Jens Axboe wrote:
> > > If you have the struct file, can't you eliminate the inode argument?
> > 
> > How so?
> 
> file->f_dentry->d_inode

Ah, ok. IMHO that should be generally done for the file_op->ioctl then.
But could easily be killed here of course.

-- 
Jens Axboe

Re: SG_IO and security

[Kai Makisara]

On Thu, 12 Aug 2004, Linus Torvalds wrote:

> 
> 
> On Thu, 12 Aug 2004, Jeff Garzik wrote:
> >
> > Linus Torvalds wrote:
> > > 
> > > On Thu, 12 Aug 2004, Linus Torvalds wrote:
> > > 
> > >>Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> > > 
> > > 
> > > Btw, I think the _right_ thing to check is the write access of the file 
> > > descriptor. If you have write access to a block device, you can delete the 
> > > data, so you might as well be able to do the raw commands. And that would 
> > > allow things like "disk" groups etc to work and burn CD's.
> > 
> > Define raw commands.  I certainly don't want non-root users to be able 
> > to issue FORMAT UNIT on my hard drive.
> 
> Ehh? The same ones you allow to write all over the raw device?
> 
> Let's see now:
> 
> 	brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
> 
> would you put people you don't trust with your disk in the "disk" group?
> 
This protects disks in practice but SG_IO is currently supported by other 
devices, at least SCSI tapes. It is reasonable in some organizations to 
give r/w access to ordinary users so that they can read/write tapes. I 
would be worried if this would enable the users, for instance, to mess up 
the mode page contents of the drive or change the firmware.

-- 
Kai

Re: SG_IO and security

[Alan Cox]

On Iau, 2004-08-12 at 17:45, Linus Torvalds wrote:
> On Thu, 12 Aug 2004, Linus Torvalds wrote:
> > 
> > Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).

That uses sg_io() so gets caught as well unless I screwed up following
the code paths.

> Btw, I think the _right_ thing to check is the write access of the file 
> descriptor. If you have write access to a block device, you can delete the 
> data, so you might as well be able to do the raw commands. And that would 
> allow things like "disk" groups etc to work and burn CD's.

With the current code I can destroy all your hard disks given read
access to the drive. With checks on writable I can destroy all your hard
disks/cdroms as appropriate with write access.

Destroy here means "dead, defunct, pushing up the daisies, go order
a new one kind of dead".

In essence the interface (and the SCSI/ATA/.. layers below) don't
seperate media and device. This also kicks in for partitioning since
write access to /dev/hda1 giving me SG_IO scsi access doesn't enforce
partitioning.

We end up needing some notion of what commands should be allowed to any
user and what commands should be allowed solely to superusers. That
leads to a second question for you which is one I had an argument about
Jens on.

Do we

a) Have code that essentially says "if read on base device can do ....,
if write can do ... , else capable(...)"

b) ioctls/other command functionality for the stuff users should be
allowed to do. 

Option (a) means parsing command blocks which are pretty regular and
parseable. 

Alan

Re: SG_IO and security

[Alan Cox]

On Iau, 2004-08-12 at 19:29, Jens Axboe wrote:
> +static int sg_allowed_cmd(unsigned char opcode, int may_write)
> +{
> +	if (capable(CAP_SYS_RAWIO))
> +		return 1;
> +	if (may_write)
> +		return 1;

I agree with passing the data down, unfortunately anyone with a raw
device access they can open for write can still physically anihiliate
the hardware. That causes real problems for anyone allocating partitions
for databases like Oracle, giving direct user access to devices for
virtualization like UML, giving direct user access to a M/O drive.

It also doesn't solve the read/write outside of partition problem.


Alan

Re: SG_IO and security

[Eric Lammerts]

On Thu, 12 Aug 2004, Alan Cox wrote:
> In essence the interface (and the SCSI/ATA/.. layers below) don't
> seperate media and device. This also kicks in for partitioning since
> write access to /dev/hda1 giving me SG_IO scsi access doesn't enforce
> partitioning.

But who needs SG_IO on partition devices?
Why not simply disallow it completely for non-root users?

Eric

Re: SG_IO and security

[Linus Torvalds]

On Thu, 12 Aug 2004, Alan Cox wrote:
> > > 
> > > Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> 
> That uses sg_io() so gets caught as well unless I screwed up following
> the code paths.

No, while the cdrom_ioctl thing does use sg_io, the really old and 
horrible sg_scsi_ioctl thing does it's own commands by hand.

I don't know why. I get the feeling that it _should_ use sg_io().

> With the current code I can destroy all your hard disks given read
> access to the drive.

Oh, I clearly agree with you that something had to be done. Which is why I 
did an extended version of your patch already.

> Do we
> 
> a) Have code that essentially says "if read on base device can do ....,
> if write can do ... , else capable(...)"
> 
> b) ioctls/other command functionality for the stuff users should be
> allowed to do. 
> 
> Option (a) means parsing command blocks which are pretty regular and
> parseable. 

Yes. I think we should do:
 - harmless read-only commands that we know, we allow for read opens
 - harmless write-only commands that we know, we allow for write opens
 - commands we don't know, or commands that aren't harmless, we require 
   RAWIO-capability for.

That seems fairly trivial to implement, and is clearly the user-friendly 
thing to do. The quick patch is just too damn unfriendly.

		Linus

Re: SG_IO and security

[Jens Axboe]

On Thu, Aug 12 2004, Linus Torvalds wrote:
> 
> 
> On Thu, 12 Aug 2004, Alan Cox wrote:
> > > > 
> > > > Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
> > 
> > That uses sg_io() so gets caught as well unless I screwed up following
> > the code paths.
> 
> No, while the cdrom_ioctl thing does use sg_io, the really old and 
> horrible sg_scsi_ioctl thing does it's own commands by hand.
> 
> I don't know why. I get the feeling that it _should_ use sg_io().

While that does make sense, it would be more code to fold them together
than what is currently there. SCSI_IOCTL_SEND_COMMAND is really
horrible, the person inventing that API should be subject to daily
public ridicule.

-- 
Jens Axboe

Re: SG_IO and security

[viro]

On Fri, Aug 13, 2004 at 08:59:03AM +0200, Jens Axboe wrote:
 
> While that does make sense, it would be more code to fold them together
> than what is currently there. SCSI_IOCTL_SEND_COMMAND is really
> horrible, the person inventing that API should be subject to daily
> public ridicule.

Introduced in 0.96a, between Apr 9 1992 and Apr 24 1992; part of initial
SCSI merge, so probably existed earlier than that (ultrastor claims to
be 1991, aha1542 appears to have started in January 1992).  Drew Eckhardt,
most likely...

Re: SG_IO and security

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]


> While that does make sense, it would be more code to fold them together
> than what is currently there. SCSI_IOCTL_SEND_COMMAND is really
> horrible, the person inventing that API should be subject to daily
> public ridicule.

sounds like deprecation of this interface should start (I suspect this
one will need ample notice of that so the sooner we do .... )

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: SG_IO and security

[Jens Axboe]

On Fri, Aug 13 2004, Arjan van de Ven wrote:
> 
> > While that does make sense, it would be more code to fold them together
> > than what is currently there. SCSI_IOCTL_SEND_COMMAND is really
> > horrible, the person inventing that API should be subject to daily
> > public ridicule.
> 
> sounds like deprecation of this interface should start (I suspect this

Very much so, it should have been deprecated for the last 5 years :)

> one will need ample notice of that so the sooner we do .... )

I have no idea how many apps use this ioctl, does anyone have a rough
list?

-- 
Jens Axboe

Re: SG_IO and security

[Jeff Garzik]

Jens Axboe wrote:
> On Fri, Aug 13 2004, Arjan van de Ven wrote:
> 
>>>While that does make sense, it would be more code to fold them together
>>>than what is currently there. SCSI_IOCTL_SEND_COMMAND is really
>>>horrible, the person inventing that API should be subject to daily
>>>public ridicule.
>>
>>sounds like deprecation of this interface should start (I suspect this
> 
> 
> Very much so, it should have been deprecated for the last 5 years :)
> 
> 
>>one will need ample notice of that so the sooner we do .... )
> 
> 
> I have no idea how many apps use this ioctl, does anyone have a rough
> list?


Add a rate-limited "this feature is deprecated" feature and find out...

	Jeff

Re: SG_IO and security

[Peter Jones]

On Thu, 12 Aug 2004 22:22:36 +0300 (EEST), Kai Makisara
<kai.makisara@kolumbus.fi> wrote:
> On Thu, 12 Aug 2004, Linus Torvalds wrote:
> > Let's see now:
> >
> >       brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
> >
> > would you put people you don't trust with your disk in the "disk" group?
> > 
> This protects disks in practice but SG_IO is currently supported by other
> devices, at least SCSI tapes. It is reasonable in some organizations to
> give r/w access to ordinary users so that they can read/write tapes. I
> would be worried if this would enable the users, for instance, to mess up
> the mode page contents of the drive or change the firmware.

Sure, but for that we need command based filtering.

This is at least a step in the right direction.

Re: SG_IO and security

[Linus Torvalds]

On Fri, 13 Aug 2004, Jeff Garzik wrote:
>2B
> Jens Axboe wrote:
> > 
> > I have no idea how many apps use this ioctl, does anyone have a rough
> > list?
> 
> Add a rate-limited "this feature is deprecated" feature and find out...

Googling for it does show it as being documented and apparently used by a 
few programs, at least...

		Linus

Re: SG_IO and security

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

Peter Jones wrote:
> On Thu, 12 Aug 2004 22:22:36 +0300 (EEST), Kai Makisara
> <kai.makisara@kolumbus.fi> wrote:
> 
>>On Thu, 12 Aug 2004, Linus Torvalds wrote:
>>
>>>Let's see now:
>>>
>>>      brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
>>>
>>>would you put people you don't trust with your disk in the "disk" group?
>>>
>>
>>This protects disks in practice but SG_IO is currently supported by other
>>devices, at least SCSI tapes. It is reasonable in some organizations to
>>give r/w access to ordinary users so that they can read/write tapes. I
>>would be worried if this would enable the users, for instance, to mess up
>>the mode page contents of the drive or change the firmware.
> 
> 
> Sure, but for that we need command based filtering.

We have that now (sigh).  See attached patch, which is in BK...

A similar approach could be applied to tape as well.

Though in general I think command-based filtering is not scalable...  at 
the very least I would prefer a list loaded from userspace at boot.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4666 bytes --]

# ChangeSet
#   2004/08/12 17:51:15-07:00 torvalds@ppc970.osdl.org 
#   Allow non-root users certain raw commands if they are deemed safe.
#   
#   We allow more commands if the disk was opened read-write.
# 
diff -Nru a/drivers/block/scsi_ioctl.c b/drivers/block/scsi_ioctl.c
--- a/drivers/block/scsi_ioctl.c	2004-08-13 15:35:28 -04:00
+++ b/drivers/block/scsi_ioctl.c	2004-08-13 15:35:28 -04:00
@@ -105,8 +105,80 @@
 	return put_user(1, p);
 }
 
-static int sg_io(request_queue_t *q, struct gendisk *bd_disk,
-		 struct sg_io_hdr *hdr)
+#define CMD_READ_SAFE	0x01
+#define CMD_WRITE_SAFE	0x02
+#define safe_for_read(cmd)	[cmd] = CMD_READ_SAFE
+#define safe_for_write(cmd)	[cmd] = CMD_WRITE_SAFE
+
+static int verify_command(struct file *file, unsigned char *cmd)
+{
+	static const unsigned char cmd_type[256] = {
+
+		/* Basic read-only commands */
+		safe_for_read(TEST_UNIT_READY),
+		safe_for_read(REQUEST_SENSE),
+		safe_for_read(READ_6),
+		safe_for_read(READ_10),
+		safe_for_read(READ_12),
+		safe_for_read(READ_16),
+		safe_for_read(READ_BUFFER),
+		safe_for_read(READ_LONG),
+		safe_for_read(INQUIRY),
+		safe_for_read(MODE_SENSE),
+		safe_for_read(MODE_SENSE_10),
+		safe_for_read(START_STOP),
+
+		/* Audio CD commands */
+		safe_for_read(GPCMD_PLAY_CD),
+		safe_for_read(GPCMD_PLAY_AUDIO_10),
+		safe_for_read(GPCMD_PLAY_AUDIO_MSF),
+		safe_for_read(GPCMD_PLAY_AUDIO_TI),
+
+		/* CD/DVD data reading */
+		safe_for_read(GPCMD_READ_CD),
+		safe_for_read(GPCMD_READ_CD_MSF),
+		safe_for_read(GPCMD_READ_DISC_INFO),
+		safe_for_read(GPCMD_READ_CDVD_CAPACITY),
+		safe_for_read(GPCMD_READ_DVD_STRUCTURE),
+		safe_for_read(GPCMD_READ_HEADER),
+		safe_for_read(GPCMD_READ_TRACK_RZONE_INFO),
+		safe_for_read(GPCMD_READ_SUBCHANNEL),
+		safe_for_read(GPCMD_READ_TOC_PMA_ATIP),
+		safe_for_read(GPCMD_REPORT_KEY),
+		safe_for_read(GPCMD_SCAN),
+
+		/* Basic writing commands */
+		safe_for_write(WRITE_6),
+		safe_for_write(WRITE_10),
+		safe_for_write(WRITE_VERIFY),
+		safe_for_write(WRITE_12),
+		safe_for_write(WRITE_VERIFY_12),
+		safe_for_write(WRITE_16),
+		safe_for_write(WRITE_BUFFER),
+		safe_for_write(WRITE_LONG),
+	};
+	unsigned char type = cmd_type[cmd[0]];
+
+	/* Anybody who can open the device can do a read-safe command */
+	if (type & CMD_READ_SAFE)
+		return 0;
+
+	/* Write-safe commands just require a writable open.. */
+	if (type & CMD_WRITE_SAFE) {
+		if (file->f_mode & FMODE_WRITE)
+			return 0;
+	}
+
+	/* And root can do any command.. */
+	if (capable(CAP_SYS_RAWIO))
+		return 0;
+
+	/* Otherwise fail it with an "Operation not permitted" */
+	return -EPERM;
+}
+
+static int sg_io(struct file *file, request_queue_t *q,
+		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;
 	int reading, writing;
@@ -115,14 +187,14 @@
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	unsigned char cmd[BLK_MAX_CDB];
 
-	if (!capable(CAP_SYS_RAWIO))
-		return -EPERM;
 	if (hdr->interface_id != 'S')
 		return -EINVAL;
 	if (hdr->cmd_len > BLK_MAX_CDB)
 		return -EINVAL;
 	if (copy_from_user(cmd, hdr->cmdp, hdr->cmd_len))
 		return -EFAULT;
+	if (verify_command(file, cmd))
+		return -EPERM;
 
 	/*
 	 * we'll do that later
@@ -228,15 +300,13 @@
 #define READ_DEFECT_DATA_TIMEOUT	(60 * HZ )
 #define OMAX_SB_LEN 16          /* For backward compatibility */
 
-static int sg_scsi_ioctl(request_queue_t *q, struct gendisk *bd_disk,
-			 Scsi_Ioctl_Command __user *sic)
+static int sg_scsi_ioctl(struct file *file, request_queue_t *q,
+			 struct gendisk *bd_disk, Scsi_Ioctl_Command __user *sic)
 {
 	struct request *rq;
 	int err, in_len, out_len, bytes, opcode, cmdlen;
 	char *buffer = NULL, sense[SCSI_SENSE_BUFFERSIZE];
 
-	if (!capable(CAP_SYS_RAWIO))
-		return -EPERM;
 	/*
 	 * get in an out lengths, verify they don't exceed a page worth of data
 	 */
@@ -273,6 +343,10 @@
 	if (copy_from_user(buffer, sic->data + cmdlen, in_len))
 		goto error;
 
+	err = verify_command(file, rq->cmd);
+	if (err)
+		goto error;
+
 	switch (opcode) {
 		case SEND_DIAGNOSTIC:
 		case FORMAT_UNIT:
@@ -370,7 +444,7 @@
 			err = -EFAULT;
 			if (copy_from_user(&hdr, arg, sizeof(hdr)))
 				break;
-			err = sg_io(q, bd_disk, &hdr);
+			err = sg_io(file, q, bd_disk, &hdr);
 			if (err == -EFAULT)
 				break;
 
@@ -418,7 +492,7 @@
 			hdr.cmdp = ((struct cdrom_generic_command __user*) arg)->cmd;
 			hdr.cmd_len = sizeof(cgc.cmd);
 
-			err = sg_io(q, bd_disk, &hdr);
+			err = sg_io(file, q, bd_disk, &hdr);
 			if (err == -EFAULT)
 				break;
 
@@ -441,7 +515,7 @@
 			if (!arg)
 				break;
 
-			err = sg_scsi_ioctl(q, bd_disk, arg);
+			err = sg_scsi_ioctl(file, q, bd_disk, arg);
 			break;
 		case CDROMCLOSETRAY:
 			close = 1;

Re: SG_IO and security

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Fri, 13 Aug 2004, Jeff Garzik wrote:
> 
>>2B
>>Jens Axboe wrote:
>>
>>>I have no idea how many apps use this ioctl, does anyone have a rough
>>>list?
>>
>>Add a rate-limited "this feature is deprecated" feature and find out...
> 
> 
> Googling for it does show it as being documented and apparently used by a 
> few programs, at least...


Personally I know it's in use, but for some programs it's a fallback if 
SG_IO or a more modern method doesn't work...

Since we have (AFAICS) five interfaces (scsi-send-command, sg v1, sg v2, 
sg v3, and SG_IO) I would rather just go ahead and

static int printed_warning;
if (!printed_warning++)
	printk(KERN_WARNING "SCSI_SEND_COMMAND deprecated\n");

Also, inevitably some of the programs using this will be ancient disk 
tools (some written by disk vendors long gone)...

	Jeff

Re: SG_IO and security

[Florian Weimer]

* Linus Torvalds:

> On Thu, 12 Aug 2004, Linus Torvalds wrote:
>> 
>> Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
>
> Btw, I think the _right_ thing to check is the write access of the file 
> descriptor. If you have write access to a block device, you can delete the 
> data, so you might as well be able to do the raw commands. And that would 
> allow things like "disk" groups etc to work and burn CD's.

But wouldn't the ability to burn CDs imply that a user can also
corrupted the firmware of the drive, unless other countermeasures are
in place?

Re: SG_IO and security

[Kai Makisara]

On Fri, 13 Aug 2004, Jeff Garzik wrote:

> Peter Jones wrote:
> > On Thu, 12 Aug 2004 22:22:36 +0300 (EEST), Kai Makisara
> > <kai.makisara@kolumbus.fi> wrote:
> > 
> >>On Thu, 12 Aug 2004, Linus Torvalds wrote:
> >>
> >>>Let's see now:
> >>>
> >>>      brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
> >>>
> >>>would you put people you don't trust with your disk in the "disk" group?
> >>>
> >>
> >>This protects disks in practice but SG_IO is currently supported by other
> >>devices, at least SCSI tapes. It is reasonable in some organizations to
> >>give r/w access to ordinary users so that they can read/write tapes. I
> >>would be worried if this would enable the users, for instance, to mess up
> >>the mode page contents of the drive or change the firmware.
> > 
> > 
> > Sure, but for that we need command based filtering.
> 
> We have that now (sigh).  See attached patch, which is in BK...
> 

This patch looks OK except that it includes the command WRITE BUFFER. It 
is used to flash the firmware for many devices. Even the SCSI standard 
(draft) specifies that mode 06h is "Download microcode and save". This 
command _should be removed_ from the list of allowed commands.

> A similar approach could be applied to tape as well.
> 
As far as I can read the code, the filtering applies to all devices. 
Except WRITE BUFFER, the commands are acceptable for tapes considering the 
opening mode or undefined for tapes. The undefined commands may cause a 
device with bad firmware to lock the SCSI bus and in this way to cause DoS. 
However, this applies also to commands defined for a type of device but 
not implemented because they are optional.

I am ready to accept this risk of DoS (provided that allowing the filtered 
commands is really useful for somebody) but this is a risk we must 
recognize.

> Though in general I think command-based filtering is not scalable...  at 
> the very least I would prefer a list loaded from userspace at boot.
> 
I think always requiring CAP_RAWIO would be the approach of least 
surprise.

-- 
Kai

Re: SG_IO and security

[Alan Cox]

On Sad, 2004-08-14 at 08:22, Kai Makisara wrote:
> > Though in general I think command-based filtering is not scalable...  at 
> > the very least I would prefer a list loaded from userspace at boot.
> > 
> I think always requiring CAP_RAWIO would be the approach of least 
> surprise.

Sounds like an excuse for Al to get yet another file system into the
kernel so you can edit filter rules 8)

Alan

Re: SG_IO and security

[Bill Davidsen]

Linus Torvalds wrote:
> 
> On Thu, 12 Aug 2004, Jeff Garzik wrote:
> 
>>Linus Torvalds wrote:
>>
>>>On Thu, 12 Aug 2004, Linus Torvalds wrote:
>>>
>>>
>>>>Hmm.. This still allows the old "junk" commands (SCSI_IOCTL_SEND_COMMAND).
>>>
>>>
>>>Btw, I think the _right_ thing to check is the write access of the file 
>>>descriptor. If you have write access to a block device, you can delete the 
>>>data, so you might as well be able to do the raw commands. And that would 
>>>allow things like "disk" groups etc to work and burn CD's.
>>
>>Define raw commands.  I certainly don't want non-root users to be able 
>>to issue FORMAT UNIT on my hard drive.
> 
> 
> Ehh? The same ones you allow to write all over the raw device?
> 
> Let's see now:
> 
> 	brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
> 
> would you put people you don't trust with your disk in the "disk" group?
> 
> Right. If you trust somebody enough that you give him write access to the 
> disk, then you might as well trust him enough to do commands on it. 
> 
> Conversely, if you don't trust him enough to do things like that, you 
> shouldn't give him write access in the first place.
> 
> It's a hell of a lot easier to destroy a disk with
> 
> 	dd if=/dev/zero of=/dev/xxx bs=8k
> 
> than it is to do it with some special malicious command. 
> 
> And yes, there's clearly a difference, but in general I'd say it is the 
> _data_ on the disk that is worth something to you. The disk itself? Do you 
> really fundamentally care?

I will offer two cases which is not wildly improbable. User complains 
the CD burner will {burn faster, burn brand X media, write HD mode} if 
the firmware is upgraded. User has write to burn CDs, decides to flash 
the firmware herself, turns CD into paperweight. Or possibly user tries 
to install CD firmware on a disk drive.

Case two, user is DBA, has write on raw partitions for Oracle, can 
mangle the whole device, and through some stupidity does.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: SG_IO and security

[Bill Davidsen]

Jeff Garzik wrote:
> Peter Jones wrote:
> 
>> On Thu, 12 Aug 2004 22:22:36 +0300 (EEST), Kai Makisara
>> <kai.makisara@kolumbus.fi> wrote:
>>
>>> On Thu, 12 Aug 2004, Linus Torvalds wrote:
>>>
>>>> Let's see now:
>>>>
>>>>      brw-rw----    1 root     disk       3,   0 Jan 30  2003 /dev/hda
>>>>
>>>> would you put people you don't trust with your disk in the "disk" 
>>>> group?
>>>>
>>>
>>> This protects disks in practice but SG_IO is currently supported by 
>>> other
>>> devices, at least SCSI tapes. It is reasonable in some organizations to
>>> give r/w access to ordinary users so that they can read/write tapes. I
>>> would be worried if this would enable the users, for instance, to 
>>> mess up
>>> the mode page contents of the drive or change the firmware.
>>
>>
>>
>> Sure, but for that we need command based filtering.
> 
> 
> We have that now (sigh).  See attached patch, which is in BK...
> 
> A similar approach could be applied to tape as well.
> 
> Though in general I think command-based filtering is not scalable...  at 
> the very least I would prefer a list loaded from userspace at boot.

It would seem that the list is unlikely to change much, since it would 
presumably be limited to the standard SCSI commands, and require RAWIO 
for vendor commands. Do you see any like change I'm missing?

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me