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