[PATCH v3 part1 0/4] Fix SG_IO ambiguity between READ SUBCHANNEL and UNMAP (and other similar cases)

[PATCH v3 part1 0/4] Fix SG_IO ambiguity between READ SUBCHANNEL and UNMAP (and other similar cases)

[Paolo Bonzini]

The SG_IO ioctl's command whitelist is designed for MMC devices (roughly,
"play/burn CDs without requiring root") but some opcodes overlap across SCSI
device classes and have different meanings for different classes.

To fix this, use different bitmaps for the various device classes.
This is CVE-2012-4542.

v2->v3: patches are now split differently, according to Tejun's indications;
	added conflict on operation code A4h.

Paolo Bonzini (4):
  sg_io: pass request_queue to blk_verify_command
  sg_io: prepare to introduce per-class command filters
  sg_io: use different default filters for each device class
  sg_io: resolve conflicts between commands assigned to multiple classes
    (CVE-2012-4542)

 block/bsg.c              |   2 +-
 block/scsi_ioctl.c       | 193 +++++++++++++++++++++++++++--------------------
 drivers/scsi/scsi_scan.c |   2 +
 drivers/scsi/sg.c        |   3 +-
 include/linux/blkdev.h   |   5 +-
 5 files changed, 118 insertions(+), 87 deletions(-)

-- 
1.8.1.4

[PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[Paolo Bonzini]

Adjust the blk_verify_command function to let it look at per-queue
data.  This will be done in the next patch.

Acked-by: Tejun Heo <tj@kernel.org>
Cc: stable@gnu.org
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Doug Gilbert <dgilbert@interlog.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/bsg.c            | 2 +-
 block/scsi_ioctl.c     | 7 ++++---
 drivers/scsi/sg.c      | 3 ++-
 include/linux/blkdev.h | 3 ++-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 420a5a9..dedd83c 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -187,7 +187,7 @@ static int blk_fill_sgv4_hdr_rq(struct request_queue *q, struct request *rq,
 		return -EFAULT;
 
 	if (hdr->subprotocol == BSG_SUB_PROTOCOL_SCSI_CMD) {
-		if (blk_verify_command(rq->cmd, has_write_perm))
+		if (blk_verify_command(q, rq->cmd, has_write_perm))
 			return -EPERM;
 	} else if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index a5ffcc9..96cab50 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -197,7 +197,8 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
 	__set_bit(GPCMD_SET_READ_AHEAD, filter->write_ok);
 }
 
-int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm)
+int blk_verify_command(struct request_queue *q,
+		       unsigned char *cmd, fmode_t has_write_perm)
 {
 	struct blk_cmd_filter *filter = &blk_default_cmd_filter;
 
@@ -226,7 +227,7 @@ static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
 {
 	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
 		return -EFAULT;
-	if (blk_verify_command(rq->cmd, mode & FMODE_WRITE))
+	if (blk_verify_command(q, rq->cmd, mode & FMODE_WRITE))
 		return -EPERM;
 
 	/*
@@ -473,7 +474,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 	if (in_len && copy_from_user(buffer, sic->data + cmdlen, in_len))
 		goto error;
 
-	err = blk_verify_command(rq->cmd, mode & FMODE_WRITE);
+	err = blk_verify_command(q, rq->cmd, mode & FMODE_WRITE);
 	if (err)
 		goto error;
 
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index df5e961..533e789 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -218,11 +218,12 @@ static void sg_put_dev(Sg_device *sdp);
 static int sg_allow_access(struct file *filp, unsigned char *cmd)
 {
 	struct sg_fd *sfp = filp->private_data;
+	struct request_queue *q = sfp->parentdp->device->request_queue;
 
 	if (sfp->parentdp->device->type == TYPE_SCANNER)
 		return 0;
 
-	return blk_verify_command(cmd, filp->f_mode & FMODE_WRITE);
+	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
 }
 
 static int get_exclude(Sg_device *sdp)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 2fdb4a4..4fca347 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1089,7 +1089,8 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
 				    gfp_mask);
 }
 
-extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
+extern int blk_verify_command(struct request_queue *q,
+			      unsigned char *cmd, fmode_t has_write_perm);
 
 enum blk_default_limits {
 	BLK_MAX_SEGMENTS	= 128,
-- 
1.8.1.4

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[James Bottomley]

On Thu, 2013-05-23 at 15:58 +0200, Paolo Bonzini wrote:
> Adjust the blk_verify_command function to let it look at per-queue
> data.  This will be done in the next patch.

This is not a bug fix.  This is an enabler for your complex and to my
mind dubious rework of the SG_IO command filter.  I'm running out of
ways to say please don't mix bug fixes with features, because this
redesignating of the original patch set as part 1 and parts 2,3 doesn't
satisfy the requirement.

Does anyone in the real world actually care about this bug?  because if
not perhaps we can just remove the confusion and consider this as a
feature set.  If there's someone who actually cares, please lets just do
the bug fix first and argue about the feature later.

James

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[Paolo Bonzini]

Il 24/05/2013 09:36, James Bottomley ha scritto:
> On Thu, 2013-05-23 at 15:58 +0200, Paolo Bonzini wrote:
>> Adjust the blk_verify_command function to let it look at per-queue
>> data.  This will be done in the next patch.
> 
> This is not a bug fix.  This is an enabler for your complex and to my
> mind dubious rework of the SG_IO command filter.  I'm running out of
> ways to say please don't mix bug fixes with features, because this
> redesignating of the original patch set as part 1 and parts 2,3 doesn't
> satisfy the requirement.

I made it part 1/2/3 because parts 2/3 depend on part 1.  It makes
dependency tracking easier, at least in my mind.

If you have another solution that does not require passing request_queue
to blk_verify_command, I'm all ears.

> Does anyone in the real world actually care about this bug?

Yes, or I would move on and not waste so much time on this.

Paolo

> because if
> not perhaps we can just remove the confusion and consider this as a
> feature set.  If there's someone who actually cares, please lets just do
> the bug fix first and argue about the feature later.
> 
> James
> 
> 

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[James Bottomley]

On Fri, 2013-05-24 at 09:43 +0200, Paolo Bonzini wrote:
> Il 24/05/2013 09:36, James Bottomley ha scritto:
> > On Thu, 2013-05-23 at 15:58 +0200, Paolo Bonzini wrote:
> >> Adjust the blk_verify_command function to let it look at per-queue
> >> data.  This will be done in the next patch.
> > 
> > This is not a bug fix.  This is an enabler for your complex and to my
> > mind dubious rework of the SG_IO command filter.  I'm running out of
> > ways to say please don't mix bug fixes with features, because this
> > redesignating of the original patch set as part 1 and parts 2,3 doesn't
> > satisfy the requirement.
> 
> I made it part 1/2/3 because parts 2/3 depend on part 1.  It makes
> dependency tracking easier, at least in my mind.
> 
> If you have another solution that does not require passing request_queue
> to blk_verify_command, I'm all ears.

That's a circular response that doesn't answer the question.  The actual
question is: what is simple fix for the bug that isn't entangled with
enabling the SG_IO per device type whitelist feature.

> > Does anyone in the real world actually care about this bug?
> 
> Yes, or I would move on and not waste so much time on this.

Fine, so produce a simple fix for this bug which we can discuss that's
not tied to this feature.

James

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[Paolo Bonzini]

Il 24/05/2013 09:50, James Bottomley ha scritto:
> On Fri, 2013-05-24 at 09:43 +0200, Paolo Bonzini wrote:
>> Il 24/05/2013 09:36, James Bottomley ha scritto:
>>> On Thu, 2013-05-23 at 15:58 +0200, Paolo Bonzini wrote:
>>>> Adjust the blk_verify_command function to let it look at per-queue
>>>> data.  This will be done in the next patch.
>>>
>>> This is not a bug fix.  This is an enabler for your complex and to my
>>> mind dubious rework of the SG_IO command filter.  I'm running out of
>>> ways to say please don't mix bug fixes with features, because this
>>> redesignating of the original patch set as part 1 and parts 2,3 doesn't
>>> satisfy the requirement.
>>
>> I made it part 1/2/3 because parts 2/3 depend on part 1.  It makes
>> dependency tracking easier, at least in my mind.
>>
>> If you have another solution that does not require passing request_queue
>> to blk_verify_command, I'm all ears.
> 
> That's a circular response that doesn't answer the question.  The actual
> question is: what is simple fix for the bug that isn't entangled with
> enabling the SG_IO per device type whitelist feature.
> 
>>> Does anyone in the real world actually care about this bug?
>>
>> Yes, or I would move on and not waste so much time on this.
> 
> Fine, so produce a simple fix for this bug which we can discuss that's
> not tied to this feature.

Honestly, I have no idea how this is even possible.

Paolo

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[James Bottomley]

On Fri, 2013-05-24 at 09:53 +0200, Paolo Bonzini wrote:
> Il 24/05/2013 09:50, James Bottomley ha scritto:
> > On Fri, 2013-05-24 at 09:43 +0200, Paolo Bonzini wrote:
> >> Il 24/05/2013 09:36, James Bottomley ha scritto:
> >>> On Thu, 2013-05-23 at 15:58 +0200, Paolo Bonzini wrote:
> >>>> Adjust the blk_verify_command function to let it look at per-queue
> >>>> data.  This will be done in the next patch.
> >>>
> >>> This is not a bug fix.  This is an enabler for your complex and to my
> >>> mind dubious rework of the SG_IO command filter.  I'm running out of
> >>> ways to say please don't mix bug fixes with features, because this
> >>> redesignating of the original patch set as part 1 and parts 2,3 doesn't
> >>> satisfy the requirement.
> >>
> >> I made it part 1/2/3 because parts 2/3 depend on part 1.  It makes
> >> dependency tracking easier, at least in my mind.
> >>
> >> If you have another solution that does not require passing request_queue
> >> to blk_verify_command, I'm all ears.
> > 
> > That's a circular response that doesn't answer the question.  The actual
> > question is: what is simple fix for the bug that isn't entangled with
> > enabling the SG_IO per device type whitelist feature.
> > 
> >>> Does anyone in the real world actually care about this bug?
> >>
> >> Yes, or I would move on and not waste so much time on this.
> > 
> > Fine, so produce a simple fix for this bug which we can discuss that's
> > not tied to this feature.
> 
> Honestly, I have no idea how this is even possible.

Really?  It looks to me like a simple block on the commands for disk
devices in the opcode switch would do it (with a corresponding change to
sg.c:sg_allow_access).

James

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[Paolo Bonzini]

Il 24/05/2013 10:03, James Bottomley ha scritto:
>>>>> > >>> Does anyone in the real world actually care about this bug?
>>>> > >>
>>>> > >> Yes, or I would move on and not waste so much time on this.
>>> > > 
>>> > > Fine, so produce a simple fix for this bug which we can discuss that's
>>> > > not tied to this feature.
>> > 
>> > Honestly, I have no idea how this is even possible.
> Really?  It looks to me like a simple block on the commands for disk
> devices in the opcode switch would do it (with a corresponding change to
> sg.c:sg_allow_access).

Which switch?  What I can do is something like this in blk_verify_command:

        if (q->sgio_type == TYPE_ROM)
		return 0;
	if (rq->cmd[0] == 0xA4)
		return -EPERM;
	if (!is_write &&
	    (req->cmd[0] == ... || rq->cmd[0] == ...))
		return -EPERM;

But then the particular patch you're replying to is still necessary,
and you're slowing down blk_verify_command.  It may be fine for stable
and -rc, but IMHO it calls for a better implementation in 3.11.

(Besides, I did it like this because it is what Tejun suggested).

Paolo

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[Paolo Bonzini]

Il 24/05/2013 10:32, Paolo Bonzini ha scritto:
> Il 24/05/2013 10:03, James Bottomley ha scritto:
>>>>>>>>>> Does anyone in the real world actually care about this bug?
>>>>>>>>
>>>>>>>> Yes, or I would move on and not waste so much time on this.
>>>>>>
>>>>>> Fine, so produce a simple fix for this bug which we can discuss that's
>>>>>> not tied to this feature.
>>>>
>>>> Honestly, I have no idea how this is even possible.
>> Really?  It looks to me like a simple block on the commands for disk
>> devices in the opcode switch would do it (with a corresponding change to
>> sg.c:sg_allow_access).
> 
> Which switch?  What I can do is something like this in blk_verify_command:
> 
>         if (q->sgio_type == TYPE_ROM)
> 		return 0;
> 	if (rq->cmd[0] == 0xA4)
> 		return -EPERM;
> 	if (!is_write &&
> 	    (req->cmd[0] == ... || rq->cmd[0] == ...))
> 		return -EPERM;
> 
> But then the particular patch you're replying to is still necessary,
> and you're slowing down blk_verify_command.  It may be fine for stable
> and -rc, but IMHO it calls for a better implementation in 3.11.

Ok, so I did a patch along these lines.  And it's just as ugly as
everything else that I've been posting in these threads.  Yes, perhaps
it has a redeeming grace in that it is fine for <= 3.10, but that's it.

Because actually I agree with you.  The rework of the SG_IO command
filter _is_ dubious to say the least; 300 lines of default, immutable
policy don't belong in the kernel.

So why am I posting this crap?  Because I have to work around the nack
of the generic sysfs bitmap patches, which would have beatifully solved
all of this.

In fact, you had proposed that approach.  I posted it in September 2012.
 Then (as usual) silence for one month until it was quickly dismissed by
Tejun.

*You and Jens* failed to review patches, and this rathole is what that
led to.  It's unpleasant for me as it is for everyone else.

Yes, you and Jens are busy, we all are.  But *you and Jens* are the
maintainers.  Please make a decision instead of drawing straws, so that
we can all go back to our regular business.

Paolo

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[James Bottomley]

On Fri, 2013-05-24 at 10:32 +0200, Paolo Bonzini wrote:
> Il 24/05/2013 10:03, James Bottomley ha scritto:
> >>>>> > >>> Does anyone in the real world actually care about this bug?
> >>>> > >>
> >>>> > >> Yes, or I would move on and not waste so much time on this.
> >>> > > 
> >>> > > Fine, so produce a simple fix for this bug which we can discuss that's
> >>> > > not tied to this feature.
> >> > 
> >> > Honestly, I have no idea how this is even possible.
> > Really?  It looks to me like a simple block on the commands for disk
> > devices in the opcode switch would do it (with a corresponding change to
> > sg.c:sg_allow_access).
> 
> Which switch?  What I can do is something like this in blk_verify_command:

not in blk_verify_command: outside of it, in the three places it's used.

>         if (q->sgio_type == TYPE_ROM)
> 		return 0;
> 	if (rq->cmd[0] == 0xA4)
> 		return -EPERM;
> 	if (!is_write &&
> 	    (req->cmd[0] == ... || rq->cmd[0] == ...))
> 		return -EPERM;
> 
> But then the particular patch you're replying to is still necessary,
> and you're slowing down blk_verify_command.

It's a set if if switches in non performance critical code.

>   It may be fine for stable
> and -rc, but IMHO it calls for a better implementation in 3.11.

What goes into stable should be what goes into the real kernel and it
helps separate the bug fix from feature argument.

James

> (Besides, I did it like this because it is what Tejun suggested).

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

[Paolo Bonzini]

Il 25/05/2013 06:14, James Bottomley ha scritto:
> On Fri, 2013-05-24 at 10:32 +0200, Paolo Bonzini wrote:
>> Il 24/05/2013 10:03, James Bottomley ha scritto:
>>>>>>>>>>> Does anyone in the real world actually care about this bug?
>>>>>>>>>
>>>>>>>>> Yes, or I would move on and not waste so much time on this.
>>>>>>>
>>>>>>> Fine, so produce a simple fix for this bug which we can discuss that's
>>>>>>> not tied to this feature.
>>>>>
>>>>> Honestly, I have no idea how this is even possible.
>>> Really?  It looks to me like a simple block on the commands for disk
>>> devices in the opcode switch would do it (with a corresponding change to
>>> sg.c:sg_allow_access).
>>
>> Which switch?  What I can do is something like this in blk_verify_command:
> 
> not in blk_verify_command: outside of it, in the three places it's used.

In other words,

	if (blk_verify_command(...) ||
	    blk_verify_command_with_queue(q, ...))

We must have different taste.

Paolo

[PATCH v3 part1 2/4] sg_io: prepare to introduce per-class command filters

[Paolo Bonzini]

To prepare for the next patches, abstract setting of an entry in the
command filter behind a macro.

The next patch will change the implementation of the macro.

Cc: stable@gnu.org
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/scsi_ioctl.c | 148 +++++++++++++++++++++++++++--------------------------
 1 file changed, 76 insertions(+), 72 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 96cab50..21ddf17 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -116,85 +116,89 @@ static int sg_emulated_host(struct request_queue *q, int __user *p)
 
 static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
 {
+#define sgio_bitmap_set(cmd, rw) \
+	__set_bit((cmd), filter->rw##_ok)
+
 	/* Basic read-only commands */
-	__set_bit(TEST_UNIT_READY, filter->read_ok);
-	__set_bit(REQUEST_SENSE, filter->read_ok);
-	__set_bit(READ_6, filter->read_ok);
-	__set_bit(READ_10, filter->read_ok);
-	__set_bit(READ_12, filter->read_ok);
-	__set_bit(READ_16, filter->read_ok);
-	__set_bit(READ_BUFFER, filter->read_ok);
-	__set_bit(READ_DEFECT_DATA, filter->read_ok);
-	__set_bit(READ_CAPACITY, filter->read_ok);
-	__set_bit(READ_LONG, filter->read_ok);
-	__set_bit(INQUIRY, filter->read_ok);
-	__set_bit(MODE_SENSE, filter->read_ok);
-	__set_bit(MODE_SENSE_10, filter->read_ok);
-	__set_bit(LOG_SENSE, filter->read_ok);
-	__set_bit(START_STOP, filter->read_ok);
-	__set_bit(GPCMD_VERIFY_10, filter->read_ok);
-	__set_bit(VERIFY_16, filter->read_ok);
-	__set_bit(REPORT_LUNS, filter->read_ok);
-	__set_bit(SERVICE_ACTION_IN, filter->read_ok);
-	__set_bit(RECEIVE_DIAGNOSTIC, filter->read_ok);
-	__set_bit(MAINTENANCE_IN, filter->read_ok);
-	__set_bit(GPCMD_READ_BUFFER_CAPACITY, filter->read_ok);
+	sgio_bitmap_set(TEST_UNIT_READY, read);
+	sgio_bitmap_set(REQUEST_SENSE, read);
+	sgio_bitmap_set(READ_6, read);
+	sgio_bitmap_set(READ_10, read);
+	sgio_bitmap_set(READ_12, read);
+	sgio_bitmap_set(READ_16, read);
+	sgio_bitmap_set(READ_BUFFER, read);
+	sgio_bitmap_set(READ_DEFECT_DATA, read);
+	sgio_bitmap_set(READ_CAPACITY, read);
+	sgio_bitmap_set(READ_LONG, read);
+	sgio_bitmap_set(INQUIRY, read);
+	sgio_bitmap_set(MODE_SENSE, read);
+	sgio_bitmap_set(MODE_SENSE_10, read);
+	sgio_bitmap_set(LOG_SENSE, read);
+	sgio_bitmap_set(START_STOP, read);
+	sgio_bitmap_set(GPCMD_VERIFY_10, read);
+	sgio_bitmap_set(VERIFY_16, read);
+	sgio_bitmap_set(REPORT_LUNS, read);
+	sgio_bitmap_set(SERVICE_ACTION_IN, read);
+	sgio_bitmap_set(RECEIVE_DIAGNOSTIC, read);
+	sgio_bitmap_set(MAINTENANCE_IN, read);
+	sgio_bitmap_set(GPCMD_READ_BUFFER_CAPACITY, read);
 
 	/* Audio CD commands */
-	__set_bit(GPCMD_PLAY_CD, filter->read_ok);
-	__set_bit(GPCMD_PLAY_AUDIO_10, filter->read_ok);
-	__set_bit(GPCMD_PLAY_AUDIO_MSF, filter->read_ok);
-	__set_bit(GPCMD_PLAY_AUDIO_TI, filter->read_ok);
-	__set_bit(GPCMD_PAUSE_RESUME, filter->read_ok);
+	sgio_bitmap_set(GPCMD_PLAY_CD, read);
+	sgio_bitmap_set(GPCMD_PLAY_AUDIO_10, read);
+	sgio_bitmap_set(GPCMD_PLAY_AUDIO_MSF, read);
+	sgio_bitmap_set(GPCMD_PLAY_AUDIO_TI, read);
+	sgio_bitmap_set(GPCMD_PAUSE_RESUME, read);
 
 	/* CD/DVD data reading */
-	__set_bit(GPCMD_READ_CD, filter->read_ok);
-	__set_bit(GPCMD_READ_CD_MSF, filter->read_ok);
-	__set_bit(GPCMD_READ_DISC_INFO, filter->read_ok);
-	__set_bit(GPCMD_READ_CDVD_CAPACITY, filter->read_ok);
-	__set_bit(GPCMD_READ_DVD_STRUCTURE, filter->read_ok);
-	__set_bit(GPCMD_READ_HEADER, filter->read_ok);
-	__set_bit(GPCMD_READ_TRACK_RZONE_INFO, filter->read_ok);
-	__set_bit(GPCMD_READ_SUBCHANNEL, filter->read_ok);
-	__set_bit(GPCMD_READ_TOC_PMA_ATIP, filter->read_ok);
-	__set_bit(GPCMD_REPORT_KEY, filter->read_ok);
-	__set_bit(GPCMD_SCAN, filter->read_ok);
-	__set_bit(GPCMD_GET_CONFIGURATION, filter->read_ok);
-	__set_bit(GPCMD_READ_FORMAT_CAPACITIES, filter->read_ok);
-	__set_bit(GPCMD_GET_EVENT_STATUS_NOTIFICATION, filter->read_ok);
-	__set_bit(GPCMD_GET_PERFORMANCE, filter->read_ok);
-	__set_bit(GPCMD_SEEK, filter->read_ok);
-	__set_bit(GPCMD_STOP_PLAY_SCAN, filter->read_ok);
+	sgio_bitmap_set(GPCMD_READ_CD, read);
+	sgio_bitmap_set(GPCMD_READ_CD_MSF, read);
+	sgio_bitmap_set(GPCMD_READ_DISC_INFO, read);
+	sgio_bitmap_set(GPCMD_READ_CDVD_CAPACITY, read);
+	sgio_bitmap_set(GPCMD_READ_DVD_STRUCTURE, read);
+	sgio_bitmap_set(GPCMD_READ_HEADER, read);
+	sgio_bitmap_set(GPCMD_READ_TRACK_RZONE_INFO, read);
+	sgio_bitmap_set(GPCMD_READ_SUBCHANNEL, read);
+	sgio_bitmap_set(GPCMD_READ_TOC_PMA_ATIP, read);
+	sgio_bitmap_set(GPCMD_REPORT_KEY, read);
+	sgio_bitmap_set(GPCMD_SCAN, read);
+	sgio_bitmap_set(GPCMD_GET_CONFIGURATION, read);
+	sgio_bitmap_set(GPCMD_READ_FORMAT_CAPACITIES, read);
+	sgio_bitmap_set(GPCMD_GET_EVENT_STATUS_NOTIFICATION, read);
+	sgio_bitmap_set(GPCMD_GET_PERFORMANCE, read);
+	sgio_bitmap_set(GPCMD_SEEK, read);
+	sgio_bitmap_set(GPCMD_STOP_PLAY_SCAN, read);
 
 	/* Basic writing commands */
-	__set_bit(WRITE_6, filter->write_ok);
-	__set_bit(WRITE_10, filter->write_ok);
-	__set_bit(WRITE_VERIFY, filter->write_ok);
-	__set_bit(WRITE_12, filter->write_ok);
-	__set_bit(WRITE_VERIFY_12, filter->write_ok);
-	__set_bit(WRITE_16, filter->write_ok);
-	__set_bit(WRITE_LONG, filter->write_ok);
-	__set_bit(WRITE_LONG_2, filter->write_ok);
-	__set_bit(ERASE, filter->write_ok);
-	__set_bit(GPCMD_MODE_SELECT_10, filter->write_ok);
-	__set_bit(MODE_SELECT, filter->write_ok);
-	__set_bit(LOG_SELECT, filter->write_ok);
-	__set_bit(GPCMD_BLANK, filter->write_ok);
-	__set_bit(GPCMD_CLOSE_TRACK, filter->write_ok);
-	__set_bit(GPCMD_FLUSH_CACHE, filter->write_ok);
-	__set_bit(GPCMD_FORMAT_UNIT, filter->write_ok);
-	__set_bit(GPCMD_REPAIR_RZONE_TRACK, filter->write_ok);
-	__set_bit(GPCMD_RESERVE_RZONE_TRACK, filter->write_ok);
-	__set_bit(GPCMD_SEND_DVD_STRUCTURE, filter->write_ok);
-	__set_bit(GPCMD_SEND_EVENT, filter->write_ok);
-	__set_bit(GPCMD_SEND_KEY, filter->write_ok);
-	__set_bit(GPCMD_SEND_OPC, filter->write_ok);
-	__set_bit(GPCMD_SEND_CUE_SHEET, filter->write_ok);
-	__set_bit(GPCMD_SET_SPEED, filter->write_ok);
-	__set_bit(GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL, filter->write_ok);
-	__set_bit(GPCMD_LOAD_UNLOAD, filter->write_ok);
-	__set_bit(GPCMD_SET_STREAMING, filter->write_ok);
-	__set_bit(GPCMD_SET_READ_AHEAD, filter->write_ok);
+	sgio_bitmap_set(WRITE_6, write);
+	sgio_bitmap_set(WRITE_10, write);
+	sgio_bitmap_set(WRITE_VERIFY, write);
+	sgio_bitmap_set(WRITE_12, write);
+	sgio_bitmap_set(WRITE_VERIFY_12, write);
+	sgio_bitmap_set(WRITE_16, write);
+	sgio_bitmap_set(WRITE_LONG, write);
+	sgio_bitmap_set(WRITE_LONG_2, write);
+	sgio_bitmap_set(ERASE, write);
+	sgio_bitmap_set(GPCMD_MODE_SELECT_10, write);
+	sgio_bitmap_set(MODE_SELECT, write);
+	sgio_bitmap_set(LOG_SELECT, write);
+	sgio_bitmap_set(GPCMD_BLANK, write);
+	sgio_bitmap_set(GPCMD_CLOSE_TRACK, write);
+	sgio_bitmap_set(GPCMD_FLUSH_CACHE, write);
+	sgio_bitmap_set(GPCMD_FORMAT_UNIT, write);
+	sgio_bitmap_set(GPCMD_REPAIR_RZONE_TRACK, write);
+	sgio_bitmap_set(GPCMD_RESERVE_RZONE_TRACK, write);
+	sgio_bitmap_set(GPCMD_SEND_DVD_STRUCTURE, write);
+	sgio_bitmap_set(GPCMD_SEND_EVENT, write);
+	sgio_bitmap_set(GPCMD_SEND_KEY, write);
+	sgio_bitmap_set(GPCMD_SEND_OPC, write);
+	sgio_bitmap_set(GPCMD_SEND_CUE_SHEET, write);
+	sgio_bitmap_set(GPCMD_SET_SPEED, write);
+	sgio_bitmap_set(GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL, write);
+	sgio_bitmap_set(GPCMD_LOAD_UNLOAD, write);
+	sgio_bitmap_set(GPCMD_SET_STREAMING, write);
+	sgio_bitmap_set(GPCMD_SET_READ_AHEAD, write);
+#undef sgio_bitmap_set
 }
 
 int blk_verify_command(struct request_queue *q,
-- 
1.8.1.4

[PATCH v3 part1 3/4] sg_io: use different default filters for each device class

[Paolo Bonzini]

Store the filters in a 256-entry array, and pick an appropriate filter
for SCSI devices.  Apart from SCSI disks, SG_IO is supported for CCISS,
ide-floppy and virtio-blk devices; TYPE_DISK (which is zero, i.e. the
default) is more appropriate for these devices than TYPE_ROM.

However, all lists are still the same, so there is no semantic change
in this patch.

Cc: stable@gnu.org
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/scsi_ioctl.c       | 14 +++++---------
 drivers/scsi/scsi_scan.c |  2 ++
 include/linux/blkdev.h   |  2 +-
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 21ddf17..6e18156 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -35,8 +35,8 @@
 #include <scsi/scsi_cmnd.h>
 
 struct blk_cmd_filter {
-	unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
-	unsigned long write_ok[BLK_SCSI_CMD_PER_LONG];
+	u32 read_ok[BLK_SCSI_MAX_CMDS];
+	u32 write_ok[BLK_SCSI_MAX_CMDS];
 };
 
 static struct blk_cmd_filter blk_default_cmd_filter;
@@ -117,7 +117,7 @@ static int sg_emulated_host(struct request_queue *q, int __user *p)
 static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
 {
 #define sgio_bitmap_set(cmd, rw) \
-	__set_bit((cmd), filter->rw##_ok)
+	filter->rw##_ok[(cmd)] = ~0;
 
 	/* Basic read-only commands */
 	sgio_bitmap_set(TEST_UNIT_READY, read);
@@ -210,16 +210,12 @@ int blk_verify_command(struct request_queue *q,
 	if (capable(CAP_SYS_RAWIO))
 		return 0;
 
-	/* if there's no filter set, assume we're filtering everything out */
-	if (!filter)
-		return -EPERM;
-
 	/* Anybody who can open the device can do a read-safe command */
-	if (test_bit(cmd[0], filter->read_ok))
+	if (filter->read_ok[cmd[0]] & (1 << q->sgio_type))
 		return 0;
 
 	/* Write-safe commands require a writable open */
-	if (test_bit(cmd[0], filter->write_ok) && has_write_perm)
+	if (has_write_perm && filter->write_ok[cmd[0]] & (1 << q->sgio_type))
 		return 0;
 
 	return -EPERM;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3e58b22..86940f3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -782,6 +782,8 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		sdev->removable = (inq_result[1] & 0x80) >> 7;
 	}
 
+	sdev->request_queue->sgio_type = sdev->type;
+
 	switch (sdev->type) {
 	case TYPE_RBC:
 	case TYPE_TAPE:
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4fca347..5e18969 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -257,7 +257,6 @@ struct blk_queue_tag {
 };
 
 #define BLK_SCSI_MAX_CMDS	(256)
-#define BLK_SCSI_CMD_PER_LONG	(BLK_SCSI_MAX_CMDS / (sizeof(long) * 8))
 
 struct queue_limits {
 	unsigned long		bounce_pfn;
@@ -410,6 +409,7 @@ struct request_queue {
 	 */
 	unsigned int		sg_timeout;
 	unsigned int		sg_reserved_size;
+	unsigned char		sgio_type;
 	int			node;
 #ifdef CONFIG_BLK_DEV_IO_TRACE
 	struct blk_trace	*blk_trace;
-- 
1.8.1.4

[PATCH v3 part1 4/4] sg_io: resolve conflicts between commands assigned to multiple classes (CVE-2012-4542)

[Paolo Bonzini]

Some SCSI commands can be sent to disks via SG_IO even by unprivileged
users.  Unfortunately, some opcodes overlap across SCSI device classes
and have different meanings for different classes.  Four of them can
be used for read-only file descriptors on MMC, but should be limited to
descriptors opened for read-write on SBC:

- READ SUBCHANNEL <-> UNMAP (destructive, but no control on written
  data)

- GET PERFORMANCE <-> ERASE (not really a problem, no one supports
  ERASE anyway)

- READ DISC INFORMATION <-> XPWRITE (not commonly implemented but
  most dangerous)

- PLAY AUDIO TI <-> SANITIZE (a very new command)

In addition, REPORT KEY's opcode A4h is used in SPC for SET TARGET PORT
GROUPS and various other management commands, and should be blocked
for everything except CD-ROMs and the like.

To fix this, the series modifies the bitmap entries for these five
commands.  This is the smallest change that fixes this bug.

Cc: stable@gnu.org
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/scsi_ioctl.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 6e18156..7a1d9f6 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -199,6 +199,32 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
 	sgio_bitmap_set(GPCMD_SET_STREAMING, write);
 	sgio_bitmap_set(GPCMD_SET_READ_AHEAD, write);
 #undef sgio_bitmap_set
+
+	/*
+	 * Treat specially those commands that have a different meaning
+	 * for disks: READ SUBCHANNEL conflicts with UNMAP.
+	 */
+	filter->read_ok[GPCMD_READ_SUBCHANNEL] &= ~(1 << TYPE_DISK);
+	filter->write_ok[GPCMD_READ_SUBCHANNEL] |= (1 << TYPE_DISK);
+
+	/* PLAY AUDIO TI conflicts with SANITIZE.  */
+	filter->read_ok[GPCMD_PLAY_AUDIO_TI] &= ~((1 << TYPE_DISK) | (1 << TYPE_RBC));
+	filter->write_ok[GPCMD_PLAY_AUDIO_TI] |= (1 << TYPE_DISK) | (1 << TYPE_RBC);
+
+	/* READ DISC INFORMATION conflicts with XPWRITE.  */
+	filter->read_ok[GPCMD_READ_DISC_INFO] &= ~(1 << TYPE_DISK);
+	filter->write_ok[GPCMD_READ_DISC_INFO] |= (1 << TYPE_DISK);
+
+	/* GET PERFORMANCE conflicts with ERASE.  */
+	filter->read_ok[GPCMD_GET_PERFORMANCE] &= ~(1 << TYPE_MOD);
+	filter->write_ok[GPCMD_GET_PERFORMANCE] |= (1 << TYPE_MOD);
+
+	/*
+	 * REPORT KEY conflicts with many management commands under operation
+	 * code 0xA4, enable it only for MMC devices.
+	 */
+	filter->read_ok[GPCMD_REPORT_KEY] = (1 << TYPE_ROM);
+	filter->write_ok[GPCMD_REPORT_KEY] = (1 << TYPE_ROM);
 }
 
 int blk_verify_command(struct request_queue *q,
-- 
1.8.1.4

Re: [PATCH v3 part1 0/4] Fix SG_IO ambiguity between READ SUBCHANNEL and UNMAP (and other similar cases)

[Luis Henriques]

On Thu, May 23, 2013 at 03:58:19PM +0200, Paolo Bonzini wrote:
> The SG_IO ioctl's command whitelist is designed for MMC devices (roughly,
> "play/burn CDs without requiring root") but some opcodes overlap across SCSI
> device classes and have different meanings for different classes.
> 
> To fix this, use different bitmaps for the various device classes.
> This is CVE-2012-4542.
>

Sorry for bringing this old issue again, but I was wondering what
happen to this fix.

Cheers,
--
Luís

> v2->v3: patches are now split differently, according to Tejun's indications;
> 	added conflict on operation code A4h.
> 
> Paolo Bonzini (4):
>   sg_io: pass request_queue to blk_verify_command
>   sg_io: prepare to introduce per-class command filters
>   sg_io: use different default filters for each device class
>   sg_io: resolve conflicts between commands assigned to multiple classes
>     (CVE-2012-4542)
> 
>  block/bsg.c              |   2 +-
>  block/scsi_ioctl.c       | 193 +++++++++++++++++++++++++++--------------------
>  drivers/scsi/scsi_scan.c |   2 +
>  drivers/scsi/sg.c        |   3 +-
>  include/linux/blkdev.h   |   5 +-
>  5 files changed, 118 insertions(+), 87 deletions(-)
> 
> -- 
> 1.8.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 part1 0/4] Fix SG_IO ambiguity between READ SUBCHANNEL and UNMAP (and other similar cases)

[Paolo Bonzini]

Il 27/08/2014 11:34, Luis Henriques ha scritto:
> > The SG_IO ioctl's command whitelist is designed for MMC devices (roughly,
> > "play/burn CDs without requiring root") but some opcodes overlap across SCSI
> > device classes and have different meanings for different classes.
> > 
> > To fix this, use different bitmaps for the various device classes.
> > This is CVE-2012-4542.
>
> Sorry for bringing this old issue again, but I was wondering what
> happen to this fix.

Nothing; everybody lost interest in it.

Paolo

Re: [PATCH v3 part1 0/4] Fix SG_IO ambiguity between READ SUBCHANNEL and UNMAP (and other similar cases)

[Luis Henriques]

On Wed, Aug 27, 2014 at 12:09:41PM +0200, Paolo Bonzini wrote:
> Il 27/08/2014 11:34, Luis Henriques ha scritto:
> > > The SG_IO ioctl's command whitelist is designed for MMC devices (roughly,
> > > "play/burn CDs without requiring root") but some opcodes overlap across SCSI
> > > device classes and have different meanings for different classes.
> > > 
> > > To fix this, use different bitmaps for the various device classes.
> > > This is CVE-2012-4542.
> >
> > Sorry for bringing this old issue again, but I was wondering what
> > happen to this fix.
> 
> Nothing; everybody lost interest in it.

Thanks a lot for the update, Paolo.

This is what I was actually expecting, although it's somewhat
surprising, taking into account this is a security issue :-)

Cheers,
--
Luís

Re: [PATCH v3 part1 0/4] Fix SG_IO ambiguity between READ SUBCHANNEL and UNMAP (and other similar cases)

[Christoph Hellwig]

On Wed, Aug 27, 2014 at 01:08:06PM +0100, Luis Henriques wrote:
> Thanks a lot for the update, Paolo.
> 
> This is what I was actually expecting, although it's somewhat
> surprising, taking into account this is a security issue :-)

The right fix is to get rid of all the whitelisting BS and just allow
SG_IO if capable, not based on ownership checks.

With this all the problems with trying to work around the different commands
sets are gone.