From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjTdS-0001pW-Hw for qemu-devel@nongnu.org; Tue, 06 Oct 2015 10:54:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZjTdO-0001bm-Cs for qemu-devel@nongnu.org; Tue, 06 Oct 2015 10:54:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjTdO-0001bX-6B for qemu-devel@nongnu.org; Tue, 06 Oct 2015 10:54:30 -0400 Date: Tue, 6 Oct 2015 16:54:24 +0200 From: Marc =?UTF-8?B?TWFyw60=?= Message-ID: <20151006165424.42126747@markmb_rh> In-Reply-To: <20151006144453.GA28347@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> 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: Drew , "Gabriel L. Somlo" , qemu-devel@nongnu.org, Kevin O'Connor , Gerd Hoffmann , Laszlo On Tue, 6 Oct 2015 15:44:53 +0100 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); > > } > > =20 > > +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)); >=20 > 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 > Instead, please use: >=20 > 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); > return; > } >=20 > dma.address =3D be64_to_cpu(dma.address); > dma.length =3D be32_to_cpu(dma.length); > dma.control =3D be32_to_cpu(dma.control); >=20 > > + > > + if (dma.control & FW_CFG_DMA_CTL_SELECT) { > > + fw_cfg_select(s, dma.control >> 16); > > + } > > + > > + arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > > + e =3D &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > > + > > + if (dma.control & FW_CFG_DMA_CTL_READ) { > > + read =3D 1; > > + } else if (dma.control & FW_CFG_DMA_CTL_SKIP) { > > + read =3D 0; > > + } else { > > + dma.length =3D 0; > > + } > > + > > + dma.control =3D 0; > > + > > + while (dma.length > 0 && !(dma.control & > > FW_CFG_DMA_CTL_ERROR)) { > > + if (s->cur_entry =3D=3D FW_CFG_INVALID || !e->data || > > + s->cur_offset >=3D e->len) { > > + len =3D dma.length; > > + > > + /* If the access is not a read access, it will be a > > skip access, > > + * tested before. > > + */ > > + if (read) { > > + if (dma_memory_set(s->dma_as, dma.address, 0, > > len)) { > > + dma.control |=3D FW_CFG_DMA_CTL_ERROR; > > + } > > + } > > + > > + } else { > > + if (dma.length <=3D (e->len - s->cur_offset)) { > > + len =3D dma.length; > > + } else { > > + len =3D (e->len - s->cur_offset); > > + } > > + > > + if (e->read_callback) { > > + e->read_callback(e->callback_opaque, > > s->cur_offset); > > + } > > + > > + /* If the access is not a read access, it will be a > > skip access, > > + * tested before. > > + */ > > + if (read) { > > + if (dma_memory_write(s->dma_as, dma.address, > > + &e->data[s->cur_offset], len)) > > { > > + dma.control |=3D FW_CFG_DMA_CTL_ERROR; > > + } > > + } > > + > > + s->cur_offset +=3D len; > > + } > > + > > + dma.address +=3D len; > > + dma.length -=3D len; >=20 > I thought these fields are written back to guest memory? For example, > so the guest knows how many bytes were read before the error occurred. This was proposed here: http://lists.gnu.org/archive/html/qemu-devel/2015-08/msg04001.html I also don't see much benefit into knowing how many bytes were read. If the guest is trying to read an invalid entry or past the end of that entry, the memory will be filled with zeros. The only moment when an error would be reported is when there's some problem in the mapping. And this problem is bad enough to just abort the whole operation. Regards Marc