From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7c7A-0007pV-BL for qemu-devel@nongnu.org; Tue, 31 May 2016 01:21:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b7c74-0004hx-B6 for qemu-devel@nongnu.org; Tue, 31 May 2016 01:21:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37139) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7c74-0004ht-5k for qemu-devel@nongnu.org; Tue, 31 May 2016 01:21:10 -0400 Date: Tue, 31 May 2016 10:50:57 +0530 (IST) From: P J P In-Reply-To: Message-ID: References: <1464634728-8723-1-git-send-email-ppandit@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Subject: Re: [Qemu-devel] [PATCH] scsi: esp: check TI buffer index before read/write List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Qemu Developers , Paolo Bonzini , Huawei PSIRT Hello Peter, +-- On Mon, 30 May 2016, Peter Maydell wrote --+ | > + } else if (s->ti_rptr < TI_BUFSZ) { | > s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++]; | > + } else { | > + trace_esp_error_fifo_overrun(); | | Isn't this an underrun, not an overrun? OOB read occurs when 's->ti_rptr' exceeds 'TI_BUFSZ'. | In any case, something weird seems to be going on here: | it looks like the amount of data in the fifo should be | constrained by ti_size (which we're already checking), so | when can we get into a situation where ti_rptr can | get beyond the buffer size? It seems to me that we should | fix whatever that underlying bug is, and then have an | assert() on ti_rptr here. | | > - } else if (s->ti_size == TI_BUFSZ - 1) { | > + } else if (s->ti_wptr == TI_BUFSZ - 1) { | > trace_esp_error_fifo_overrun(); | | Similarly, this looks odd -- the ti_size check should be | sufficient if the rest of the code is correctly managing | the ti_size/ti_wptr/ti_rptr values. Both issues occur as guest could control the value of 's->ti_size' by alternating between 'esp_reg_write(, ESP_FIFO, )' & 'esp_reg_read(, ESP_FIFO)' calls. One increases 's->ti_size' and other descreases the same. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F