From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60019) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVUde-0000ds-AI for qemu-devel@nongnu.org; Thu, 25 Apr 2013 18:27:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVUdc-0007yM-UY for qemu-devel@nongnu.org; Thu, 25 Apr 2013 18:27:38 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:41999) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVUdc-0007y7-DU for qemu-devel@nongnu.org; Thu, 25 Apr 2013 18:27:36 -0400 Received: from /spool/local by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 26 Apr 2013 08:21:32 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 8EE3F2CE804A for ; Fri, 26 Apr 2013 08:27:27 +1000 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3PMDlLB16842814 for ; Fri, 26 Apr 2013 08:13:50 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3PMROtL016217 for ; Fri, 26 Apr 2013 08:27:24 +1000 From: Anthony Liguori In-Reply-To: <20130425210242.GB2908@redhat.com> References: <1366875807-3491-1-git-send-email-jasowang@redhat.com> <87fvyebbwb.fsf@codemonkey.ws> <20130425210242.GB2908@redhat.com> Date: Thu, 25 Apr 2013 17:27:19 -0500 Message-ID: <878v461c1k.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH] virtio: abort on zero config length List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Jason Wang , qemu-devel@nongnu.org "Michael S. Tsirkin" writes: > On Thu, Apr 25, 2013 at 03:20:20PM -0500, Anthony Liguori wrote: >> Jason Wang writes: >> >> > In fact we don't support zero length config length for virtio device. >> >> virtio-rng? > > It has config_len == 0? In that case guest using virtio-rng can crash > qemu or read qemu memory: > > uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) > { > uint8_t val; > > vdev->get_config(vdev, vdev->config); > > if (addr > (vdev->config_len - sizeof(val))) > > ^^^^^^^^^ quiz: spot a bug above if config_len is 0 :) Then we need to fix these bugs and allocate a CVE. virtio-rng has shipped. This code is also dumb. There's no requirement that config_len is >= 4 so this would fail equally awfully with config_len=1|2|3. Regards, Anthony Liguori > > > return (uint32_t)-1; > > val = ldub_p(vdev->config + addr); > return val; > } > > how about corrupting qemu memory? That too: > > void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data) > { > uint8_t val = data; > > if (addr > (vdev->config_len - sizeof(val))) > return; > > stb_p(vdev->config + addr, val); > > if (vdev->set_config) > vdev->set_config(vdev, vdev->config); > } > > > >> > And it can lead outbound memory access. So abort on zero config length >> > to catch the bug earlier. >> >> Not sure what you mean, but virtio-rng has a zero length config space. >> >> Regards, >> >> Anthony Liguori >> >> > >> > Signed-off-by: Jason Wang >> > --- >> > hw/virtio/virtio.c | 7 ++----- >> > 1 files changed, 2 insertions(+), 5 deletions(-) >> > >> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> > index 1c2282c..a6fa667 100644 >> > --- a/hw/virtio/virtio.c >> > +++ b/hw/virtio/virtio.c >> > @@ -923,6 +923,7 @@ void virtio_init(VirtIODevice *vdev, const char *name, >> > uint16_t device_id, size_t config_size) >> > { >> > int i; >> > + assert(config_size); >> > vdev->device_id = device_id; >> > vdev->status = 0; >> > vdev->isr = 0; >> > @@ -938,11 +939,7 @@ void virtio_init(VirtIODevice *vdev, const char *name, >> > >> > vdev->name = name; >> > vdev->config_len = config_size; >> > - if (vdev->config_len) { >> > - vdev->config = g_malloc0(config_size); >> > - } else { >> > - vdev->config = NULL; >> > - } >> > + vdev->config = g_malloc0(config_size); >> > vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change, >> > vdev); >> > } >> > -- >> > 1.7.1