[PATCH] block: avoid false positive warnings on ioctl to partition

[PATCH] block: avoid false positive warnings on ioctl to partition

[Paolo Bonzini]

After a month of reports, the warnings from non-whitelisted ioctls to
a partitions can be classified in three groups.

BLKFLSBUF and BLKROSET are always sent to devices.  Not having them in
the whitelist did not cause any visible harm, but anyway they can and
should be added to the whitelist.

Many unrecognized ioctls are sent to partitions as an attempt to probe for
CD-ROMs, floppies and other kinds of devices.  Like CDROM_GET_CAPABILITY,
they can be blocked safely.

Of the actual SCSI ioctls, only SG_IO was reported in the wild (twice).
udev's scsi_id can issue INQUIRY commands if passed a partition; this
only occurs with custom rules and is strictly speaking invalid, but we
need a transition period so that people can fix their configuration.
zfs-fuse also can issue SYNCHRONIZE CACHE commands, where it should
simply use fdatasync.

Therefore, this patch silently blocks all ioctls except SG_IO, since
all of them turned out to be false positives; in case some escaped, it
should be easily diagnosable or at least bisectable.  The warning text
is separated for root and non-root, and the deprecation period for root
users is set to end a year from now.

Cc: stable@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Note: I will take care of the stable backport as soon as this
	patch or something similar hits Linus's tree.

 block/scsi_ioctl.c |   43 ++++++++++++++++++++++++++++++-------------
 1 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 260fa80..fe86923 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -696,9 +696,6 @@ int scsi_verify_blk_ioctl(struct block_device *bd, unsigned int cmd)
 	if (bd && bd == bd->bd_contains)
 		return 0;
 
-	/* Actually none of these is particularly useful on a partition,
-	 * but they are safe.
-	 */
 	switch (cmd) {
 	case SCSI_IOCTL_GET_IDLUN:
 	case SCSI_IOCTL_GET_BUS_NUMBER:
@@ -710,22 +707,42 @@ int scsi_verify_blk_ioctl(struct block_device *bd, unsigned int cmd)
 	case SG_GET_RESERVED_SIZE:
 	case SG_SET_RESERVED_SIZE:
 	case SG_EMULATED_HOST:
+		/* Actually none of these is particularly useful on a partition,
+		 * but they are safe.
+		 */
 		return 0;
-	case CDROM_GET_CAPABILITY:
-		/* Keep this until we remove the printk below.  udev sends it
-		 * and we do not want to spam dmesg about it.   CD-ROMs do
-		 * not have partitions, so we get here only for disks.
+
+	case BLKROSET:
+	case BLKFLSBUF:
+		/* These are generic block layer ioctls that are nevertheless
+		 * passed down to devices.  They are certainly valid for
+		 * partitions!
+		 */
+		return 0;
+
+	case SG_IO:
+		/* Accept this for non-root users during a one-year transition
+		 * period.
 		 */
-		return -ENOIOCTLCMD;
-	default:
 		break;
+
+	default:
+		/* In particular, rule out all resets and host-specific ioctls.  */
+		return -ENOIOCTLCMD;
 	}
 
-	/* In particular, rule out all resets and host-specific ioctls.  */
-	printk_ratelimited(KERN_WARNING
-			   "%s: sending ioctl %x to a partition!\n", current->comm, cmd);
+	if (capable(CAP_SYS_RAWIO)) {
+		printk_ratelimited(KERN_WARNING
+				   "%s: SG_IO to a partition will be "
+				   "disallowed in January 2013\n",
+				   current->comm);
+		return 0;
+	}
 
-	return capable(CAP_SYS_RAWIO) ? 0 : -ENOIOCTLCMD;
+	printk_ratelimited(KERN_WARNING
+			   "%s: rejecting SG_IO to a partition for non-root user\n",
+			   current->comm);
+	return -ENOIOCTLCMD;
 }
 EXPORT_SYMBOL(scsi_verify_blk_ioctl);
 
-- 
1.7.1

Re: [PATCH] block: avoid false positive warnings on ioctl to partition

[Alan Cox]

> Therefore, this patch silently blocks all ioctls except SG_IO, since
> all of them turned out to be false positives; in case some escaped, it
> should be easily diagnosable or at least bisectable.  The warning text
> is separated for root and non-root, and the deprecation period for root
> users is set to end a year from now.

NAK.

Firstly blocking CAP_SYS_RAWIO access by any means to a partition is
itself nonsense as the process has enough privilege to go poke the
controller I/O registers by hand. That's a gratuitious API breakage.

Secondly SG_IO allows users to read and write blocks outside their partition as
far as I can see from the verify logic. You either need to block it or
smarten up the filter.

What were the SG_IO command blocks that were caught ? It's going to be
pretty trivial to add a filter->partition_ok to some of them if need be.

Anyway it fails both by stopping valid stuff and not stopping insecure and
unsafe stuff.

Alan

Re: [PATCH] block: avoid false positive warnings on ioctl to partition

[Paolo Bonzini]

On 02/13/2012 06:25 PM, Alan Cox wrote:
> Firstly blocking CAP_SYS_RAWIO access by any means to a partition is
> itself nonsense as the process has enough privilege to go poke the
> controller I/O registers by hand.

What if a process has dropped all capabilities except CAP_SYS_RAWIO, and 
has permission to access to /dev/sdb1 and /dev/sda?  If ioctls are 
protected correctly, it will not be able to poke the I/O registers for 
/dev/sdb.  That's pretty much the point of this.

> Secondly SG_IO allows users to read and write blocks outside their partition as
> far as I can see from the verify logic.

Sure, I do not do any attempt to block anything for CAP_SYS_RAWIO, 
including reads/writes outside partitions.  As far as I undertsood Linus 
agreed to block SG_IO completely, except with a grace period during 
which CAP_SYS_RAWIO will still be able to use SG_IO arbitrarily.  During 
this time they'll get warnings but no change in SG_IO behavior.

> What were the SG_IO command blocks that were caught ? It's going to be
> pretty trivial to add a filter->partition_ok to some of them if need be.

INQUIRY and SYNCHRONIZE CACHE.  We certainly do not want to break the 
former right away; but given that the cause was a mistake in udev 
configuration, we can expect people to fix their rule files in 2012. 
But the latter was pointless because they should have just typed 
"apropos synchronize" and used fdatasync.

Sure, we can add something like filter->partition_ok and erect a 
security vulnerability to the status of feature.  It doesn't mean that's 
the right thing to do.

> Anyway it fails both by stopping valid stuff and not stopping insecure and
> unsafe stuff.

It is not "valid", it's at best "not harmful".

Paolo