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

[John Snow]

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.

--js