John Snow <js...@redhat.com> writes:
> On 09/15/2015 02:11 PM, Markus Armbruster wrote:
>> John Snow <js...@redhat.com> writes:
>>
>>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>>>> John Snow <js...@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 <luodalon...@gmail.com>
>>>>> Signed-off-by: John Snow <js...@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 <arm...@redhat.com>
