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

[John Snow <jsnow@redhat.com>]

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