From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk7Ar-0005Pq-De for qemu-devel@nongnu.org; Thu, 08 Oct 2015 05:07:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zk7Ao-0003Pj-4g for qemu-devel@nongnu.org; Thu, 08 Oct 2015 05:07:41 -0400 Received: from mail-wi0-x230.google.com ([2a00:1450:400c:c05::230]:36025) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk7An-0003Pf-UA for qemu-devel@nongnu.org; Thu, 08 Oct 2015 05:07:38 -0400 Received: by wicgb1 with SMTP id gb1so15911828wic.1 for ; Thu, 08 Oct 2015 02:07:37 -0700 (PDT) Date: Thu, 8 Oct 2015 10:07:34 +0100 From: Stefan Hajnoczi Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: Peter Maydell Cc: Drew , "Gabriel L. Somlo" , QEMU Developers , Kevin O'Connor , Gerd Hoffmann , Marc =?iso-8859-1?Q?Mar=ED?= , Laszlo 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í 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 = s->dma_addr; > >> + s->dma_addr = 0; > >> + > >> + dma.address = ldq_be_dma(s->dma_as, > >> + dma_addr + offsetof(FWCfgDmaAccess, address)); > >> + dma.length = ldl_be_dma(s->dma_as, > >> + dma_addr + offsetof(FWCfgDmaAccess, length)); > >> + dma.control = 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! > > 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. Even if the guest-visible behavior doesn't matter, Valgrind won't like this. ldq_be_dma() reads from uninitialized stack memory: #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); \ } Bad QEMU, bad userspace process :). I think we really need to check the error and at least return early. > > 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); > > If the guest handed us a bad dma_addr then this write will also > be bogus and could corrupt the guest's memory. That's fine because it's not a random address - it's the address that the guest gave us. Stefan