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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 09/15/2015 02:11 PM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>> 
>>> On 09/15/2015 02:53 AM, Markus Armbruster wrote:
>>>> John Snow <address@hidden> 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 <address@hidden>
>>>>> Signed-off-by: John Snow <address@hidden>
>>>>
>>>> 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 <address@hidden>