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 08:53:07 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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.

> 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?

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?



reply via email to

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