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

[PATCH v2] 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.  The warning for SG_IO is left in
because a) it will alert users to possible bugs, and b) we do want to
hear about more uses in the wild.  However, no deprecation period is
set for now.

Cc: stable@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
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 |   40 ++++++++++++++++++++++++++++++-------------
 1 files changed, 27 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,39 @@ 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.
-		 */
-		return -ENOIOCTLCMD;
+
+	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 root users, at least for now.  */
+		break;
+
 	default:
-		break;
+		/* In particular, rule out 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 is likely a bug\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 v2] block: avoid false positive warnings on ioctl to partition

[Paolo Bonzini]

Ping.

On 02/17/2012 08:38 AM, Paolo Bonzini wrote:
> 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.  The warning for SG_IO is left in
> because a) it will alert users to possible bugs, and b) we do want to
> hear about more uses in the wild.  However, no deprecation period is
> set for now.
> 
> Cc: stable@vger.kernel.org
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> 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 |   40 ++++++++++++++++++++++++++++++-------------
>  1 files changed, 27 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,39 @@ 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.
> -		 */
> -		return -ENOIOCTLCMD;
> +
> +	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 root users, at least for now.  */
> +		break;
> +
>  	default:
> -		break;
> +		/* In particular, rule out 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 is likely a bug\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);
>  

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

[Linus Torvalds]

On Mon, Feb 27, 2012 at 4:36 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Ping.

So I'm still not convinced this is safe, and feel a bit worried about
us possibly silently missing some things. That

   default:
      return -ENOIOCTLCMD;

is what worries me.

Blocking the ones we *know* about and understand I'm perfectly fine
with. And the SG_IO case looks fine. It's the possibly unknown users
that still worry me.

                  Linus

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

[Paolo Bonzini]

Il 29/02/2012 01:14, Linus Torvalds ha scritto:
> So I'm still not convinced this is safe, and feel a bit worried about
> us possibly silently missing some things. That
> 
>    default:
>       return -ENOIOCTLCMD;
> 
> is what worries me.
> 
> Blocking the ones we *know* about and understand I'm perfectly fine
> with. And the SG_IO case looks fine. It's the possibly unknown users
> that still worry me.

I understand.

We do have a good grasp of what's happening.  We did get reports for
SG_IO, for false positives that would have returned -ENOTTY, and for
ioctls that need to be passed.  We couldn't expect anything better than
this, I think.

I checked in the source and all scsi_host-specific ioctls need
filtering.  Of course we might be missing something really obscure which
is rarely used in the wild.  But being 100% sure that nothing breaks is
impossible, unfortunately, so does it make sense to aim at 100%?  And it
should be extremely easy to bisect failures.  Even with all the
differences, it reminds me of the recent change to poll.

Paolo

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

[Ray Lee]

On Wed, Feb 29, 2012 at 12:13 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Il 29/02/2012 01:14, Linus Torvalds ha scritto:
> > So I'm still not convinced this is safe, and feel a bit worried about
> > us possibly silently missing some things. That
> >
> >    default:
> >       return -ENOIOCTLCMD;
> >
> > is what worries me.
> >
> > Blocking the ones we *know* about and understand I'm perfectly fine
> > with. And the SG_IO case looks fine. It's the possibly unknown users
> > that still worry me.
>
> I understand.
>
> But being 100% sure that nothing breaks is
> impossible, unfortunately, so does it make sense to aim at 100%?  And it
> should be extremely easy to bisect failures.  Even with all the
> differences, it reminds me of the recent change to poll.

You can help avoid the bisect entirely by silently dropping the ones
you're sure about, and noisily dropping the ones you aren't.

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

[Paolo Bonzini]

Il 29/02/2012 20:56, Ray Lee ha scritto:
> On Wed, Feb 29, 2012 at 12:13 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Il 29/02/2012 01:14, Linus Torvalds ha scritto:
>>> So I'm still not convinced this is safe, and feel a bit worried about
>>> us possibly silently missing some things. That
>>>
>>>    default:
>>>       return -ENOIOCTLCMD;
>>>
>>> is what worries me.
>>>
>>> Blocking the ones we *know* about and understand I'm perfectly fine
>>> with. And the SG_IO case looks fine. It's the possibly unknown users
>>> that still worry me.
>>
>> I understand.
>>
>> But being 100% sure that nothing breaks is
>> impossible, unfortunately, so does it make sense to aim at 100%?  And it
>> should be extremely easy to bisect failures.  Even with all the
>> differences, it reminds me of the recent change to poll.
> 
> You can help avoid the bisect entirely by silently dropping the ones
> you're sure about, and noisily dropping the ones you aren't.

It's not so easy, see http://permalink.gmane.org/gmane.linux.kernel/1244476.

Paolo

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

[Paolo Bonzini]

Il 29/02/2012 01:14, Linus Torvalds ha scritto:
> So I'm still not convinced this is safe, and feel a bit worried about
> us possibly silently missing some things. That
> 
>    default:
>       return -ENOIOCTLCMD;
> 
> is what worries me.
> 
> Blocking the ones we *know* about and understand I'm perfectly fine
> with. And the SG_IO case looks fine. It's the possibly unknown users
> that still worry me.

Are you okay with applying this at the beginning of the next merge window?

My worry is that people get desensitized to the warnings.  As time
passes, the likelihood decreases that an actual problem is reported.

Paolo