From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37416) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk81k-0007VK-5k for qemu-devel@nongnu.org; Thu, 08 Oct 2015 06:02:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zk81E-000750-G3 for qemu-devel@nongnu.org; Thu, 08 Oct 2015 06:02:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43461) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk81E-00074U-9Q for qemu-devel@nongnu.org; Thu, 08 Oct 2015 06:01:48 -0400 Date: Thu, 8 Oct 2015 12:01:42 +0200 From: Marc =?UTF-8?B?TWFyw60=?= Message-ID: <20151008120142.285e46fb@markmb_rh> In-Reply-To: <20151008090734.GB29433@stefanha-thinkpad.redhat.com> References: <1443701677-13629-1-git-send-email-markmb@redhat.com> <1443701819-13855-1-git-send-email-markmb@redhat.com> <1443701819-13855-4-git-send-email-markmb@redhat.com> <20151006144453.GA28347@stefanha-thinkpad.redhat.com> <20151008090734.GB29433@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 3/7] Implement fw_cfg DMA interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Peter Maydell , Drew , "Gabriel L. Somlo" , QEMU Developers , Kevin O'Connor , Gerd Hoffmann , Laszlo On Thu, 8 Oct 2015 10:07:34 +0100 Stefan Hajnoczi wrote: > On Tue, Oct 06, 2015 at 03:53:33PM +0100, Peter Maydell wrote: > > On 6 October 2015 at 15:44, Stefan Hajnoczi > > wrote: > > > On Thu, Oct 01, 2015 at 02:16:55PM +0200, Marc Mar=C3=AD wrote: > > >> @@ -292,6 +307,119 @@ static void fw_cfg_data_mem_write(void > > >> *opaque, hwaddr addr, } while (i); > > >> } > > >> > > >> +static void fw_cfg_dma_transfer(FWCfgState *s) > > >> +{ > > >> + dma_addr_t len; > > >> + FWCfgDmaAccess dma; > > >> + int arch; > > >> + FWCfgEntry *e; > > >> + int read; > > >> + dma_addr_t dma_addr; > > >> + > > >> + /* Reset the address before the next access */ > > >> + dma_addr =3D s->dma_addr; > > >> + s->dma_addr =3D 0; > > >> + > > >> + dma.address =3D ldq_be_dma(s->dma_as, > > >> + dma_addr + offsetof(FWCfgDmaAccess, > > >> address)); > > >> + dma.length =3D ldl_be_dma(s->dma_as, > > >> + dma_addr + offsetof(FWCfgDmaAccess, > > >> length)); > > >> + dma.control =3D ldl_be_dma(s->dma_as, > > >> + dma_addr + offsetof(FWCfgDmaAccess, > > >> control)); > > > > > > ldq_be_dma() doesn't report errors. If dma_addr is invalid the > > > return value could be anything. Memory corruption inside the > > > guest is possible if the address/length/control values happen to > > > cause a memory read operation! > >=20 > > We discussed this in a previous revision. IMHO if the guest has > > passed us a bogus dma_addr it should expect memory corruption. > > We only need to be sure we don't allow a VM escape. >=20 > Even if the guest-visible behavior doesn't matter, Valgrind won't like > this. ldq_be_dma() reads from uninitialized stack memory: >=20 > #define DEFINE_LDST_DMA(_lname, _sname, _bits, _end) \ > static inline uint##_bits##_t > ld##_lname##_##_end##_dma(AddressSpace *as, \ dma_addr_t addr) \ > { \ > uint##_bits##_t > val; \ dma_memory_read(as, > addr, &val, (_bits) / 8); \ return > _end##_bits##_to_cpu(val); \ } >=20 > Bad QEMU, bad userspace process :). >=20 > I think we really need to check the error and at least return early. It doesn't hurt to check the error. I'll add it. Thanks Marc > > > Instead, please use: > > > > > > if (dma_memory_read(s->dma_as, dma_addr, &dma, sizeof(dma))) { > > > stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, > > > control), FW_CFG_DMA_CTL_ERROR); > >=20 > > If the guest handed us a bad dma_addr then this write will also > > be bogus and could corrupt the guest's memory. >=20 > That's fine because it's not a random address - it's the address that > the guest gave us. >=20 > Stefan