Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[Markus Armbruster]

John Snow  writes:

> We're a little too lenient with what we'll let an ATAPI drive handle.
> Clamp down on the IDE command execution table to remove CD_OK permissions
> from commands that are not and have never been ATAPI commands.
>
> For ATAPI command validity, please see:
> - ATA4 Section 6.5 ("PACKET Command feature set")
> - ATA8/ACS Section 4.3 ("The PACKET feature set")
> - ACS3 Section 4.3 ("The PACKET feature set")
>
> ACS3 has a historical command validity table in Table B.4
> ("Historical Command Assignments") that can be referenced to find when
> a command was introduced, deprecated, obsoleted, etc.
>
> The only reference for ATAPI command validity is by checking that
> version's PACKET feature set section.
>
> ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
> therefore are assumed to have never been ATAPI commands.
>
> Mandatory commands, as listed in ATA8-ACS3, are:
>
> - DEVICE RESET
> - EXECUTE DEVICE DIAGNOSTIC
> - IDENTIFY DEVICE
> - IDENTIFY PACKET DEVICE
> - NOP
> - PACKET
> - READ SECTOR(S)
> - SET FEATURES
>
> Optional commands as listed in ATA8-ACS3, are:
>
> - FLUSH CACHE
> - READ LOG DMA EXT
> - READ LOG EXT
> - WRITE LOG DMA EXT
> - WRITE LOG EXT
>
> All other commands are illegal to send to an ATAPI device and should
> be rejected by the device.

We could perhaps argue about "should be rejected by the device", but I
think the weaker "a device is free to reject it" still suffices to
support your patch.

> CD_OK removal justifications:
>
> 0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
> 0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
> 0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
> 0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
> 0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
> 0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
> 0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
> 0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
> 0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
> 0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.

Actual patch matches this list.

> This patch fixes a divide by zero fault that can be caused by sending
> the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
> it to attempt to use zeroed CHS values to perform sector arithmetic.
>
> Reported-by: Qinghao Tang 
> Signed-off-by: John Snow 

I appreciate you going to the root of the problem instead of merely
fixing the narrow bug.

Could a similar argument be made for dropping CFA_OK from some commands?

Do we still need this conditional in cmd_read_native_max()?

/* Refuse if no sectors are addressable (e.g. medium not inserted) */
if (s->nb_sectors == 0) {
ide_abort_command(s);
return true;
}

Why does it fail at guarding the CHS use from empty ATAPI drives before
your patch?

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[Markus Armbruster]

John Snow  writes:

> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>> John Snow  writes:
>> 
>>> We're a little too lenient with what we'll let an ATAPI drive handle.
>>> Clamp down on the IDE command execution table to remove CD_OK permissions
>>> from commands that are not and have never been ATAPI commands.
>>>
>>> For ATAPI command validity, please see:
>>> - ATA4 Section 6.5 ("PACKET Command feature set")
>>> - ATA8/ACS Section 4.3 ("The PACKET feature set")
>>> - ACS3 Section 4.3 ("The PACKET feature set")
>>>
>>> ACS3 has a historical command validity table in Table B.4
>>> ("Historical Command Assignments") that can be referenced to find when
>>> a command was introduced, deprecated, obsoleted, etc.
>>>
>>> The only reference for ATAPI command validity is by checking that
>>> version's PACKET feature set section.
>>>
>>> ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
>>> therefore are assumed to have never been ATAPI commands.
>>>
>>> Mandatory commands, as listed in ATA8-ACS3, are:
>>>
>>> - DEVICE RESET
>>> - EXECUTE DEVICE DIAGNOSTIC
>>> - IDENTIFY DEVICE
>>> - IDENTIFY PACKET DEVICE
>>> - NOP
>>> - PACKET
>>> - READ SECTOR(S)
>>> - SET FEATURES
>>>
>>> Optional commands as listed in ATA8-ACS3, are:
>>>
>>> - FLUSH CACHE
>>> - READ LOG DMA EXT
>>> - READ LOG EXT
>>> - WRITE LOG DMA EXT
>>> - WRITE LOG EXT
>>>
>>> All other commands are illegal to send to an ATAPI device and should
>>> be rejected by the device.
>> 
>> We could perhaps argue about "should be rejected by the device", but I
>> think the weaker "a device is free to reject it" still suffices to
>> support your patch.
>> 
>
> Sure -- I suppose drives CAN support a superset if they want to. In my
> mind, anything above the ATAPI spec should be justified directly with
> "Guest X breaks without it."
>
>>> CD_OK removal justifications:
>>>
>>> 0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
>>> 0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
>>> 0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
>>> 0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
>>> 0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
>>> 0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
>>> 0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
>>> 0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
>>> 0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
>>> 0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
>>> 0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
>>> 0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
>>> 0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
>>> 0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
>>> 0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.
>> 
>> Actual patch matches this list.
>> 
>>> This patch fixes a divide by zero fault that can be caused by sending
>>> the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
>>> it to attempt to use zeroed CHS values to perform sector arithmetic.
>>>
>>> Reported-by: Qinghao Tang 
>>> Signed-off-by: John Snow 
>> 
>> I appreciate you going to the root of the problem instead of merely
>> fixing the narrow bug.
>> 
>> Could a similar argument be made for dropping CFA_OK from some commands?
>> 
>
> Very likely, but that's another patch. I didn't audit that yet.
>
>> Do we still need this conditional in cmd_read_native_max()?
>> 
>> /* Refuse if no sectors are addressable (e.g. medium not inserted) */
>> if (s->nb_sectors == 0) {
>> ide_abort_command(s);
>> return true;
>> }
>> 
>> Why does it fail at guarding the CHS use from empty ATAPI drives before
>> your patch?
>> 
>
> Because I misunderstood the real reason myself, and my POC test was a
> little bananas. This works *with* a CDROM inserted, not without.
>
> So s->nb_sectors can be non-zero, and ide_set_sector then triggers e.g.
> SIGFPE.
>
> If you'll save me the re-spin, I can fix that part of the commit message
> in my staging branch.

Let me paraphrase to make sure I got you.

If the drive is empty, the guard aborts the command correctly.

If the drive isn't empty, the guard doesn't abort.  The code then goes
on and happily uses CHS.  However, ATAPI devices don't have CHS
geometry, see ide_dev_initfn():

if (kind != IDE_CD) {
blkconf_geometry(>conf, >chs_trans, 65536, 16, 255, );
if (err) {
error_report_err(err);
return -1;
}
}

Therefore, CHS is all zero, and the code using it blows up.

Correct?

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[John Snow]

On 09/15/2015 02:11 PM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>>> John Snow  writes:
>>>
 We're a little too lenient with what we'll let an ATAPI drive handle.
 Clamp down on the IDE command execution table to remove CD_OK permissions
 from commands that are not and have never been ATAPI commands.

 For ATAPI command validity, please see:
 - ATA4 Section 6.5 ("PACKET Command feature set")
 - ATA8/ACS Section 4.3 ("The PACKET feature set")
 - ACS3 Section 4.3 ("The PACKET feature set")

 ACS3 has a historical command validity table in Table B.4
 ("Historical Command Assignments") that can be referenced to find when
 a command was introduced, deprecated, obsoleted, etc.

 The only reference for ATAPI command validity is by checking that
 version's PACKET feature set section.

 ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
 therefore are assumed to have never been ATAPI commands.

 Mandatory commands, as listed in ATA8-ACS3, are:

 - DEVICE RESET
 - EXECUTE DEVICE DIAGNOSTIC
 - IDENTIFY DEVICE
 - IDENTIFY PACKET DEVICE
 - NOP
 - PACKET
 - READ SECTOR(S)
 - SET FEATURES

 Optional commands as listed in ATA8-ACS3, are:

 - FLUSH CACHE
 - READ LOG DMA EXT
 - READ LOG EXT
 - WRITE LOG DMA EXT
 - WRITE LOG EXT

 All other commands are illegal to send to an ATAPI device and should
 be rejected by the device.
>>>
>>> We could perhaps argue about "should be rejected by the device", but I
>>> think the weaker "a device is free to reject it" still suffices to
>>> support your patch.
>>>
>>
>> Sure -- I suppose drives CAN support a superset if they want to. In my
>> mind, anything above the ATAPI spec should be justified directly with
>> "Guest X breaks without it."
>>
 CD_OK removal justifications:

 0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
 0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
 0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
 0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
 0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
 0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
 0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
 0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
 0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
 0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
 0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
 0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
 0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
 0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
 0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.
>>>
>>> Actual patch matches this list.
>>>
 This patch fixes a divide by zero fault that can be caused by sending
 the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
 it to attempt to use zeroed CHS values to perform sector arithmetic.

 Reported-by: Qinghao Tang 
 Signed-off-by: John Snow 
>>>
>>> I appreciate you going to the root of the problem instead of merely
>>> fixing the narrow bug.
>>>
>>> Could a similar argument be made for dropping CFA_OK from some commands?
>>>
>>
>> Very likely, but that's another patch. I didn't audit that yet.
>>
>>> Do we still need this conditional in cmd_read_native_max()?
>>>
>>> /* Refuse if no sectors are addressable (e.g. medium not inserted) */
>>> if (s->nb_sectors == 0) {
>>> ide_abort_command(s);
>>> return true;
>>> }
>>>
>>> Why does it fail at guarding the CHS use from empty ATAPI drives before
>>> your patch?
>>>
>>
>> Because I misunderstood the real reason myself, and my POC test was a
>> little bananas. This works *with* a CDROM inserted, not without.
>>
>> So s->nb_sectors can be non-zero, and ide_set_sector then triggers e.g.
>> SIGFPE.
>>
>> If you'll save me the re-spin, I can fix that part of the commit message
>> in my staging branch.
> 
> Let me paraphrase to make sure I got you.
> 
> If the drive is empty, the guard aborts the command correctly.
> 
> If the drive isn't empty, the guard doesn't abort.  The code then goes
> on and happily uses CHS.  However, ATAPI devices don't have CHS
> geometry, see ide_dev_initfn():
> 
> if (kind != IDE_CD) {
> blkconf_geometry(>conf, >chs_trans, 65536, 16, 255, );
> if (err) {
> error_report_err(err);
> return -1;
> }
> }
> 
> Therefore, CHS is all zero, and the code using it blows up.
> 
> 

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[Markus Armbruster]

John Snow  writes:

> On 09/15/2015 02:11 PM, Markus Armbruster wrote:
>> John Snow  writes:
>> 
>>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
 John Snow  writes:

> We're a little too lenient with what we'll let an ATAPI drive handle.
> Clamp down on the IDE command execution table to remove CD_OK permissions
> from commands that are not and have never been ATAPI commands.
>
> For ATAPI command validity, please see:
> - ATA4 Section 6.5 ("PACKET Command feature set")
> - ATA8/ACS Section 4.3 ("The PACKET feature set")
> - ACS3 Section 4.3 ("The PACKET feature set")
>
> ACS3 has a historical command validity table in Table B.4
> ("Historical Command Assignments") that can be referenced to find when
> a command was introduced, deprecated, obsoleted, etc.
>
> The only reference for ATAPI command validity is by checking that
> version's PACKET feature set section.
>
> ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
> therefore are assumed to have never been ATAPI commands.
>
> Mandatory commands, as listed in ATA8-ACS3, are:
>
> - DEVICE RESET
> - EXECUTE DEVICE DIAGNOSTIC
> - IDENTIFY DEVICE
> - IDENTIFY PACKET DEVICE
> - NOP
> - PACKET
> - READ SECTOR(S)
> - SET FEATURES
>
> Optional commands as listed in ATA8-ACS3, are:
>
> - FLUSH CACHE
> - READ LOG DMA EXT
> - READ LOG EXT
> - WRITE LOG DMA EXT
> - WRITE LOG EXT
>
> All other commands are illegal to send to an ATAPI device and should
> be rejected by the device.

 We could perhaps argue about "should be rejected by the device", but I
 think the weaker "a device is free to reject it" still suffices to
 support your patch.

>>>
>>> Sure -- I suppose drives CAN support a superset if they want to. In my
>>> mind, anything above the ATAPI spec should be justified directly with
>>> "Guest X breaks without it."
>>>
> CD_OK removal justifications:
>
> 0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
> 0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
> 0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
> 0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
> 0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
> 0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
> 0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
> 0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
> 0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
> 0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
> 0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.

 Actual patch matches this list.

> This patch fixes a divide by zero fault that can be caused by sending
> the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
> it to attempt to use zeroed CHS values to perform sector arithmetic.
>
> Reported-by: Qinghao Tang 
> Signed-off-by: John Snow 

 I appreciate you going to the root of the problem instead of merely
 fixing the narrow bug.

 Could a similar argument be made for dropping CFA_OK from some commands?

>>>
>>> Very likely, but that's another patch. I didn't audit that yet.
>>>
 Do we still need this conditional in cmd_read_native_max()?

 /* Refuse if no sectors are addressable (e.g. medium not inserted) */
 if (s->nb_sectors == 0) {
 ide_abort_command(s);
 return true;
 }

 Why does it fail at guarding the CHS use from empty ATAPI drives before
 your patch?

>>>
>>> Because I misunderstood the real reason myself, and my POC test was a
>>> little bananas. This works *with* a CDROM inserted, not without.
>>>
>>> So s->nb_sectors can be non-zero, and ide_set_sector then triggers e.g.
>>> SIGFPE.
>>>
>>> If you'll save me the re-spin, I can fix that part of the commit message
>>> in my staging branch.
>> 
>> Let me paraphrase to make sure I got you.
>> 
>> If the drive is empty, the guard aborts the command correctly.
>> 
>> If the drive isn't empty, the guard doesn't abort.  The code then goes
>> on and happily uses CHS.  However, ATAPI devices don't have CHS
>> geometry, see ide_dev_initfn():
>> 
>> if (kind != IDE_CD) {
>> blkconf_geometry(>conf, >chs_trans, 65536, 16, 255, );
>> if (err) {

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[John Snow]

On 09/15/2015 02:50 PM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> On 09/15/2015 02:11 PM, Markus Armbruster wrote:
>>> John Snow  writes:
>>>
 On 09/15/2015 02:53 AM, Markus Armbruster wrote:
> John Snow  writes:
>
>> We're a little too lenient with what we'll let an ATAPI drive handle.
>> Clamp down on the IDE command execution table to remove CD_OK permissions
>> from commands that are not and have never been ATAPI commands.
>>
>> For ATAPI command validity, please see:
>> - ATA4 Section 6.5 ("PACKET Command feature set")
>> - ATA8/ACS Section 4.3 ("The PACKET feature set")
>> - ACS3 Section 4.3 ("The PACKET feature set")
>>
>> ACS3 has a historical command validity table in Table B.4
>> ("Historical Command Assignments") that can be referenced to find when
>> a command was introduced, deprecated, obsoleted, etc.
>>
>> The only reference for ATAPI command validity is by checking that
>> version's PACKET feature set section.
>>
>> ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
>> therefore are assumed to have never been ATAPI commands.
>>
>> Mandatory commands, as listed in ATA8-ACS3, are:
>>
>> - DEVICE RESET
>> - EXECUTE DEVICE DIAGNOSTIC
>> - IDENTIFY DEVICE
>> - IDENTIFY PACKET DEVICE
>> - NOP
>> - PACKET
>> - READ SECTOR(S)
>> - SET FEATURES
>>
>> Optional commands as listed in ATA8-ACS3, are:
>>
>> - FLUSH CACHE
>> - READ LOG DMA EXT
>> - READ LOG EXT
>> - WRITE LOG DMA EXT
>> - WRITE LOG EXT
>>
>> All other commands are illegal to send to an ATAPI device and should
>> be rejected by the device.
>
> We could perhaps argue about "should be rejected by the device", but I
> think the weaker "a device is free to reject it" still suffices to
> support your patch.
>

 Sure -- I suppose drives CAN support a superset if they want to. In my
 mind, anything above the ATAPI spec should be justified directly with
 "Guest X breaks without it."

>> CD_OK removal justifications:
>>
>> 0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
>> 0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
>> 0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
>> 0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
>> 0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
>> 0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
>> 0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
>> 0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
>> 0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.
>
> Actual patch matches this list.
>
>> This patch fixes a divide by zero fault that can be caused by sending
>> the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
>> it to attempt to use zeroed CHS values to perform sector arithmetic.
>>
>> Reported-by: Qinghao Tang 
>> Signed-off-by: John Snow 
>
> I appreciate you going to the root of the problem instead of merely
> fixing the narrow bug.
>
> Could a similar argument be made for dropping CFA_OK from some commands?
>

 Very likely, but that's another patch. I didn't audit that yet.

> Do we still need this conditional in cmd_read_native_max()?
>
> /* Refuse if no sectors are addressable (e.g. medium not inserted) */
> if (s->nb_sectors == 0) {
> ide_abort_command(s);
> return true;
> }
>
> Why does it fail at guarding the CHS use from empty ATAPI drives before
> your patch?
>

 Because I misunderstood the real reason myself, and my POC test was a
 little bananas. This works *with* a CDROM inserted, not without.

 So s->nb_sectors can be non-zero, and ide_set_sector then triggers e.g.
 SIGFPE.

 If you'll save me the re-spin, I can fix that part of the commit message
 in my staging branch.
>>>
>>> Let me paraphrase to make sure I got you.
>>>
>>> If the drive is empty, the guard aborts the command correctly.
>>>
>>> If the drive isn't empty, the guard doesn't abort.  The code then goes
>>> on and happily uses CHS.  However, ATAPI devices 

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[Kevin Wolf]

Am 14.09.2015 um 20:49 hat John Snow geschrieben:
> On 09/14/2015 02:43 PM, Michael Tokarev wrote:
> > 14.09.2015 21:04, John Snow wrote:
> >> On 09/11/2015 02:56 AM, Michael Tokarev wrote:
> >>> 09.09.2015 19:28, John Snow wrote:
>  We're a little too lenient with what we'll let an ATAPI drive handle.
>  Clamp down on the IDE command execution table to remove CD_OK permissions
>  from commands that are not and have never been ATAPI commands.
> >>>
> >>> FWIW, this issue has been assigned CVE-2015-6855 identifier, which
> >>> can be reflected in the commit message when applying.  Since this
> >>> issue has security impact, it might be a good idea to add
> >>>
> >>> Cc: qemu-sta...@nongnu.org
> >>
> >> I'm still awaiting review/acks, but would you like me to re-send this
> >> patch to trivial, or just fwd/reply-to?
> > 
> > I think it is anything but trivial ;)  Well, the semantics is trivial,
> > but it isn't -trivial material per se.
> > 
> > I suggested add a Cc to qemu-stable, this can be done at commit,
> > together with mentioning the CVE# assigned meanwhile, and I don't
> > know who will commit it, Kevin?
> > 
> > Thanks,
> > 
> > /mjt
> > 
> 
> I'll be sending the pullreq through my tree, but I was waiting on at
> least an ACK or so before I went ahead.
> 
> I can add CC: qemu-stable to the patch on-pull if that's sufficient for you.

Ideally, stable patches should have a "Cc: qemu-sta...@nongnu.org" line
in the commit message, too. Just adding that on pull should be enough.

Also copying the list for this reply so that there is some trace of the
patch on it.

Kevin

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[John Snow]

On 09/15/2015 02:53 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> We're a little too lenient with what we'll let an ATAPI drive handle.
>> Clamp down on the IDE command execution table to remove CD_OK permissions
>> from commands that are not and have never been ATAPI commands.
>>
>> For ATAPI command validity, please see:
>> - ATA4 Section 6.5 ("PACKET Command feature set")
>> - ATA8/ACS Section 4.3 ("The PACKET feature set")
>> - ACS3 Section 4.3 ("The PACKET feature set")
>>
>> ACS3 has a historical command validity table in Table B.4
>> ("Historical Command Assignments") that can be referenced to find when
>> a command was introduced, deprecated, obsoleted, etc.
>>
>> The only reference for ATAPI command validity is by checking that
>> version's PACKET feature set section.
>>
>> ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
>> therefore are assumed to have never been ATAPI commands.
>>
>> Mandatory commands, as listed in ATA8-ACS3, are:
>>
>> - DEVICE RESET
>> - EXECUTE DEVICE DIAGNOSTIC
>> - IDENTIFY DEVICE
>> - IDENTIFY PACKET DEVICE
>> - NOP
>> - PACKET
>> - READ SECTOR(S)
>> - SET FEATURES
>>
>> Optional commands as listed in ATA8-ACS3, are:
>>
>> - FLUSH CACHE
>> - READ LOG DMA EXT
>> - READ LOG EXT
>> - WRITE LOG DMA EXT
>> - WRITE LOG EXT
>>
>> All other commands are illegal to send to an ATAPI device and should
>> be rejected by the device.
> 
> We could perhaps argue about "should be rejected by the device", but I
> think the weaker "a device is free to reject it" still suffices to
> support your patch.
> 

Sure -- I suppose drives CAN support a superset if they want to. In my
mind, anything above the ATAPI spec should be justified directly with
"Guest X breaks without it."

>> CD_OK removal justifications:
>>
>> 0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
>> 0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
>> 0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
>> 0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
>> 0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
>> 0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
>> 0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
>> 0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
>> 0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
>> 0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.
> 
> Actual patch matches this list.
> 
>> This patch fixes a divide by zero fault that can be caused by sending
>> the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
>> it to attempt to use zeroed CHS values to perform sector arithmetic.
>>
>> Reported-by: Qinghao Tang 
>> Signed-off-by: John Snow 
> 
> I appreciate you going to the root of the problem instead of merely
> fixing the narrow bug.
> 
> Could a similar argument be made for dropping CFA_OK from some commands?
> 

Very likely, but that's another patch. I didn't audit that yet.

> Do we still need this conditional in cmd_read_native_max()?
> 
> /* Refuse if no sectors are addressable (e.g. medium not inserted) */
> if (s->nb_sectors == 0) {
> ide_abort_command(s);
> return true;
> }
> 
> Why does it fail at guarding the CHS use from empty ATAPI drives before
> your patch?
> 

Because I misunderstood the real reason myself, and my POC test was a
little bananas. This works *with* a CDROM inserted, not without.

So s->nb_sectors can be non-zero, and ide_set_sector then triggers e.g.
SIGFPE.

If you'll save me the re-spin, I can fix that part of the commit message
in my staging branch.

--js

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[John Snow]

On 09/11/2015 02:56 AM, Michael Tokarev wrote:
> 09.09.2015 19:28, John Snow wrote:
>> We're a little too lenient with what we'll let an ATAPI drive handle.
>> Clamp down on the IDE command execution table to remove CD_OK permissions
>> from commands that are not and have never been ATAPI commands.
> 
> FWIW, this issue has been assigned CVE-2015-6855 identifier, which
> can be reflected in the commit message when applying.  Since this
> issue has security impact, it might be a good idea to add
> 
> Cc: qemu-sta...@nongnu.org
> 
> Thanks,
> 
> /mjt
> 

I'm still awaiting review/acks, but would you like me to re-send this
patch to trivial, or just fwd/reply-to?

Thanks,
--js

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[Michael Tokarev]

14.09.2015 21:04, John Snow wrote:
> On 09/11/2015 02:56 AM, Michael Tokarev wrote:
>> 09.09.2015 19:28, John Snow wrote:
>>> We're a little too lenient with what we'll let an ATAPI drive handle.
>>> Clamp down on the IDE command execution table to remove CD_OK permissions
>>> from commands that are not and have never been ATAPI commands.
>>
>> FWIW, this issue has been assigned CVE-2015-6855 identifier, which
>> can be reflected in the commit message when applying.  Since this
>> issue has security impact, it might be a good idea to add
>>
>> Cc: qemu-sta...@nongnu.org
> 
> I'm still awaiting review/acks, but would you like me to re-send this
> patch to trivial, or just fwd/reply-to?

I think it is anything but trivial ;)  Well, the semantics is trivial,
but it isn't -trivial material per se.

I suggested add a Cc to qemu-stable, this can be done at commit,
together with mentioning the CVE# assigned meanwhile, and I don't
know who will commit it, Kevin?

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[John Snow]

On 09/14/2015 02:43 PM, Michael Tokarev wrote:
> 14.09.2015 21:04, John Snow wrote:
>> On 09/11/2015 02:56 AM, Michael Tokarev wrote:
>>> 09.09.2015 19:28, John Snow wrote:
 We're a little too lenient with what we'll let an ATAPI drive handle.
 Clamp down on the IDE command execution table to remove CD_OK permissions
 from commands that are not and have never been ATAPI commands.
>>>
>>> FWIW, this issue has been assigned CVE-2015-6855 identifier, which
>>> can be reflected in the commit message when applying.  Since this
>>> issue has security impact, it might be a good idea to add
>>>
>>> Cc: qemu-sta...@nongnu.org
>>
>> I'm still awaiting review/acks, but would you like me to re-send this
>> patch to trivial, or just fwd/reply-to?
> 
> I think it is anything but trivial ;)  Well, the semantics is trivial,
> but it isn't -trivial material per se.
> 
> I suggested add a Cc to qemu-stable, this can be done at commit,
> together with mentioning the CVE# assigned meanwhile, and I don't
> know who will commit it, Kevin?
> 
> Thanks,
> 
> /mjt
> 

I'll be sending the pullreq through my tree, but I was waiting on at
least an ACK or so before I went ahead.

I can add CC: qemu-stable to the patch on-pull if that's sufficient for you.

--js

Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[Michael Tokarev]

09.09.2015 19:28, John Snow wrote:
> We're a little too lenient with what we'll let an ATAPI drive handle.
> Clamp down on the IDE command execution table to remove CD_OK permissions
> from commands that are not and have never been ATAPI commands.

FWIW, this issue has been assigned CVE-2015-6855 identifier, which
can be reflected in the commit message when applying.  Since this
issue has security impact, it might be a good idea to add

Cc: qemu-sta...@nongnu.org

Thanks,

/mjt

[Qemu-devel] [PATCH] ide: fix ATAPI command permissions

[John Snow]

We're a little too lenient with what we'll let an ATAPI drive handle.
Clamp down on the IDE command execution table to remove CD_OK permissions
from commands that are not and have never been ATAPI commands.

For ATAPI command validity, please see:
- ATA4 Section 6.5 ("PACKET Command feature set")
- ATA8/ACS Section 4.3 ("The PACKET feature set")
- ACS3 Section 4.3 ("The PACKET feature set")

ACS3 has a historical command validity table in Table B.4
("Historical Command Assignments") that can be referenced to find when
a command was introduced, deprecated, obsoleted, etc.

The only reference for ATAPI command validity is by checking that
version's PACKET feature set section.

ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
therefore are assumed to have never been ATAPI commands.

Mandatory commands, as listed in ATA8-ACS3, are:

- DEVICE RESET
- EXECUTE DEVICE DIAGNOSTIC
- IDENTIFY DEVICE
- IDENTIFY PACKET DEVICE
- NOP
- PACKET
- READ SECTOR(S)
- SET FEATURES

Optional commands as listed in ATA8-ACS3, are:

- FLUSH CACHE
- READ LOG DMA EXT
- READ LOG EXT
- WRITE LOG DMA EXT
- WRITE LOG EXT

All other commands are illegal to send to an ATAPI device and should
be rejected by the device.

CD_OK removal justifications:

0x06 WIN_DSM  Defined in ACS2. Not valid for ATAPI.
0x21 WIN_READ_ONCERetired in ATA5. Not ATAPI in ATA4.
0x94 WIN_STANDBYNOW2  Retired in ATA4. Did not coexist with ATAPI.
0x95 WIN_IDLEIMMEDIATE2   Retired in ATA4. Did not coexist with ATAPI.
0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
0x98 WIN_CHECKPOWERMODE2  Retired in ATA4. Did not coexist with ATAPI.
0x99 WIN_SLEEPNOW2Retired in ATA4. Did not coexist with ATAPI.
0xE0 WIN_STANDBYNOW1  Not part of ATAPI in ATA4, ACS or ACS3.
0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
0xE2 WIN_STANDBY  Not part of ATAPI in ATA4, ACS or ACS3.
0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
0xE4 WIN_CHECKPOWERMODE1  Not part of ATAPI in ATA4, ACS or ACS3.
0xE5 WIN_SLEEPNOW1Not part of ATAPI in ATA4, ACS or ACS3.
0xF8 WIN_READ_NATIVE_MAX  Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.

This patch fixes a divide by zero fault that can be caused by sending
the WIN_READ_NATIVE_MAX command to an empty ATAPI drive, which causes
it to attempt to use zeroed CHS values to perform sector arithmetic.

Reported-by: Qinghao Tang 
Signed-off-by: John Snow 
---
 hw/ide/core.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 50449ca..71caea9 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1747,11 +1747,11 @@ static const struct {
 } ide_cmd_table[0x100] = {
 /* NOP not implemented, mandatory for CD */
 [CFA_REQ_EXT_ERROR_CODE]  = { cmd_cfa_req_ext_error_code, CFA_OK },
-[WIN_DSM] = { cmd_data_set_management, ALL_OK },
+[WIN_DSM] = { cmd_data_set_management, HD_CFA_OK },
 [WIN_DEVICE_RESET]= { cmd_device_reset, CD_OK },
 [WIN_RECAL]   = { cmd_nop, HD_CFA_OK | SET_DSC},
 [WIN_READ]= { cmd_read_pio, ALL_OK },
-[WIN_READ_ONCE]   = { cmd_read_pio, ALL_OK },
+[WIN_READ_ONCE]   = { cmd_read_pio, HD_CFA_OK },
 [WIN_READ_EXT]= { cmd_read_pio, HD_CFA_OK },
 [WIN_READDMA_EXT] = { cmd_read_dma, HD_CFA_OK },
 [WIN_READ_NATIVE_MAX_EXT] = { cmd_read_native_max, HD_CFA_OK | SET_DSC 
},
@@ -1770,12 +1770,12 @@ static const struct {
 [CFA_TRANSLATE_SECTOR]= { cmd_cfa_translate_sector, CFA_OK },
 [WIN_DIAGNOSE]= { cmd_exec_dev_diagnostic, ALL_OK },
 [WIN_SPECIFY] = { cmd_nop, HD_CFA_OK | SET_DSC },
-[WIN_STANDBYNOW2] = { cmd_nop, ALL_OK },
-[WIN_IDLEIMMEDIATE2]  = { cmd_nop, ALL_OK },
-[WIN_STANDBY2]= { cmd_nop, ALL_OK },
-[WIN_SETIDLE2]= { cmd_nop, ALL_OK },
-[WIN_CHECKPOWERMODE2] = { cmd_check_power_mode, ALL_OK | SET_DSC },
-[WIN_SLEEPNOW2]   = { cmd_nop, ALL_OK },
+[WIN_STANDBYNOW2] = { cmd_nop, HD_CFA_OK },
+[WIN_IDLEIMMEDIATE2]  = { cmd_nop, HD_CFA_OK },
+[WIN_STANDBY2]= { cmd_nop, HD_CFA_OK },
+[WIN_SETIDLE2]= { cmd_nop, HD_CFA_OK },
+[WIN_CHECKPOWERMODE2] = { cmd_check_power_mode, HD_CFA_OK | 
SET_DSC },
+[WIN_SLEEPNOW2]   = { cmd_nop, HD_CFA_OK },
 [WIN_PACKETCMD]   = { cmd_packet, CD_OK },
 [WIN_PIDENTIFY]   = { cmd_identify_packet, CD_OK },
 [WIN_SMART]   = { cmd_smart, HD_CFA_OK | SET_DSC },
@@ -1789,19 +1789,19 @@ static const struct {
 [WIN_WRITEDMA]