allow root to modify raw scsi command permissions list

PATCH In review Peter Jones 5 participants 18 messages Sep 14, 2004 – Sep 16, 2004 Block layer

lore

Share & tools

lore

Peter Jones

author

2004-09-14 14:58 UTC #0 lore raw

PATCH 1/1 allow root to modify raw scsi command permissions list

drivers/block/Makefile +1 -1

diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
@@ -13,7 +13,7 @@
 # kblockd threads
 #
 
-obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o
+obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o rcf.o
 
 obj-$(CONFIG_IOSCHED_NOOP)	+= noop-iosched.o
 obj-$(CONFIG_IOSCHED_AS)	+= as-iosched.o

drivers/block/genhd.c +14 -0

diff -urpN linux-2.5-export/drivers/block/genhd.c pjones-2.5-export/drivers/block/genhd.c
@@ -197,6 +197,7 @@ void add_disk(struct gendisk *disk)
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 	register_disk(disk);
 	blk_register_queue(disk);
+	blk_register_filter(disk);
 }
 
 EXPORT_SYMBOL(add_disk);

@@ -204,6 +205,7 @@ EXPORT_SYMBOL(del_gendisk);	/* in partit
 
 void unlink_gendisk(struct gendisk *disk)
 {
+	blk_unregister_filter(disk);
 	blk_unregister_queue(disk);
 	blk_unregister_region(MKDEV(disk->major, disk->first_minor),
 			      disk->minors);

@@ -382,6 +384,13 @@ static ssize_t disk_stats_read(struct ge
 		jiffies_to_msecs(disk_stat_read(disk, io_ticks)),
 		jiffies_to_msecs(disk_stat_read(disk, time_in_queue)));
 }
+
+static ssize_t disk_driver_read(struct gendisk * disk, char *page)
+{
+	return sprintf(page, "%s\n", disk->disk_name);
+
+}
+
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },
 	.show	= disk_dev_read

@@ -402,6 +411,10 @@ static struct disk_attribute disk_attr_s
 	.attr = {.name = "stat", .mode = S_IRUGO },
 	.show	= disk_stats_read
 };
+static struct disk_attribute disk_attribute_driver = {
+	.attr = {.name = "driver", .mode = S_IRUGO },
+	.show	= disk_driver_read
+};
 
 static struct attribute * default_attrs[] = {
 	&disk_attr_dev.attr,

@@ -409,6 +422,7 @@ static struct attribute * default_attrs[
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_driver.attr,
 	NULL,
 };

drivers/block/rcf.c +322 -0

322 lines not on this page · raw message

drivers/block/scsi_ioctl.c +2 ~~-101~~

121 lines not on this page · raw message

include/linux/blkdev.h +2 -0

diff -urpN linux-2.5-export/include/linux/blkdev.h pjones-2.5-export/include/linux/blkdev.h
@@ -512,6 +512,8 @@ struct sec_size {
 	unsigned block_size_bits;
 };
 
+extern int blk_register_filter(struct gendisk *disk);
+extern void blk_unregister_filter(struct gendisk *disk);
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

include/linux/genhd.h +10 -0

diff -urpN linux-2.5-export/include/linux/genhd.h pjones-2.5-export/include/linux/genhd.h
@@ -78,6 +78,15 @@ struct disk_stats {
 	unsigned io_ticks;
 	unsigned time_in_queue;
 };
+
+#define RCF_MAX_NR_CMDS	256
+#define	RCF_CMD_BYTES	(RCF_MAX_NR_CMDS / (sizeof (unsigned long) * 8))
+
+struct rawio_cmd_filter {
+	unsigned long read_ok[RCF_CMD_BYTES];
+	unsigned long write_ok[RCF_CMD_BYTES];
+	struct kobject kobj;
+};
 	
 struct gendisk {
 	int major;			/* major number of driver */

@@ -88,6 +97,7 @@ struct gendisk {
 	struct hd_struct **part;	/* [indexed by minor] */
 	struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct rawio_cmd_filter filter;
 	void *private_data;
 	sector_t capacity;

Marc Ballarin

2004-09-14 22:40 UTC #1 lore raw

On Tue, 14 Sep 2004 10:51:10 -0400
Peter Jones <[email protected]> wrote:

Good to see someone do this. I tried myself but didn't get very far.

Quoted 1 line

diff -urpN linux-2.5-export/drivers/block/genhd.c pjones-2.5-export/drivers/block/genhd.c

...

Quoted 2 lines

};
+static struct disk_attribute disk_attribute_driver = {

This obviously needs to be

+static struct disk_attribute disk_attr_driver = {

otherwise the following part won't work:

drivers/block/genhd.c static struct attribute * default_attrs[ · line 427

Quoted 7 lines

drivers/block/genhd.c +1 -0

@@ -409,6 +422,7 @@ static struct attribute * default_attrs[
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_driver.attr,
 	NULL,
 };

Additionally, the naming "rawio_cmd_filter" and RCF_* seems a bit confusing (after all raw IO usually refers to O_DIRECT file access). "sg_cmd_filter" and SGF_* appear to be a better choice IMHO.

How should changes to the table be made? I suppose something like "echo add 04 > ok_read_commands". But rcf_store doesn't seem to get called. Am I missing something?

Regards, and thanks for this patch

Peter Jones

author

2004-09-15 17:16 UTC #2 lore raw

Quoted 5 lines · Marc Ballarin

Quoted 1 line
+static struct disk_attribute disk_attribute_driver = {
This obviously needs to be
+static struct disk_attribute disk_attr_driver = {

Absolutely right; I had this checked into my bk tree, but no cset made. That makes it not get exported, I guess...

[...]

Quoted 3 lines

Additionally, the naming "rawio_cmd_filter" and RCF_* seems a bit confusing (after all raw IO usually refers to O_DIRECT file access). "sg_cmd_filter" and SGF_* appear to be a better choice IMHO.

I could go either way on this; we use CAP_SYS_RAWIO to govern if root is getting access, but at the same time this really is scsi commands. I wonder Jens thinks, as he's the maintainer here...

Quoted 3 lines

How should changes to the table be made? I suppose something like "echo add 04 > ok_read_commands". But rcf_store doesn't seem to get called. Am I missing something?

"add read 04", "del read 04", "add write 04", ...

rcf_store() gets called as rcf_writecmds_entry.store() and rcf_readcommands_entry.store() . The get called from rcf_attr_store(), which gets called by sysfs as rcf_sysfs_ops.store()

Quoted 1 line

Regards, and thanks for this patch

Quite welcome. Here's an updated version with the attribute struct name fixed:

drivers/block/Makefile +1 -1

diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
@@ -13,7 +13,7 @@
 # kblockd threads
 #
 
-obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o
+obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o rcf.o
 
 obj-$(CONFIG_IOSCHED_NOOP)	+= noop-iosched.o
 obj-$(CONFIG_IOSCHED_AS)	+= as-iosched.o

drivers/block/genhd.c +14 -0

diff -urpN linux-2.5-export/drivers/block/genhd.c pjones-2.5-export/drivers/block/genhd.c
@@ -197,6 +197,7 @@ void add_disk(struct gendisk *disk)
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 	register_disk(disk);
 	blk_register_queue(disk);
+	blk_register_filter(disk);
 }
 
 EXPORT_SYMBOL(add_disk);

@@ -204,6 +205,7 @@ EXPORT_SYMBOL(del_gendisk);	/* in partit
 
 void unlink_gendisk(struct gendisk *disk)
 {
+	blk_unregister_filter(disk);
 	blk_unregister_queue(disk);
 	blk_unregister_region(MKDEV(disk->major, disk->first_minor),
 			      disk->minors);

@@ -382,6 +384,13 @@ static ssize_t disk_stats_read(struct ge
 		jiffies_to_msecs(disk_stat_read(disk, io_ticks)),
 		jiffies_to_msecs(disk_stat_read(disk, time_in_queue)));
 }
+
+static ssize_t disk_driver_read(struct gendisk * disk, char *page)
+{
+	return sprintf(page, "%s\n", disk->disk_name);
+
+}
+
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },
 	.show	= disk_dev_read

@@ -402,6 +411,10 @@ static struct disk_attribute disk_attr_s
 	.attr = {.name = "stat", .mode = S_IRUGO },
 	.show	= disk_stats_read
 };
+static struct disk_attribute disk_attr_driver = {
+	.attr = {.name = "driver", .mode = S_IRUGO },
+	.show	= disk_driver_read
+};
 
 static struct attribute * default_attrs[] = {
 	&disk_attr_dev.attr,

@@ -409,6 +422,7 @@ static struct attribute * default_attrs[
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_driver.attr,
 	NULL,
 };

drivers/block/rcf.c +322 -0

322 lines not on this page · raw message

drivers/block/scsi_ioctl.c +2 ~~-101~~

121 lines not on this page · raw message

include/linux/blkdev.h +2 -0

diff -urpN linux-2.5-export/include/linux/blkdev.h pjones-2.5-export/include/linux/blkdev.h
@@ -513,6 +513,8 @@ struct sec_size {
 	unsigned block_size_bits;
 };
 
+extern int blk_register_filter(struct gendisk *disk);
+extern void blk_unregister_filter(struct gendisk *disk);
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

include/linux/genhd.h +10 -0

diff -urpN linux-2.5-export/include/linux/genhd.h pjones-2.5-export/include/linux/genhd.h
@@ -78,6 +78,15 @@ struct disk_stats {
 	unsigned io_ticks;
 	unsigned time_in_queue;
 };
+
+#define RCF_MAX_NR_CMDS	256
+#define	RCF_CMD_BYTES	(RCF_MAX_NR_CMDS / (sizeof (unsigned long) * 8))
+
+struct rawio_cmd_filter {
+	unsigned long read_ok[RCF_CMD_BYTES];
+	unsigned long write_ok[RCF_CMD_BYTES];
+	struct kobject kobj;
+};
 	
 struct gendisk {
 	int major;			/* major number of driver */

@@ -88,6 +97,7 @@ struct gendisk {
 	struct hd_struct **part;	/* [indexed by minor] */
 	struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct rawio_cmd_filter filter;
 	void *private_data;
 	sector_t capacity;

Signature · 1 line

        Peter

Kai Makisara

2004-09-15 19:14 UTC #3 lore raw

In reply to Peter Jones

drivers/block/Makefile line 14

Quoted 6 lines · Peter Jones

diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
drivers/block/Makefile +0 -0
@@ -13,7 +13,7 @@
 # kblockd threads
 #

[Patch snipped]

Your patch allows updating the filters for each device. This is one aspect of the problem. Another problem is that the command filter appropriate for CD/DVD writers is _forced on all devices_. In your patch this is in the defaults. In the current scsi_ioctl filter it applies to everything.

I have already commented on MODE SELECT. I still think it is dangerous. Looking at the command list reveals a couple of other problems:
- The command GPCMD_READ_DISC_INFO is "safe_for_read". The command 0x51 has also another use: XPWRITE(10), i.e., a write command. Clearly a problem.
- The "safe_for_read" command GPCMD_REPORT_KEY, 0xa4, has aliases CHANGE ALIAS, SET DEVICE IDENTIFIER and SET TARGET PORT.
- The "safe_for_write" command GPCMD_SEND_DVD_STRUCTURE has alias VOLUME SET OUT (raid configuration).

There are other aliases but they return information and don't look too dangerous. The command code is only 8 bits and there probably will be more aliasing in future.

My conclusion is that the filter _necessary_ for burning CD/DVDs is _not safe for all devices_.

How to solve this problem? One idea I had was to add a sysfs-changable attribute accessible to the filter (disk or queue) that would have a few possible states: allow only root, allow filtered, allow all? The default would be to "allow only root". The cdrom registering could set this to "allow filtered". This would allow the current behaviour for CD/DVD drives but would be safe for others. Other devices could be selectively allowed passthrough access if necessary.

This could also be done in your approach. One possibility would be to start with empty filter and call from CD/DVD registering a function that sets the filter you currently have as default. This would be both flexible and safe.

Opinions?

Signature · 1 line

Kai

Peter Jones

author

2004-09-15 19:49 UTC #4 lore raw

Quoted 10 lines · Kai Makisara

drivers/block/Makefile line 14
Quoted 6 lines · Peter Jones
diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
drivers/block/Makefile +0 -0
@@ -13,7 +13,7 @@
 # kblockd threads
 #
[Patch snipped]

Quoted 4 lines

Your patch allows updating the filters for each device. This is one aspect of the problem. Another problem is that the command filter appropriate for CD/DVD writers is _forced on all devices_. In your patch this is in the defaults. In the current scsi_ioctl filter it applies to everything.

My patch leaves the defaults as what are currently in the kernel.

Quoted 1 line

I have already commented on MODE SELECT. I still think it is dangerous.

It's only marked as being allowed if you have write access; /dev/hda usually isn't a+w, last I checked.

I'd actually say that this isn't suitable for CD drives. To do CDDA reads in many cases you need MODE_SELECT. As such, CD drives should have MODE_SELECT in the list of things allowed to read. But the point is that this gives control of the policy to the userland, where it belongs.

Quoted 8 lines

Looking at the command list reveals a couple of other problems:
- The command GPCMD_READ_DISC_INFO is "safe_for_read". The command 0x51 has also another use: XPWRITE(10), i.e., a write command. Clearly a problem.
- The "safe_for_read" command GPCMD_REPORT_KEY, 0xa4, has aliases CHANGE ALIAS, SET DEVICE IDENTIFIER and SET TARGET PORT.
- The "safe_for_write" command GPCMD_SEND_DVD_STRUCTURE has alias VOLUME SET OUT (raid configuration).

My intent with the patch isn't really to take any stance on which commands should and shouldn't be allowed. In fact, it's quite the opposite -- this allows initscripts/hotplug to install an appropriate list of commands for each device.

Quoted 6 lines

There are other aliases but they return information and don't look too dangerous. The command code is only 8 bits and there probably will be more aliasing in future.
My conclusion is that the filter _necessary_ for burning CD/DVDs is _not safe for all devices_.

Sure. The point is that there's no reason the kernel should make that its problem.

Quoted 1 line

How to solve this problem?

Let the distros pick the list.

Quoted 3 lines

One idea I had was to add a sysfs-changable attribute accessible to the filter (disk or queue) that would have a few possible states: allow only root, allow filtered, allow all?

This is basically what my patch gives you. Here's a handy table:

owner       mode  explanation
root.disk   0600  allow only root
root.disk   0400  allow root read commands, and write if CAP_SYS_RAWIO
root.disk   0660  allow root and disk members to do everything
root.disk   0640  allow root everything, disk can do read commands.
root.disk   0644  allow root everything, everybody read commands.
root.disk   0664  allow root and disk everything, all else get read

etc

That's everything except "allow all", but that is easy:

for x in {0,1,2,3,4,5,6,7,8,9,a,b,c,d,e,f}\
{0,1,2,3,4,5,6,7,8,9,a,b,c,d,e,f} ; do
echo "add read $x" > /sys/block/hda/filter/ok_read_commands echo "add write %x" > /sys/block/hda/filter/ok_write_commands
done

Quoted 4 lines

The default would be to "allow only root". The cdrom registering could set this to "allow filtered". This would allow the current behaviour for CD/DVD drives but would be safe for others. Other devices could be selectively allowed passthrough access if necessary.

There's no point in making the cdrom registration have code for this -- just let userland do the policy stuff, like it should.

Quoted 4 lines

This could also be done in your approach. One possibility would be to start with empty filter and call from CD/DVD registering a function that sets the filter you currently have as default. This would be both flexible and safe.

I don't want to do an empty filter, because that'll mean existing users of SG_IO and such break for no real reason. The approach my patch takes is not to change *any* of the existing policy, but to enable distros to change it to their hearts' content. It's *dead* simple to make initscripts and hotplug pick "correct" lists for each device.

Signature · 1 line

        Peter

Kai Makisara

2004-09-15 20:51 UTC #5 lore raw

Quoted 1 line · Peter Jones

On Wed, 2004-09-15 at 22:14 +0300, Kai Makisara wrote:

...

Quoted 2 lines

My patch leaves the defaults as what are currently in the kernel.

Yes but what I wanted to say the filter currently in the kernel is not safe for all devices.

Quoted 5 lines

Quoted 1 line
I have already commented on MODE SELECT. I still think it is dangerous.
It's only marked as being allowed if you have write access; /dev/hda usually isn't a+w, last I checked.

I am thinking about other devices than disks. Tapes are what I know best but there are also other device types (all that support SG_IO).

For tapes it is fairly common to allow users read/write permissions for reading/writing tapes. MODE SELECT allows a user to mess up the drive parameters so that the next user thinks it is broken. This is not the purpose of giving read/write permissions in this case.

Quoted 6 lines

I'd actually say that this isn't suitable for CD drives. To do CDDA reads in many cases you need MODE_SELECT. As such, CD drives should have MODE_SELECT in the list of things allowed to read. But the point is that this gives control of the policy to the userland, where it belongs.

This is why I like your approach.

...

Quoted 5 lines

My intent with the patch isn't really to take any stance on which commands should and shouldn't be allowed. In fact, it's quite the opposite -- this allows initscripts/hotplug to install an appropriate list of commands for each device.

If you are not taking a stance, then the default list should be empty. The starting point must be safe and it can be relaxed. I am just trying to have a safe enough starting point.

In an ideal world the initscripts/hotplug scripts would be updated instantaneously and the empty default would not be a problem. In the real world this does not happen and we have decide how far we want to relax the default security. I think this is why Linus added the filter: he wanted to allow users to burn CDs without special permissions. This is why I suggested that the default filter should allow the current default for CDs. It is a compromise.

...

Quoted 11 lines

Quoted 4 lines
This could also be done in your approach. One possibility would be to start with empty filter and call from CD/DVD registering a function that sets the filter you currently have as default. This would be both flexible and safe.
I don't want to do an empty filter, because that'll mean existing users of SG_IO and such break for no real reason. The approach my patch takes is not to change *any* of the existing policy, but to enable distros to change it to their hearts' content. It's *dead* simple to make initscripts and hotplug pick "correct" lists for each device.

I don't know any other SG_IO users than CD/DVD software that try to use passthrough without CAP_RAWIO. This is probably politically incorrect, but no sane software developer expects to be able to send raw commands without proper permissions. If this kind of security hole is present, it will be plugged sooner or later.

Signature · 1 line

Kai

Jens Axboe

2004-09-15 19:50 UTC #6 lore raw

In reply to Kai Makisara

Quoted 40 lines · Kai Makisara

drivers/block/Makefile line 14
Quoted 6 lines · Peter Jones
diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
drivers/block/Makefile +0 -0
@@ -13,7 +13,7 @@
 # kblockd threads
 #
[Patch snipped]
Your patch allows updating the filters for each device. This is one aspect of the problem. Another problem is that the command filter appropriate for CD/DVD writers is _forced on all devices_. In your patch this is in the defaults. In the current scsi_ioctl filter it applies to everything.
I have already commented on MODE SELECT. I still think it is dangerous. Looking at the command list reveals a couple of other problems:
- The command GPCMD_READ_DISC_INFO is "safe_for_read". The command 0x51 has also another use: XPWRITE(10), i.e., a write command. Clearly a problem.
- The "safe_for_read" command GPCMD_REPORT_KEY, 0xa4, has aliases CHANGE ALIAS, SET DEVICE IDENTIFIER and SET TARGET PORT.
- The "safe_for_write" command GPCMD_SEND_DVD_STRUCTURE has alias VOLUME SET OUT (raid configuration).
There are other aliases but they return information and don't look too dangerous. The command code is only 8 bits and there probably will be more aliasing in future.
My conclusion is that the filter _necessary_ for burning CD/DVDs is _not safe for all devices_.
How to solve this problem? One idea I had was to add a sysfs-changable attribute accessible to the filter (disk or queue) that would have a few possible states: allow only root, allow filtered, allow all? The default would be to "allow only root". The cdrom registering could set this to "allow filtered". This would allow the current behaviour for CD/DVD drives but would be safe for others. Other devices could be selectively allowed passthrough access if necessary.

Quoted 4 lines

This could also be done in your approach. One possibility would be to start with empty filter and call from CD/DVD registering a function that sets the filter you currently have as default. This would be both flexible and safe.

I think that's a really nice idea. I've said right from the beginning that the command tables cannot even be per-device type, at least with CDROMs we have examples of commands with different meanings. But at least with a device-type default list we're a little closer.

A good reminder of why the whole thing is a mess :-)

Signature · 1 line

Jens Axboe

Peter Jones

author

2004-09-15 20:34 UTC #7 lore raw

Quoted 9 lines · Jens Axboe

Quoted 4 lines
This could also be done in your approach. One possibility would be to start with empty filter and call from CD/DVD registering a function that sets the filter you currently have as default. This would be both flexible and safe.
I think that's a really nice idea. I've said right from the beginning that the command tables cannot even be per-device type, at least with CDROMs we have examples of commands with different meanings. But at least with a device-type default list we're a little closer.

It'd be pretty easy to add device-type defaults to my patch, and make them registered by the individual drivers.

I'm just not sure I see the point in doing it. Without them, you get a _somewhat_ reasonable set of defaults if you don't want to mess with it, and distros can easily set their own per-device tables for each device type, vendor specific commands, etc. It's even pretty simple to just deny everything, if that's what distro maintainers or sysadmins want to do.

Not doing it also means that the device driver code itself needs less maintenance, and won't need to be updated every time some vendor comes up with a new READ command. That makes it easier if for example distros decide to push new command tables as updates, I think.

Quoted 1 line

A good reminder of why the whole thing is a mess :-)

It sure is.

Signature · 1 line

        Peter

Marc Ballarin

2004-09-16 09:23 UTC #8 lore raw

Quoted 2 lines · On Wed, 15 Sep 2004 16:32:24 -0400 Peter Jones <[email protected]> wrote:

It'd be pretty easy to add device-type defaults to my patch, and make them registered by the individual drivers.

Even this won't get us very far. Maybe we could probably find suitable defaults for most modern hardware, but this still won't help users of older or non-standard hardware.
For example, remember LG's abuse of CACHE_FLUSH for firmware updates.

Here I've done some grepping over cdrecord and k3b:
http://marc.theaimsgroup.com/?l=linux-kernel&m=109269018602502&w=2

Quoted 7 lines

I'm just not sure I see the point in doing it. Without them, you get a _somewhat_ reasonable set of defaults if you don't want to mess with it, and distros can easily set their own per-device tables for each device type, vendor specific commands, etc. It's even pretty simple to just deny everything, if that's what distro maintainers or sysadmins want to do.

Per device-type filters are unavoidable (unless we deny everything by default).
This is especially true for MODE_SELECT. See here:
http://marc.theaimsgroup.com/?l=linux-kernel&m=109274414601613&w=2

Quoted 5 lines

Not doing it also means that the device driver code itself needs less maintenance, and won't need to be updated every time some vendor comes up with a new READ command. That makes it easier if for example distros decide to push new command tables as updates, I think.

Why not deny everything and have a "smart" user space tool that assigns filters to devices during startup? The tool's database could be updated regularly by distributors.

Quoted 4 lines

Quoted 2 lines
A good reminder of why the whole thing is a mess :-)
It sure is.

IMHO all the more reason to keep all knowledge about individual devices (and thus the need to provide updates) out of the kernel.

BTW: My impression was, that in the long term applications should be adopted to work safely with increased capabilities. Is this still the goal?

mfg

Marc Ballarin

2004-09-15 21:01 UTC #9 lore raw

In reply to Peter Jones

Hi,
this is a modified version of the previous patch.

Changes:
- always default to an empty filter table (see comment)
- the loading mechanism is changed. It is now only possible to change the complete list at once. What you write into the file is what you see when doing 'cat'.
- Adding or removing single commands is no longer possible. IMHO this task (and further fanciness) is best left to userspace tools.
- instead of defining which list is changed through a keyword, the file decides. Writes to ok_read_commands only change the commands safe for read. The same is true for ok_write_commands and commands allowed for write.

IMHO those changes make usage more intuitive and easier for tools/scripts.

TODO:
- an rcf_debug feature is needed. Admins need a way to find out which commands are actually needed. (echo 0/1 > filter/rcf_debug ?)
- the new sysfs interface is probably horribly written / unreliable. Someone with actual experience in C and kernel programming needs to review this.
- is it possible to avoid the big code duplication in rcf_store_read and rcf_store_write?

Regards,
Marc

drivers/block/genhd.c renamed from linux-2.6.8.1/drivers/block/genhd.c +14 -0

diff -Naur orig/linux-2.6.8.1/drivers/block/genhd.c linux-2.6.8.1/drivers/block/genhd.c
@@ -197,6 +197,7 @@
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 	register_disk(disk);
 	blk_register_queue(disk);
+	blk_register_filter(disk);
 }
 
 EXPORT_SYMBOL(add_disk);

@@ -204,6 +205,7 @@
 
 void unlink_gendisk(struct gendisk *disk)
 {
+	blk_unregister_filter(disk);
 	blk_unregister_queue(disk);
 	blk_unregister_region(MKDEV(disk->major, disk->first_minor),
 			      disk->minors);

@@ -382,6 +384,13 @@
 		jiffies_to_msecs(disk_stat_read(disk, io_ticks)),
 		jiffies_to_msecs(disk_stat_read(disk, time_in_queue)));
 }
+
+static ssize_t disk_driver_read(struct gendisk * disk, char *page)
+{
+	return sprintf(page, "%s\n", disk->disk_name);
+
+}
+
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },
 	.show	= disk_dev_read

@@ -402,6 +411,10 @@
 	.attr = {.name = "stat", .mode = S_IRUGO },
 	.show	= disk_stats_read
 };
+static struct disk_attribute disk_attr_driver = {
+	.attr = {.name = "driver", .mode = S_IRUGO },
+	.show	= disk_driver_read
+};
 
 static struct attribute * default_attrs[] = {
 	&disk_attr_dev.attr,

@@ -409,6 +422,7 @@
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_driver.attr,
 	NULL,
 };

drivers/block/Makefile renamed from linux-2.6.8.1/drivers/block/Makefile +1 -1

diff -Naur orig/linux-2.6.8.1/drivers/block/Makefile linux-2.6.8.1/drivers/block/Makefile
@@ -13,7 +13,7 @@
 # kblockd threads
 #
 
-obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o
+obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o rcf.o
 
 obj-$(CONFIG_IOSCHED_NOOP)	+= noop-iosched.o
 obj-$(CONFIG_IOSCHED_AS)	+= as-iosched.o

drivers/block/rcf.c renamed from linux-2.6.8.1/drivers/block/rcf.c +258 -0

258 lines not on this page · raw message

drivers/block/scsi_ioctl.c renamed from linux-2.6.8.1/drivers/block/scsi_ioctl.c +2 ~~-101~~

121 lines not on this page · raw message

include/linux/blkdev.h renamed from linux-2.6.8.1/include/linux/blkdev.h +2 -0

diff -Naur orig/linux-2.6.8.1/include/linux/blkdev.h linux-2.6.8.1/include/linux/blkdev.h
@@ -512,6 +512,8 @@
 	unsigned block_size_bits;
 };
 
+extern int blk_register_filter(struct gendisk *disk);
+extern void blk_unregister_filter(struct gendisk *disk);
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

include/linux/genhd.h renamed from linux-2.6.8.1/include/linux/genhd.h +10 -0

diff -Naur orig/linux-2.6.8.1/include/linux/genhd.h linux-2.6.8.1/include/linux/genhd.h
@@ -78,6 +78,15 @@
 	unsigned io_ticks;
 	unsigned time_in_queue;
 };
+
+#define RCF_MAX_NR_CMDS	256
+#define	RCF_CMD_BYTES	(RCF_MAX_NR_CMDS / (sizeof (unsigned long) * 8))
+
+struct rawio_cmd_filter {
+	unsigned long read_ok[RCF_CMD_BYTES];
+	unsigned long write_ok[RCF_CMD_BYTES];
+	struct kobject kobj;
+};
 	
 struct gendisk {
 	int major;			/* major number of driver */

@@ -88,6 +97,7 @@
 	struct hd_struct **part;	/* [indexed by minor] */
 	struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct rawio_cmd_filter filter;
 	void *private_data;
 	sector_t capacity;

Alan Cox

2004-09-15 22:44 UTC #10 lore raw

Quoted 2 lines · Marc Ballarin

Hi,
this is a modified version of the previous patch.

You need to check for capable(CAP_SYS_RAWIO) otherwise you elevate anyone with access bypass capabilities to CAP_SYS_RAWIO equivalent powers.

Peter Jones

author

2004-09-15 23:05 UTC #11 lore raw

Quoted 7 lines · Alan Cox

Quoted 2 lines · Marc Ballarin
Hi,
this is a modified version of the previous patch.
You need to check for capable(CAP_SYS_RAWIO) otherwise you elevate anyone with access bypass capabilities to CAP_SYS_RAWIO equivalent powers.

Do you mean in the ->store path?

Signature · 1 line

        Peter

Alan Cox

2004-09-15 23:33 UTC #12 lore raw

Quoted 5 lines · Peter Jones

Quoted 3 lines
You need to check for capable(CAP_SYS_RAWIO) otherwise you elevate anyone with access bypass capabilities to CAP_SYS_RAWIO equivalent powers.
Do you mean in the ->store path?

Thats the one - otherwise you can add commands without the ability you'd normally have to bypass

Marc Ballarin

2004-09-15 23:28 UTC #13 lore raw

In reply to Alan Cox

Quoted 3 lines · On Wed, 15 Sep 2004 22:38:47 +0100 Alan Cox <[email protected]> wrote:

You need to check for capable(CAP_SYS_RAWIO) otherwise you elevate anyone with access bypass capabilities to CAP_SYS_RAWIO equivalent powers.

True. File permissions aren't enough.

Will something like this suffice?

static ssize_t rcf_store_write(struct rawio_cmd_filter *rcf, const char *page,
size_t count)
{

	...	
	
	if (!capable(CAP_SYS_RAWIO))
		return -EPERM;
	
	while(i < RCF_MAX_NR_CMDS)
		clear_bit(i++, rcf->write_ok);
	...

Regards

Peter Jones

author

2004-09-15 23:42 UTC #14 lore raw

Quoted 10 lines · Marc Ballarin

Quoted 3 lines · On Wed, 15 Sep 2004 22:38:47 +0100 Alan Cox <[email protected]> wrote:
You need to check for capable(CAP_SYS_RAWIO) otherwise you elevate anyone with access bypass capabilities to CAP_SYS_RAWIO equivalent powers.
True. File permissions aren't enough.
Will something like this suffice?

[...]

Yes.

Here's another version of the patch. It does what yours did, as well as that. It also combines the read and write code so there's one of each, with a parameter to tell which it's supposed to modify.

rcf_cmds_store() might need some locking - I'm not sure, and haven't investigated yet.

The default commands are still there; I haven't had time to make them a config option yet, and the jury still seems out on how defaults should work, anyway.

I haven't booted with this one, but I have compiled it...

drivers/block/Makefile +1 -1

diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
@@ -13,7 +13,7 @@
 # kblockd threads
 #
 
-obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o
+obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o rcf.o
 
 obj-$(CONFIG_IOSCHED_NOOP)	+= noop-iosched.o
 obj-$(CONFIG_IOSCHED_AS)	+= as-iosched.o

drivers/block/genhd.c +14 -0

diff -urpN linux-2.5-export/drivers/block/genhd.c pjones-2.5-export/drivers/block/genhd.c
@@ -197,6 +197,7 @@ void add_disk(struct gendisk *disk)
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 	register_disk(disk);
 	blk_register_queue(disk);
+	blk_register_filter(disk);
 }
 
 EXPORT_SYMBOL(add_disk);

@@ -204,6 +205,7 @@ EXPORT_SYMBOL(del_gendisk);	/* in partit
 
 void unlink_gendisk(struct gendisk *disk)
 {
+	blk_unregister_filter(disk);
 	blk_unregister_queue(disk);
 	blk_unregister_region(MKDEV(disk->major, disk->first_minor),
 			      disk->minors);

@@ -382,6 +384,13 @@ static ssize_t disk_stats_read(struct ge
 		jiffies_to_msecs(disk_stat_read(disk, io_ticks)),
 		jiffies_to_msecs(disk_stat_read(disk, time_in_queue)));
 }
+
+static ssize_t disk_driver_read(struct gendisk * disk, char *page)
+{
+	return sprintf(page, "%s\n", disk->disk_name);
+
+}
+
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },
 	.show	= disk_dev_read

@@ -402,6 +411,10 @@ static struct disk_attribute disk_attr_s
 	.attr = {.name = "stat", .mode = S_IRUGO },
 	.show	= disk_stats_read
 };
+static struct disk_attribute disk_attr_driver = {
+	.attr = {.name = "driver", .mode = S_IRUGO },
+	.show	= disk_driver_read
+};
 
 static struct attribute * default_attrs[] = {
 	&disk_attr_dev.attr,

@@ -409,6 +422,7 @@ static struct attribute * default_attrs[
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_driver.attr,
 	NULL,
 };

drivers/block/rcf.c +304 -0

304 lines not on this page · raw message

drivers/block/scsi_ioctl.c +2 ~~-101~~

121 lines not on this page · raw message

include/linux/blkdev.h +2 -0

diff -urpN linux-2.5-export/include/linux/blkdev.h pjones-2.5-export/include/linux/blkdev.h
@@ -513,6 +513,8 @@ struct sec_size {
 	unsigned block_size_bits;
 };
 
+extern int blk_register_filter(struct gendisk *disk);
+extern void blk_unregister_filter(struct gendisk *disk);
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

include/linux/genhd.h +10 -0

diff -urpN linux-2.5-export/include/linux/genhd.h pjones-2.5-export/include/linux/genhd.h
@@ -78,6 +78,15 @@ struct disk_stats {
 	unsigned io_ticks;
 	unsigned time_in_queue;
 };
+
+#define RCF_MAX_NR_CMDS	256
+#define	RCF_CMD_LONGS	(RCF_MAX_NR_CMDS / (sizeof (unsigned long) * 8))
+
+struct rawio_cmd_filter {
+	unsigned long read_ok[RCF_CMD_LONGS];
+	unsigned long write_ok[RCF_CMD_LONGS];
+	struct kobject kobj;
+};
 	
 struct gendisk {
 	int major;			/* major number of driver */

@@ -88,6 +97,7 @@ struct gendisk {
 	struct hd_struct **part;	/* [indexed by minor] */
 	struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct rawio_cmd_filter filter;
 	void *private_data;
 	sector_t capacity;

Signature · 1 line

        Peter

Marc Ballarin

2004-09-16 12:07 UTC #15 lore raw

Quoted 5 lines · On Wed, 15 Sep 2004 19:36:18 -0400 Peter Jones <[email protected]> wrote:

Yes.
Here's another version of the patch. It does what yours did, as well as that. It also combines the read and write code so there's one of each, with a parameter to tell which it's supposed to modify.

Looks *slightly* better than mine ;-)

But

drivers/block/rcf.c line 95

Quoted 2 lines

drivers/block/rcf.c +2 -0

+static ssize_t rcf_cmds_store(struct rawio_cmd_filter *rcf, const char *page,
+			size_t count, int rw)

...

drivers/block/rcf.c line 108

Quoted 2 lines

drivers/block/rcf.c +2 -0

+		ss.from = (char *)page+ret;
+		ss.to = (char *)page+ret+1;

needs to be

Review

Quoted 2 lines

(quoted diff) +2 -0

+		ss.from = (char *)page+ret;
+		ss.to = (char *)page+ret+2;

Regards

Marc Ballarin

2004-09-16 14:57 UTC #16 lore raw

In reply to Peter Jones

Hi,
I've created another version. I fixed the +1 -> +2 issue and added a debbuging function.
I think this is needed because nowadays hardware hardly ships with in-depth documentation. It greatly helps in determinig needed filter rules.

Debugging is enabled with "echo 1 > /sys/block/hdX/filter/rcf_debug". If a command is blocked, messages like the following are given via printk:
RCF: hdX: command XY not allowed in write-mode
RCF: hdX: command YZ not allowed in read-mode

BTW: There is still a compiler warning about implicit declaration of rcf_verify_command when building scsi_ioctl.c

Regards

drivers/block/genhd.c renamed from linux-2.6.8.1/drivers/block/genhd.c +14 -0

diff -Nurp orig/linux-2.6.8.1/drivers/block/genhd.c linux-2.6.8.1/drivers/block/genhd.c
@@ -197,6 +197,7 @@ void add_disk(struct gendisk *disk)
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 	register_disk(disk);
 	blk_register_queue(disk);
+	blk_register_filter(disk);
 }
 
 EXPORT_SYMBOL(add_disk);

@@ -204,6 +205,7 @@ EXPORT_SYMBOL(del_gendisk);	/* in partit
 
 void unlink_gendisk(struct gendisk *disk)
 {
+	blk_unregister_filter(disk);
 	blk_unregister_queue(disk);
 	blk_unregister_region(MKDEV(disk->major, disk->first_minor),
 			      disk->minors);

@@ -382,6 +384,13 @@ static ssize_t disk_stats_read(struct ge
 		jiffies_to_msecs(disk_stat_read(disk, io_ticks)),
 		jiffies_to_msecs(disk_stat_read(disk, time_in_queue)));
 }
+
+static ssize_t disk_driver_read(struct gendisk * disk, char *page)
+{
+	return sprintf(page, "%s\n", disk->disk_name);
+
+}
+
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },
 	.show	= disk_dev_read

@@ -402,6 +411,10 @@ static struct disk_attribute disk_attr_s
 	.attr = {.name = "stat", .mode = S_IRUGO },
 	.show	= disk_stats_read
 };
+static struct disk_attribute disk_attr_driver = {
+	.attr = {.name = "driver", .mode = S_IRUGO },
+	.show	= disk_driver_read
+};
 
 static struct attribute * default_attrs[] = {
 	&disk_attr_dev.attr,

@@ -409,6 +422,7 @@ static struct attribute * default_attrs[
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_driver.attr,
 	NULL,
 };

drivers/block/Makefile renamed from linux-2.6.8.1/drivers/block/Makefile +1 -1

diff -Nurp orig/linux-2.6.8.1/drivers/block/Makefile linux-2.6.8.1/drivers/block/Makefile
@@ -13,7 +13,7 @@
 # kblockd threads
 #
 
-obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o
+obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o rcf.o
 
 obj-$(CONFIG_IOSCHED_NOOP)	+= noop-iosched.o
 obj-$(CONFIG_IOSCHED_AS)	+= as-iosched.o

drivers/block/rcf.c renamed from linux-2.6.8.1/drivers/block/rcf.c +341 -0

341 lines not on this page · raw message

drivers/block/scsi_ioctl.c renamed from linux-2.6.8.1/drivers/block/scsi_ioctl.c +2 ~~-101~~

121 lines not on this page · raw message

include/linux/blkdev.h renamed from linux-2.6.8.1/include/linux/blkdev.h +2 -0

diff -Nurp orig/linux-2.6.8.1/include/linux/blkdev.h linux-2.6.8.1/include/linux/blkdev.h
@@ -512,6 +512,8 @@ struct sec_size {
 	unsigned block_size_bits;
 };
 
+extern int blk_register_filter(struct gendisk *disk);
+extern void blk_unregister_filter(struct gendisk *disk);
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

include/linux/genhd.h renamed from linux-2.6.8.1/include/linux/genhd.h +11 -0

diff -Nurp orig/linux-2.6.8.1/include/linux/genhd.h linux-2.6.8.1/include/linux/genhd.h
@@ -78,6 +78,16 @@ struct disk_stats {
 	unsigned io_ticks;
 	unsigned time_in_queue;
 };
+
+#define RCF_MAX_NR_CMDS	256
+#define	RCF_CMD_LONGS	(RCF_MAX_NR_CMDS / (sizeof (unsigned long) * 8))
+
+struct rawio_cmd_filter {
+	unsigned long read_ok[RCF_CMD_LONGS];
+	unsigned long write_ok[RCF_CMD_LONGS];
+	int debug_mode;
+	struct kobject kobj;
+};
 	
 struct gendisk {
 	int major;			/* major number of driver */

@@ -88,6 +98,7 @@ struct gendisk {
 	struct hd_struct **part;	/* [indexed by minor] */
 	struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct rawio_cmd_filter filter;
 	void *private_data;
 	sector_t capacity;

Peter Jones

author

2004-09-16 17:45 UTC #17 lore raw

Quoted 7 lines · Marc Ballarin

Hi,
I've created another version. I fixed the +1 -> +2 issue and added a debbuging function.
I think this is needed because nowadays hardware hardly ships with in-depth documentation. It greatly helps in determinig needed filter rules.

Quoted 8 lines

Debugging is enabled with "echo 1 > /sys/block/hdX/filter/rcf_debug". If a command is blocked, messages like the following are given via printk:
RCF: hdX: command XY not allowed in write-mode
RCF: hdX: command YZ not allowed in read-mode
BTW: There is still a compiler warning about implicit declaration of rcf_verify_command when building scsi_ioctl.c

Oh, hrm. Yeah, that needs to be declared in the header. Thanks for pointing it out.

Here's a new diff. This has changes based on your debug changes. It also removes the (useless) "driver" sysfs entry, as it doesn't show what I thought. It'd still be nice to list what type of device this is somewhere in /sys/block/*/ .

drivers/block/Makefile +1 -1

diff -urpN linux-2.5-export/drivers/block/Makefile pjones-2.5-export/drivers/block/Makefile
@@ -13,7 +13,7 @@
 # kblockd threads
 #
 
-obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o
+obj-y	:= elevator.o ll_rw_blk.o ioctl.o genhd.o scsi_ioctl.o rcf.o
 
 obj-$(CONFIG_IOSCHED_NOOP)	+= noop-iosched.o
 obj-$(CONFIG_IOSCHED_AS)	+= as-iosched.o

drivers/block/genhd.c +3 -0

diff -urpN linux-2.5-export/drivers/block/genhd.c pjones-2.5-export/drivers/block/genhd.c
@@ -197,6 +197,7 @@ void add_disk(struct gendisk *disk)
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 	register_disk(disk);
 	blk_register_queue(disk);
+	blk_register_filter(disk);
 }
 
 EXPORT_SYMBOL(add_disk);

@@ -204,6 +205,7 @@ EXPORT_SYMBOL(del_gendisk);	/* in partit
 
 void unlink_gendisk(struct gendisk *disk)
 {
+	blk_unregister_filter(disk);
 	blk_unregister_queue(disk);
 	blk_unregister_region(MKDEV(disk->major, disk->first_minor),
 			      disk->minors);

@@ -382,6 +384,7 @@ static ssize_t disk_stats_read(struct ge
 		jiffies_to_msecs(disk_stat_read(disk, io_ticks)),
 		jiffies_to_msecs(disk_stat_read(disk, time_in_queue)));
 }
+
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },
 	.show	= disk_dev_read

drivers/block/rcf.c +338 -0

338 lines not on this page · raw message

drivers/block/scsi_ioctl.c +2 ~~-101~~

121 lines not on this page · raw message

include/linux/blkdev.h +3 -0

diff -urpN linux-2.5-export/include/linux/blkdev.h pjones-2.5-export/include/linux/blkdev.h
@@ -513,6 +513,9 @@ struct sec_size {
 	unsigned block_size_bits;
 };
 
+extern int rcf_verify_command(struct file *file, unsigned char *cmd);
+extern int blk_register_filter(struct gendisk *disk);
+extern void blk_unregister_filter(struct gendisk *disk);
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

include/linux/genhd.h +11 -0

diff -urpN linux-2.5-export/include/linux/genhd.h pjones-2.5-export/include/linux/genhd.h
@@ -78,6 +78,16 @@ struct disk_stats {
 	unsigned io_ticks;
 	unsigned time_in_queue;
 };
+
+#define RCF_MAX_NR_CMDS	256
+#define	RCF_CMD_LONGS	(RCF_MAX_NR_CMDS / (sizeof (unsigned long) * 8))
+
+struct rawio_cmd_filter {
+	unsigned long read_ok[RCF_CMD_LONGS];
+	unsigned long write_ok[RCF_CMD_LONGS];
+	int debug;
+	struct kobject kobj;
+};
 	
 struct gendisk {
 	int major;			/* major number of driver */

@@ -88,6 +98,7 @@ struct gendisk {
 	struct hd_struct **part;	/* [indexed by minor] */
 	struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct rawio_cmd_filter filter;
 	void *private_data;
 	sector_t capacity;

Signature · 4 lines

        Peter

"If the facts don't fit the theory, change the facts."
                -- Einstein