From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43708) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbusK-0007TA-PT for qemu-devel@nongnu.org; Tue, 15 Sep 2015 14:22:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbusJ-0000VR-Gw for qemu-devel@nongnu.org; Tue, 15 Sep 2015 14:22:40 -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> From: John Snow Message-ID: <55F861E6.4090500@redhat.com> Date: Tue, 15 Sep 2015 14:22:30 -0400 MIME-Version: 1.0 In-Reply-To: <87zj0n5z91.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: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. :) --js