qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions
Date: Tue, 15 Sep 2015 20:11:22 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]