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

[John Snow]

On 09/15/2015 02:50 PM, Markus Armbruster wrote:
> 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>
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js