From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bgqhU-0002ZV-As for qemu-devel@nongnu.org; Mon, 05 Sep 2016 06:00:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bgqhO-0005GD-9c for qemu-devel@nongnu.org; Mon, 05 Sep 2016 06:00:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33892) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bgqhO-0005G6-3c for qemu-devel@nongnu.org; Mon, 05 Sep 2016 06:00:18 -0400 References: <1472884428-9975-1-git-send-email-ppandit@redhat.com> From: Paolo Bonzini Message-ID: Date: Mon, 5 Sep 2016 12:00:14 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] scsi: pvscsi: request descriptor data_length to 32 bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qemu Developers , Dmitry Fleytman , Li Qiang On 05/09/2016 11:50, P J P wrote: > Hello Paolo, all > > +-- On Mon, 5 Sep 2016, Paolo Bonzini wrote --+ > | > - uint64_t data_length = r->req.dataLen; > | > + uint32_t data_length = r->req.dataLen; > | > | Why is this needed if you remove the cast in MIN, below? > > The outer while loop below is controlled by 'data_length'. The cast in MIN > truncates a large(64bit) value of 'data_length' to zero(0), thus setting > 'chunk_size' to zero, which results in infinite loop as 'data_length' remains > unchanged(> 0). > > Second, Removing cast below results in 'chunk_size' being set to 'sg.resid', > for large(>32bit) values of 'data_length'. Which results in an infinite loop > because the inner 'while(!sg.resid)' loop takes forever to read non-zero > values into 'sg.resid'. No, that's not what happens. chunk_size is set to sg.resid, after which: sg.dataAddr += chunk_size; data_length -= chunk_size; sg.resid -= chunk_size; The loop is reentered with sg.resid == 0, it calls into pvscsi_get_next_sg_elem and this sets sg.resid to a nonzero value. It's not an infinite loop. > Above type change ensures that outer while loop is not entered if > 'data_length' is zero. And removing cast ensures that inner while(!sg.resid) > loop does not have run forever, ie. till large(64 bit) 'data_length' becomes > zero. > > Looking at the 'vmw_pvscsi.c' Linux kernel driver, 'dataLen' seems to be set > to an unsigned 32 bit 'bufflen' value. The driver is irrelevant. If the data_length is an uint64_t you need to ensure that a 64 bit buffer is processed correctly. Here you are truncating it, which is wrong and will cause a buffer underrun. Paolo