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. :)
--js
