* [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
@ 2015-09-09 16:28 John Snow
2015-09-11 6:56 ` Michael Tokarev
2015-09-15 6:53 ` Markus Armbruster
0 siblings, 2 replies; 12+ messages in thread
From: John Snow @ 2015-09-09 16:28 UTC (permalink / raw
To: qemu-block
Cc: kwolf, stefano.stabellini, armbru, qemu-devel, ppandit,
luodalongde, liuling-it, 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
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] = { cmd_write_dma, HD_CFA_OK },
[WIN_WRITEDMA_ONCE] = { cmd_write_dma, HD_CFA_OK },
[CFA_WRITE_MULTI_WO_ERASE] = { cmd_write_multiple, CFA_OK },
- [WIN_STANDBYNOW1] = { cmd_nop, ALL_OK },
- [WIN_IDLEIMMEDIATE] = { cmd_nop, ALL_OK },
- [WIN_STANDBY] = { cmd_nop, ALL_OK },
- [WIN_SETIDLE1] = { cmd_nop, ALL_OK },
- [WIN_CHECKPOWERMODE1] = { cmd_check_power_mode, ALL_OK | SET_DSC },
- [WIN_SLEEPNOW1] = { cmd_nop, ALL_OK },
+ [WIN_STANDBYNOW1] = { cmd_nop, HD_CFA_OK },
+ [WIN_IDLEIMMEDIATE] = { cmd_nop, HD_CFA_OK },
+ [WIN_STANDBY] = { cmd_nop, HD_CFA_OK },
+ [WIN_SETIDLE1] = { cmd_nop, HD_CFA_OK },
+ [WIN_CHECKPOWERMODE1] = { cmd_check_power_mode, HD_CFA_OK | SET_DSC },
+ [WIN_SLEEPNOW1] = { cmd_nop, HD_CFA_OK },
[WIN_FLUSH_CACHE] = { cmd_flush_cache, ALL_OK },
[WIN_FLUSH_CACHE_EXT] = { cmd_flush_cache, HD_CFA_OK },
[WIN_IDENTIFY] = { cmd_identify, ALL_OK },
[WIN_SETFEATURES] = { cmd_set_features, ALL_OK | SET_DSC },
[IBM_SENSE_CONDITION] = { cmd_ibm_sense_condition, CFA_OK | SET_DSC },
[CFA_WEAR_LEVEL] = { cmd_cfa_erase_sectors, HD_CFA_OK | SET_DSC },
- [WIN_READ_NATIVE_MAX] = { cmd_read_native_max, ALL_OK | SET_DSC },
+ [WIN_READ_NATIVE_MAX] = { cmd_read_native_max, HD_CFA_OK | SET_DSC },
};
static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
--
2.4.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-09 16:28 [Qemu-devel] [PATCH] ide: fix ATAPI command permissions John Snow
@ 2015-09-11 6:56 ` Michael Tokarev
2015-09-14 18:04 ` John Snow
2015-09-15 6:53 ` Markus Armbruster
1 sibling, 1 reply; 12+ messages in thread
From: Michael Tokarev @ 2015-09-11 6:56 UTC (permalink / raw
To: John Snow, qemu-block
Cc: kwolf, stefano.stabellini, armbru, qemu-devel, ppandit,
luodalongde, liuling-it
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-stable@nongnu.org
Thanks,
/mjt
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-11 6:56 ` Michael Tokarev
@ 2015-09-14 18:04 ` John Snow
2015-09-14 18:43 ` Michael Tokarev
0 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2015-09-14 18:04 UTC (permalink / raw
To: Michael Tokarev, qemu-block
Cc: kwolf, stefano.stabellini, armbru, qemu-devel, ppandit,
luodalongde, liuling-it
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-stable@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-14 18:04 ` John Snow
@ 2015-09-14 18:43 ` Michael Tokarev
2015-09-14 18:49 ` John Snow
0 siblings, 1 reply; 12+ messages in thread
From: Michael Tokarev @ 2015-09-14 18:43 UTC (permalink / raw
To: John Snow, qemu-block
Cc: kwolf, stefano.stabellini, armbru, qemu-devel, ppandit,
luodalongde, liuling-it
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-stable@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-14 18:43 ` Michael Tokarev
@ 2015-09-14 18:49 ` John Snow
2015-09-15 8:26 ` Kevin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2015-09-14 18:49 UTC (permalink / raw
To: Michael Tokarev, qemu-block
Cc: kwolf, stefano.stabellini, armbru, qemu-devel, ppandit,
luodalongde, liuling-it
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-stable@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-09 16:28 [Qemu-devel] [PATCH] ide: fix ATAPI command permissions John Snow
2015-09-11 6:56 ` Michael Tokarev
@ 2015-09-15 6:53 ` Markus Armbruster
2015-09-15 16:42 ` John Snow
1 sibling, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2015-09-15 6:53 UTC (permalink / raw
To: John Snow
Cc: kwolf, qemu-block, stefano.stabellini, qemu-devel, ppandit,
luodalongde, liuling-it
John Snow <jsnow@redhat.com> 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-14 18:49 ` John Snow
@ 2015-09-15 8:26 ` Kevin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2015-09-15 8:26 UTC (permalink / raw
To: John Snow
Cc: qemu-block, stefano.stabellini, armbru, qemu-stable,
Michael Tokarev, qemu-devel, ppandit, luodalongde, liuling-it
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-stable@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-stable@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-15 6:53 ` Markus Armbruster
@ 2015-09-15 16:42 ` John Snow
2015-09-15 18:11 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2015-09-15 16:42 UTC (permalink / raw
To: Markus Armbruster
Cc: kwolf, qemu-block, stefano.stabellini, qemu-devel, ppandit,
luodalongde, liuling-it
On 09/15/2015 02:53 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-15 16:42 ` John Snow
@ 2015-09-15 18:11 ` Markus Armbruster
2015-09-15 18:22 ` John Snow
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2015-09-15 18:11 UTC (permalink / raw
To: John Snow
Cc: kwolf, qemu-block, stefano.stabellini, qemu-devel, ppandit,
luodalongde, liuling-it
John Snow <jsnow@redhat.com> writes:
> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> 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(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
if (err) {
error_report_err(err);
return -1;
}
}
Therefore, CHS is all zero, and the code using it blows up.
Correct?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-15 18:11 ` Markus Armbruster
@ 2015-09-15 18:22 ` John Snow
2015-09-15 18:50 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2015-09-15 18:22 UTC (permalink / raw
To: Markus Armbruster
Cc: kwolf, qemu-block, stefano.stabellini, qemu-devel, ppandit,
luodalongde, liuling-it
On 09/15/2015 02:11 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> 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(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
> if (err) {
> error_report_err(err);
> return -1;
> }
> }
>
> Therefore, CHS is all zero, and the code using it blows up.
>
> Correct?
>
Indeed. I had wrongly assumed previously that the CHS values actually
did get initialized (incorrectly) if a CD was present at boot, but I was
wrong.
Coincidentally, the patch is still correct regardless of my wrong
assumption. :)
--js
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-15 18:22 ` John Snow
@ 2015-09-15 18:50 ` Markus Armbruster
2015-09-15 19:12 ` John Snow
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2015-09-15 18:50 UTC (permalink / raw
To: John Snow
Cc: kwolf, qemu-block, stefano.stabellini, qemu-devel, ppandit,
luodalongde, liuling-it
John Snow <jsnow@redhat.com> writes:
> On 09/15/2015 02:11 PM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>
>>>> 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(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>> if (err) {
>> error_report_err(err);
>> return -1;
>> }
>> }
>>
>> Therefore, CHS is all zero, and the code using it blows up.
>>
>> Correct?
>>
>
> Indeed. I had wrongly assumed previously that the CHS values actually
> did get initialized (incorrectly) if a CD was present at boot, but I was
> wrong.
>
> Coincidentally, the patch is still correct regardless of my wrong
> assumption. :)
With a corrected commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
2015-09-15 18:50 ` Markus Armbruster
@ 2015-09-15 19:12 ` John Snow
0 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2015-09-15 19:12 UTC (permalink / raw
To: Markus Armbruster
Cc: kwolf, qemu-block, stefano.stabellini, qemu-devel, ppandit,
luodalongde, liuling-it
On 09/15/2015 02:50 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 09/15/2015 02:11 PM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>>>>> John Snow <jsnow@redhat.com> 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_ONCE Retired 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_SLEEPNOW2 Retired 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_SLEEPNOW1 Not 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 <luodalongde@gmail.com>
>>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>>
>>>>> 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(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>>> if (err) {
>>> error_report_err(err);
>>> return -1;
>>> }
>>> }
>>>
>>> Therefore, CHS is all zero, and the code using it blows up.
>>>
>>> Correct?
>>>
>>
>> Indeed. I had wrongly assumed previously that the CHS values actually
>> did get initialized (incorrectly) if a CD was present at boot, but I was
>> wrong.
>>
>> Coincidentally, the patch is still correct regardless of my wrong
>> assumption. :)
>
> With a corrected commit message:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-09-15 19:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-09 16:28 [Qemu-devel] [PATCH] ide: fix ATAPI command permissions John Snow
2015-09-11 6:56 ` Michael Tokarev
2015-09-14 18:04 ` John Snow
2015-09-14 18:43 ` Michael Tokarev
2015-09-14 18:49 ` John Snow
2015-09-15 8:26 ` Kevin Wolf
2015-09-15 6:53 ` Markus Armbruster
2015-09-15 16:42 ` John Snow
2015-09-15 18:11 ` Markus Armbruster
2015-09-15 18:22 ` John Snow
2015-09-15 18:50 ` Markus Armbruster
2015-09-15 19:12 ` John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).