[Qemu-devel] [PATCH v2 0/2] vhost-scsi and ivshmem fixes for Coverity issues

[Qemu-devel] [PATCH v2 0/2] vhost-scsi and ivshmem fixes for Coverity issues

[Stefan Hajnoczi]

The following fixes address NULL pointer dereferences spotted by Coverity.

v2:
 * Dropped savevm.c popen(3) leak fix since Paolo has a better fix [eblake]

Stefan Hajnoczi (2):
  vhost-scsi: fix k->set_guest_notifiers() NULL dereference
  ivshmem: add missing error exit(2)

 hw/misc/ivshmem.c    | 1 +
 hw/scsi/vhost-scsi.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/2] vhost-scsi: fix k->set_guest_notifiers() NULL dereference

[Stefan Hajnoczi]

Coverity picked up a copy-paste bug.  In vhost_scsi_start() we check for
!k->set_guest_notifiers and error out.  The check probably got copied
but instead of erroring we actually use the function pointer!

Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Asias He <asias@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/vhost-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index d7a1c33..785e93f 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -123,7 +123,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     int ret = 0;
 
-    if (!k->set_guest_notifiers) {
+    if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
         if (ret < 0) {
                 error_report("vhost guest notifier cleanup failed: %d\n", ret);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/2] vhost-scsi: fix k->set_guest_notifiers() NULL dereference

[Paolo Bonzini]

Il 03/06/2013 10:58, Stefan Hajnoczi ha scritto:
> Coverity picked up a copy-paste bug.  In vhost_scsi_start() we check for
> !k->set_guest_notifiers and error out.  The check probably got copied
> but instead of erroring we actually use the function pointer!

Exactly. :)

Applied to scsi branch, thanks.

Paolo

> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Asias He <asias@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/scsi/vhost-scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index d7a1c33..785e93f 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -123,7 +123,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      int ret = 0;
>  
> -    if (!k->set_guest_notifiers) {
> +    if (k->set_guest_notifiers) {
>          ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>          if (ret < 0) {
>                  error_report("vhost guest notifier cleanup failed: %d\n", ret);
> 

[Qemu-devel] [PATCH v2 2/2] ivshmem: add missing error exit(2)

[Stefan Hajnoczi]

If the user fails to specify 'chardev' or 'shm' then we cannot continue.
Exit right away so that we don't invoke shm_open(3) with a NULL pointer.

It would be nice to replace exit(1) with error returns in the PCI device
.init() function, but leave that for another patch since exit(1) is
currently used elsewhere.

Spotted by Coverity.

Cc: Cam Macdonell <cam@cs.ualberta.ca>
Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a19a6d6..5658f73 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
         if (s->shmobj == NULL) {
             fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
+            exit(1);
         }
 
         IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 2/2] ivshmem: add missing error exit(2)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

On 06/03/2013 02:58 AM, Stefan Hajnoczi wrote:
> If the user fails to specify 'chardev' or 'shm' then we cannot continue.
> Exit right away so that we don't invoke shm_open(3) with a NULL pointer.
> 
> It would be nice to replace exit(1) with error returns in the PCI device
> .init() function, but leave that for another patch since exit(1) is
> currently used elsewhere.
> 
> Spotted by Coverity.
> 
> Cc: Cam Macdonell <cam@cs.ualberta.ca>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/misc/ivshmem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a19a6d6..5658f73 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>  
>          if (s->shmobj == NULL) {
>              fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
> +            exit(1);

Reviewed-by: Eric Blake <eblake@redhat.com>

However, I prefer exit(EXIT_FAILURE) rather than exit(1), to make it a
bit easier to grep for known failure exits.  Libvirt has a syntax
checker (taken from gnulib) that enforces such a style, if qemu would
like to adopt that style.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] ivshmem: add missing error exit(2)

[Stefan Hajnoczi]

On Mon, Jun 03, 2013 at 06:26:22AM -0600, Eric Blake wrote:
> On 06/03/2013 02:58 AM, Stefan Hajnoczi wrote:
> > If the user fails to specify 'chardev' or 'shm' then we cannot continue.
> > Exit right away so that we don't invoke shm_open(3) with a NULL pointer.
> > 
> > It would be nice to replace exit(1) with error returns in the PCI device
> > .init() function, but leave that for another patch since exit(1) is
> > currently used elsewhere.
> > 
> > Spotted by Coverity.
> > 
> > Cc: Cam Macdonell <cam@cs.ualberta.ca>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/misc/ivshmem.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index a19a6d6..5658f73 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
> >  
> >          if (s->shmobj == NULL) {
> >              fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
> > +            exit(1);
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> However, I prefer exit(EXIT_FAILURE) rather than exit(1), to make it a
> bit easier to grep for known failure exits.  Libvirt has a syntax
> checker (taken from gnulib) that enforces such a style, if qemu would
> like to adopt that style.

I like EXIT_FAILURE too but the rest of the file inconsistently uses
exit(1) and exit(-1).  I decided exit(1) was the least evil but the
longer term fix is to return instead of exiting.

Re: [Qemu-devel] [PATCH v2 2/2] ivshmem: add missing error exit(2)

[Andreas Färber]

Am 03.06.2013 15:11, schrieb Stefan Hajnoczi:
> On Mon, Jun 03, 2013 at 06:26:22AM -0600, Eric Blake wrote:
>> On 06/03/2013 02:58 AM, Stefan Hajnoczi wrote:
>>> If the user fails to specify 'chardev' or 'shm' then we cannot continue.
>>> Exit right away so that we don't invoke shm_open(3) with a NULL pointer.
>>>
>>> It would be nice to replace exit(1) with error returns in the PCI device
>>> .init() function, but leave that for another patch since exit(1) is
>>> currently used elsewhere.
>>>
>>> Spotted by Coverity.
>>>
>>> Cc: Cam Macdonell <cam@cs.ualberta.ca>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  hw/misc/ivshmem.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>> index a19a6d6..5658f73 100644
>>> --- a/hw/misc/ivshmem.c
>>> +++ b/hw/misc/ivshmem.c
>>> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>>  
>>>          if (s->shmobj == NULL) {
>>>              fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
>>> +            exit(1);
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> However, I prefer exit(EXIT_FAILURE) rather than exit(1), to make it a
>> bit easier to grep for known failure exits.  Libvirt has a syntax
>> checker (taken from gnulib) that enforces such a style, if qemu would
>> like to adopt that style.
> 
> I like EXIT_FAILURE too but the rest of the file inconsistently uses
> exit(1) and exit(-1).  I decided exit(1) was the least evil but the
> longer term fix is to return instead of exiting.

QOM realize would solve that, but similar to virtio the PCI qdev initfn
actually does something (allocating the config region among others) so
faces the same design questions of how to best inherit QOM methods as
currently being discussed. Shouldn't hold up a bug fix like this one.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 2/2] ivshmem: add missing error exit(2)

[Stefan Hajnoczi]

On Mon, Jun 03, 2013 at 10:58:31AM +0200, Stefan Hajnoczi wrote:
> If the user fails to specify 'chardev' or 'shm' then we cannot continue.
> Exit right away so that we don't invoke shm_open(3) with a NULL pointer.
> 
> It would be nice to replace exit(1) with error returns in the PCI device
> .init() function, but leave that for another patch since exit(1) is
> currently used elsewhere.
> 
> Spotted by Coverity.
> 
> Cc: Cam Macdonell <cam@cs.ualberta.ca>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/misc/ivshmem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a19a6d6..5658f73 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>  
>          if (s->shmobj == NULL) {
>              fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
> +            exit(1);
>          }
>  
>          IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
> -- 
> 1.8.1.4
> 

Ping?

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 2/2] ivshmem: add missing error exit(2)

[Michael Tokarev]

12.06.2013 11:22, Stefan Hajnoczi wrote:
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index a19a6d6..5658f73 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>  
>>          if (s->shmobj == NULL) {
>>              fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
>> +            exit(1);
>>          }
>>  
>>          IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
> 
> Ping?

Ow.  I missed it because part 1/2 were applied to scsi branch by
Paolo and I marked whole thread as "done".

Applied this 2/2 now to the trivial patches tree.

Thank you!

/mjt