From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zbvf3-0006Zq-JM for qemu-devel@nongnu.org; Tue, 15 Sep 2015 15:13:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zbvf2-0005lU-0b for qemu-devel@nongnu.org; Tue, 15 Sep 2015 15:13:01 -0400 References: <1441816082-21031-1-git-send-email-jsnow@redhat.com> <87r3m0897w.fsf@blackfin.pond.sub.org> <55F84A72.3000806@redhat.com> <87zj0n5z91.fsf@blackfin.pond.sub.org> <55F861E6.4090500@redhat.com> <87oah34iut.fsf@blackfin.pond.sub.org> From: John Snow Message-ID: <55F86DB2.8060900@redhat.com> Date: Tue, 15 Sep 2015 15:12:50 -0400 MIME-Version: 1.0 In-Reply-To: <87oah34iut.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI command permissions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-block@nongnu.org, stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, ppandit@redhat.com, luodalongde@gmail.com, liuling-it@360.cn On 09/15/2015 02:50 PM, Markus Armbruster wrote: > John Snow writes: > >> On 09/15/2015 02:11 PM, Markus Armbruster wrote: >>> John Snow writes: >>> >>>> On 09/15/2015 02:53 AM, Markus Armbruster wrote: >>>>> John Snow 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 >>>>>> Signed-off-by: John Snow >>>>> >>>>> 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. :) > > With a corrected commit message: > Reviewed-by: Markus Armbruster > Thanks, applied to my IDE tree: https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git --js