[PULL 01/22] hw/net/spapr: prevent potential NULL dereference

[Philippe Mathieu-Daudé]

From: Oleg Sviridov 

Pointer, returned from function 'spapr_vio_find_by_reg',
may be NULL and is dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
Message-ID: <20240531073636.3779559-1-oleg.sviri...@red-soft.ru>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/net/spapr_llan.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..8af33d91b6 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,12 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU 
*cpu,
 SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
 int i;
 
+if (!dev) {
+hcall_dprintf("H_CHANGE_LOGICAL_LAN_MAC called when "
+  "no NIC is present\n");
+return H_PARAMETER;
+}
+
 for (i = 0; i < ETH_ALEN; i++) {
 dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
 macaddr >>= 8;
-- 
2.41.0

Re: [PATCH v2] hw/net: prevent potential NULL dereference

[Philippe Mathieu-Daudé]

On 31/5/24 09:36, Oleg Sviridov wrote:

Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is 
dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
v2: Debug message added
  hw/net/spapr_llan.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..8af33d91b6 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,12 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU 
*cpu,
  SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
  int i;
  
+if (!dev) {

+hcall_dprintf("H_CHANGE_LOGICAL_LAN_MAC called when "
+  "no NIC is present\n");
+return H_PARAMETER;
+}
+
  for (i = 0; i < ETH_ALEN; i++) {
  dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
  macaddr >>= 8;


Thanks, patch queued.

[PATCH v2] hw/net: prevent potential NULL dereference

[Oleg Sviridov]

Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is 
dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
v2: Debug message added
 hw/net/spapr_llan.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..8af33d91b6 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,12 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU 
*cpu,
 SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
 int i;
 
+if (!dev) {
+hcall_dprintf("H_CHANGE_LOGICAL_LAN_MAC called when "
+  "no NIC is present\n");
+return H_PARAMETER;
+}
+
 for (i = 0; i < ETH_ALEN; i++) {
 dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
 macaddr >>= 8;
-- 
2.44.0

Re: [PATCH] hw/net: prevent potential NULL dereference

[David Gibson]

On Thu, May 30, 2024 at 10:03:51AM +0100, Peter Maydell wrote:
> On Thu, 30 May 2024 at 01:52, David Gibson  
> wrote:
> >
> > On Wed, May 29, 2024 at 02:07:18PM +0300, Oleg Sviridov wrote:
> > > Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and 
> > > is dereferenced immediately after.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > >
> > > Signed-off-by: Oleg Sviridov 
> > > ---
> > >  hw/net/spapr_llan.c | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > > index ecb30b7c76..f40b733229 100644
> > > --- a/hw/net/spapr_llan.c
> > > +++ b/hw/net/spapr_llan.c
> > > @@ -770,6 +770,10 @@ static target_ulong 
> > > h_change_logical_lan_mac(PowerPCCPU *cpu,
> > >  SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> >
> > Hmm... I thought VIO_SPAPR_VLAN_DEVICE() was supposed to abort if sdev
> > was NULL or not of the right type.  Or have the rules for qom helpers
> > changed since I wrote this.
> 
> QOM casts abort if the type is wrong, but a NULL pointer is
> passed through as a NULL pointer.

Ah, my mistake.  LGTM, then.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] hw/net: prevent potential NULL dereference

[Олег Свиридов]

Thanks for review.

Would it be correct to use hcall_dprintf() as in other functions of the 
module? For example, in h_add_logical_lan_buffer().


Best regards,
Oleg.

29.05.2024 16:52, Philippe Mathieu-Daudé пишет:

On 29/5/24 13:07, Oleg Sviridov wrote:
Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL 
and is dereferenced immediately after.


Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
  hw/net/spapr_llan.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..f40b733229 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,10 @@ static target_ulong 
h_change_logical_lan_mac(PowerPCCPU *cpu,

  SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
  int i;


Trying to change a MAC when no NIC is present is dubious, we could
at least report this using qemu_log_mask(LOG_GUEST_ERROR).


+    if (!dev) {
+    return H_PARAMETER;
+    }
+
  for (i = 0; i < ETH_ALEN; i++) {
  dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
  macaddr >>= 8;

Re: [PATCH] hw/net: prevent potential NULL dereference

[Peter Maydell]

On Thu, 30 May 2024 at 01:52, David Gibson  wrote:
>
> On Wed, May 29, 2024 at 02:07:18PM +0300, Oleg Sviridov wrote:
> > Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is 
> > dereferenced immediately after.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Signed-off-by: Oleg Sviridov 
> > ---
> >  hw/net/spapr_llan.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > index ecb30b7c76..f40b733229 100644
> > --- a/hw/net/spapr_llan.c
> > +++ b/hw/net/spapr_llan.c
> > @@ -770,6 +770,10 @@ static target_ulong 
> > h_change_logical_lan_mac(PowerPCCPU *cpu,
> >  SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>
> Hmm... I thought VIO_SPAPR_VLAN_DEVICE() was supposed to abort if sdev
> was NULL or not of the right type.  Or have the rules for qom helpers
> changed since I wrote this.

QOM casts abort if the type is wrong, but a NULL pointer is
passed through as a NULL pointer.

thanks
-- PMM

Re: [PATCH] hw/net: prevent potential NULL dereference

[David Gibson]

On Wed, May 29, 2024 at 02:07:18PM +0300, Oleg Sviridov wrote:
> Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is 
> dereferenced immediately after.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Oleg Sviridov 
> ---
>  hw/net/spapr_llan.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index ecb30b7c76..f40b733229 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU 
> *cpu,
>  SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);

Hmm... I thought VIO_SPAPR_VLAN_DEVICE() was supposed to abort if sdev
was NULL or not of the right type.  Or have the rules for qom helpers
changed since I wrote this.

>  int i;
>  
> +if (!dev) {
> +return H_PARAMETER;
> +}
> +
>  for (i = 0; i < ETH_ALEN; i++) {
>  dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
>  macaddr >>= 8;

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] hw/net: prevent potential NULL dereference

[Philippe Mathieu-Daudé]

On 29/5/24 13:07, Oleg Sviridov wrote:

Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is 
dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
  hw/net/spapr_llan.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..f40b733229 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU 
*cpu,
  SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
  int i;
  


Trying to change a MAC when no NIC is present is dubious, we could
at least report this using qemu_log_mask(LOG_GUEST_ERROR).


+if (!dev) {
+return H_PARAMETER;
+}
+
  for (i = 0; i < ETH_ALEN; i++) {
  dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
  macaddr >>= 8;

Re: [PATCH] hw/s390x: prevent potential NULL dereference

[Philippe Mathieu-Daudé]

Hi Oleg,

On 29/5/24 13:36, Oleg Sviridov wrote:

Pointer, returned from function 's390_ipl_get_iplb_pv', may be NULL and is 
dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
  hw/s390x/ipl.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index e934bf89d1..2fa6a340a1 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -706,9 +706,14 @@ int s390_ipl_prepare_pv_header(Error **errp)
  {
  IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
  IPLBlockPV *ipib_pv = >pv;


I suppose ipib_pv being NULL here is a bug elsewhere. You should
add here:

   assert(ipib_pv);

and look at the backtrace or audit the code.


-void *hdr = g_malloc(ipib_pv->pv_header_len);
+void *hdr;
  int rc;
  
+if (!ipib_pv) {


(If this is a legit error you have to set errp here before returning).


+return -1;
+}
+
+hdr = g_malloc(ipib_pv->pv_header_len);
  cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
   ipib_pv->pv_header_len);
  rc = s390_pv_set_sec_parms((uintptr_t)hdr, ipib_pv->pv_header_len, errp);
@@ -722,6 +727,10 @@ int s390_ipl_pv_unpack(void)
  IPLBlockPV *ipib_pv = >pv;
  int i, rc = 0;
  


Ditto assert.


+if (!ipib_pv) {
+return -1;
+}
+
  for (i = 0; i < ipib_pv->num_comp; i++) {
  rc = s390_pv_unpack(ipib_pv->components[i].addr,
  TARGET_PAGE_ALIGN(ipib_pv->components[i].size),

[PATCH] hw/net: prevent potential NULL dereference

[Oleg Sviridov]

Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is 
dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
 hw/net/spapr_llan.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..f40b733229 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU 
*cpu,
 SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
 int i;
 
+if (!dev) {
+return H_PARAMETER;
+}
+
 for (i = 0; i < ETH_ALEN; i++) {
 dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
 macaddr >>= 8;
-- 
2.44.0

[PATCH] hw/s390x: prevent potential NULL dereference

[Oleg Sviridov]

Pointer, returned from function 's390_ipl_get_iplb_pv', may be NULL and is 
dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov 
---
 hw/s390x/ipl.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index e934bf89d1..2fa6a340a1 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -706,9 +706,14 @@ int s390_ipl_prepare_pv_header(Error **errp)
 {
 IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
 IPLBlockPV *ipib_pv = >pv;
-void *hdr = g_malloc(ipib_pv->pv_header_len);
+void *hdr;
 int rc;
 
+if (!ipib_pv) {
+return -1;
+}
+
+hdr = g_malloc(ipib_pv->pv_header_len);
 cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
  ipib_pv->pv_header_len);
 rc = s390_pv_set_sec_parms((uintptr_t)hdr, ipib_pv->pv_header_len, errp);
@@ -722,6 +727,10 @@ int s390_ipl_pv_unpack(void)
 IPLBlockPV *ipib_pv = >pv;
 int i, rc = 0;
 
+if (!ipib_pv) {
+return -1;
+}
+
 for (i = 0; i < ipib_pv->num_comp; i++) {
 rc = s390_pv_unpack(ipib_pv->components[i].addr,
 TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
-- 
2.44.0

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> > Aha, new crashes! Let's look at them.
> > 
> > 41 and 155 failed with crash, 141 without but I see "+{"error": {"class": 
> > "GenericError", "desc": "Block device drv0 is in use"}}" in its output.
> > 
> > Hmm, but all crashes are because of assert(QTAILQ_EMPTY(_bdrv_states)); 
> > in bdrv_close_all..
> > 
> > So, we have a problem, but another one..
> 
> Investigate it a bit more.
> 
> Accordingly to comment, bdrv_attach_child unrefs child bs even on failure, so 
> let's do
> 
> --- a/block.c
> +++ b/block.c
> @@ -2768,6 +2768,7 @@ void bdrv_set_backing_hd_ex(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
> 
>  if (bdrv_attach_child_fail) {
>  bs->backing = NULL;
> +bdrv_unref(backing_hd);
>  error_setg(errp, "Unpredicted failure :)");
>  } else {
>  bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> _backing,
> 
> 
> - then, all three tests fails, but without crashes. OK.
> 
> The only interesting thing is that, it seems that bdrv_attach_child doesn't 
> unref child on all failure paths.
> 
> It calls bdrv_root_attach_child..
> 
> which doesn't unref child on the path
> 
> if (bdrv_get_aio_context(child_bs) != ctx) {
>   ...
>if (ret < 0) {
>  error_propagate(errp, local_err);
>  g_free(child);
>  bdrv_abort_perm_update(child_bs);
>  return NULL;
>}
> }
> 
> or am I wrong?

I think you're right, we need a bdrv_unref() there. The function comment
makes it clear that bdrv_root_attach_child() is supposed to consume the
reference both in success and error cases.

Kevin

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:

24.03.2020 12:50, Kevin Wolf wrote:

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.



While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477


Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?



No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in 
bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably 
reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
     const BdrvChildRole *child_role,
     bool allow_none, Error **errp);
  BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+    bool bdrv_attach_child_fail, Error **errp);
  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
   Error **errp);
  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState 
*child,
   * Sets the backing file link of a BDS. A new reference is created; callers
   * which don't need their own reference any more must call bdrv_unref().
   */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+    bool bdrv_attach_child_fail, Error **errp)
  {
  bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
  bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
  goto out;
  }

-    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _backing,
-    errp);
+    if (bdrv_attach_child_fail) {
+    bs->backing = NULL;
+    error_setg(errp, "Unpredicted failure :)");
+    } else {
+    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
_backing,
+    errp);
+    }
+
  /* If backing_hd was already part of bs's backing chain, and
   * inherits_from pointed recursively to bs then let's update it to
   * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
  bdrv_refresh_limits(bs, NULL);
  }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
+{
+    bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
  /*
   * Opens the backing file for a BlockDriverState if not yet open
   *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
  if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
  BlockDriverState *backing = s->is_none_mode ? src : s->base;
  if (backing_bs(target_bs) != backing) {
-    bdrv_set_backing_hd(target_bs, backing, _err);
+    bdrv_set_backing_hd_ex(target_bs, backing, true, _err);
  if (local_err) {
  error_report_err(local_err);
  ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
  if (bs->backing == NULL) {
  /* we can be here after failed bdrv_attach_child in
   * bdrv_set_backing_hd */
+    abort();
  return;
  }
  pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:50, Kevin Wolf wrote:

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.



While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477


Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?



No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in 
bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably 
reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
const BdrvChildRole *child_role,
bool allow_none, Error **errp);
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+bool bdrv_attach_child_fail, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
  Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState 
*child,
  * Sets the backing file link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
  */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+bool bdrv_attach_child_fail, Error **errp)
 {
 bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
 bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 goto out;
 }

-bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _backing,
-errp);
+if (bdrv_attach_child_fail) {
+bs->backing = NULL;
+error_setg(errp, "Unpredicted failure :)");
+} else {
+bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
_backing,
+errp);
+}
+
 /* If backing_hd was already part of bs's backing chain, and
  * inherits_from pointed recursively to bs then let's update it to
  * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
 bdrv_refresh_limits(bs, NULL);
 }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
+{
+bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
 if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
 BlockDriverState *backing = s->is_none_mode ? src : s->base;
 if (backing_bs(target_bs) != backing) {
-bdrv_set_backing_hd(target_bs, backing, _err);
+bdrv_set_backing_hd_ex(target_bs, backing, true, _err);
 if (local_err) {
 error_report_err(local_err);
 ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
 if (bs->backing == NULL) {
 /* we can be here after failed bdrv_attach_child in
  * bdrv_set_backing_hd */
+abort();
 return;
 }
 pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),


==

[root@kvm qemu-iotests]# git grep -il mirror ??? | xargs
030 041 044 

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:
> On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
> > Hi,
> > 
> > Nullness of  needs to be checked here:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
> > 
> > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...

Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.

> > 
> > While it is done at 2 other locations:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?

Kevin

Re: Potential Null dereference

[Philippe Mathieu-Daudé]

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

if(bs->backing== NULL) {return}


Thanks for spotting this. Do you mind sending a patch?



Thanks,
Mansour

Potential Null dereference

[Mansour Ahmadi]

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...

While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

if (bs->backing == NULL) { return}

Thanks,
Mansour

[Qemu-devel] [PULL 01/11] s390: avoid potential null dereference in s390_pcihost_unplug()

[Cornelia Huck]

From: Li Qiang 

When getting the 'pbdev', the if...else has no default branch.
>From Coverity, the 'pbdev' maybe null when the 'dev' is not
the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
This patch adds a default branch for device plug and unplug.

Spotted by Coverity: CID 1398593

Signed-off-by: Li Qiang 
Message-Id: <20190108151114.33140-1-liq...@163.com>
Reviewed-by: David Hildenbrand 
Reviewed-by: Halil Pasic 
Reviewed-by: Collin Walling 
Signed-off-by: Cornelia Huck 
---
 hw/s390x/s390-pci-bus.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 15759b6514..a94700a78c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -912,6 +912,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 pbdev->fh = pbdev->idx;
 QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
 g_hash_table_insert(s->zpci_table, >idx, pbdev);
+} else {
+g_assert_not_reached();
 }
 }
 
@@ -956,6 +958,8 @@ static void s390_pcihost_unplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
 pbdev = S390_PCI_DEVICE(dev);
 pci_dev = pbdev->pdev;
+} else {
+g_assert_not_reached();
 }
 
 switch (pbdev->state) {
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Tue,  8 Jan 2019 07:11:14 -0800
Li Qiang  wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
> 
> Change since v2: use g_assert_not_reached for default branch
> 
>  hw/s390x/s390-pci-bus.c | 4 
>  1 file changed, 4 insertions(+)

Thanks, applied.

Re: [Qemu-devel] [PATCH v3] s390: avoid potential null dereference ins390_pcihost_unplug()

[Collin Walling]

On 1/8/19 10:11 AM, Li Qiang wrote:
> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
> 

[...]

Reviewed-by: Collin Walling 

Re: [Qemu-devel] [PATCH v3] s390: avoid potential null dereference ins390_pcihost_unplug()

[Halil Pasic]

On Tue,  8 Jan 2019 07:11:14 -0800
Li Qiang  wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 

Reviewed-by: Halil Pasic 

[..]

Re: [Qemu-devel] [PATCH v3] s390: avoid potential null dereference ins390_pcihost_unplug()

[David Hildenbrand]

On 08.01.19 16:11, Li Qiang wrote:
> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
> 
> Change since v2: use g_assert_not_reached for default branch
> 
>  hw/s390x/s390-pci-bus.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..a94700a78c 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -912,6 +912,8 @@ static void s390_pcihost_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  pbdev->fh = pbdev->idx;
>  QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
>  g_hash_table_insert(s->zpci_table, >idx, pbdev);
> +} else {
> +g_assert_not_reached();
>  }
>  }
>  
> @@ -956,6 +958,8 @@ static void s390_pcihost_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>  pbdev = S390_PCI_DEVICE(dev);
>  pci_dev = pbdev->pdev;
> +} else {
> +g_assert_not_reached();
>  }
>  
>  switch (pbdev->state) {
> 

Reviewed-by: David Hildenbrand 

Please note that I'll be sending a bigger unplug rework the next days.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v3] s390: avoid potential null dereference ins390_pcihost_unplug()

[Li Qiang]

When getting the 'pbdev', the if...else has no default branch.
>From Coverity, the 'pbdev' maybe null when the 'dev' is not
the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
This patch adds a default branch for device plug and unplug.

Spotted by Coverity: CID 1398593

Signed-off-by: Li Qiang 
---

Change since v2: use g_assert_not_reached for default branch

 hw/s390x/s390-pci-bus.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 15759b6514..a94700a78c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -912,6 +912,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 pbdev->fh = pbdev->idx;
 QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
 g_hash_table_insert(s->zpci_table, >idx, pbdev);
+} else {
+g_assert_not_reached();
 }
 }
 
@@ -956,6 +958,8 @@ static void s390_pcihost_unplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
 pbdev = S390_PCI_DEVICE(dev);
 pci_dev = pbdev->pdev;
+} else {
+g_assert_not_reached();
 }
 
 switch (pbdev->state) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[李强]

At 2019-01-08 00:10:29, "Cornelia Huck"  wrote:
>On Mon, 7 Jan 2019 16:04:35 +
>Peter Maydell  wrote:
>
>> On Mon, 7 Jan 2019 at 15:57, Cornelia Huck  wrote:
>> > On Mon, 7 Jan 2019 15:54:21 +
>> > Peter Maydell  wrote:  
>> > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck  wrote:  
>> > > > Sounds good. But please return anyway in the unplug case, so that the
>> > > > code is fine if asserts have been configured out.  
>> > >
>> > > Hopefully that won't cause the compiler to complain about
>> > > unreachable code :-)  
>> >
>> > BTW: Is there a common configuration where asserts are configured out?
>> > Not that this is an accident waiting to happen...  
>> 
>> No -- we insist they are always enabled, and osdep.h will #error
>> out if either NDEBUG or G_DISABLE_ASSERT are set.
>
>Ah, now I remember (I thought we still had that problem.)
>

>In that case, no return is needed.


Ok, later I will send out a revised patch.


Thanks,
Li Qiang

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Mon, 7 Jan 2019 16:04:35 +
Peter Maydell  wrote:

> On Mon, 7 Jan 2019 at 15:57, Cornelia Huck  wrote:
> > On Mon, 7 Jan 2019 15:54:21 +
> > Peter Maydell  wrote:  
> > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck  wrote:  
> > > > Sounds good. But please return anyway in the unplug case, so that the
> > > > code is fine if asserts have been configured out.  
> > >
> > > Hopefully that won't cause the compiler to complain about
> > > unreachable code :-)  
> >
> > BTW: Is there a common configuration where asserts are configured out?
> > Not that this is an accident waiting to happen...  
> 
> No -- we insist they are always enabled, and osdep.h will #error
> out if either NDEBUG or G_DISABLE_ASSERT are set.

Ah, now I remember (I thought we still had that problem.)

In that case, no return is needed.

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Peter Maydell]

On Mon, 7 Jan 2019 at 15:57, Cornelia Huck  wrote:
> On Mon, 7 Jan 2019 15:54:21 +
> Peter Maydell  wrote:
> > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck  wrote:
> > > Sounds good. But please return anyway in the unplug case, so that the
> > > code is fine if asserts have been configured out.
> >
> > Hopefully that won't cause the compiler to complain about
> > unreachable code :-)
>
> BTW: Is there a common configuration where asserts are configured out?
> Not that this is an accident waiting to happen...

No -- we insist they are always enabled, and osdep.h will #error
out if either NDEBUG or G_DISABLE_ASSERT are set.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Mon, 7 Jan 2019 15:54:21 +
Peter Maydell  wrote:

> On Mon, 7 Jan 2019 at 15:48, Cornelia Huck  wrote:
> >
> > On Fri, 4 Jan 2019 22:33:51 +0800
> > Li Qiang  wrote:
> >  
> > > What do you think of ‘g_assert_not_reached();’. For example:
> > >
> > > else {
> > >  g_assert_not_reached();
> > > }  
> >
> > Sounds good. But please return anyway in the unplug case, so that the
> > code is fine if asserts have been configured out.  
> 
> Hopefully that won't cause the compiler to complain about
> unreachable code :-)

BTW: Is there a common configuration where asserts are configured out?
Not that this is an accident waiting to happen...

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Peter Maydell]

On Mon, 7 Jan 2019 at 15:48, Cornelia Huck  wrote:
>
> On Fri, 4 Jan 2019 22:33:51 +0800
> Li Qiang  wrote:
>
> > What do you think of ‘g_assert_not_reached();’. For example:
> >
> > else {
> >  g_assert_not_reached();
> > }
>
> Sounds good. But please return anyway in the unplug case, so that the
> code is fine if asserts have been configured out.

Hopefully that won't cause the compiler to complain about
unreachable code :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Fri, 4 Jan 2019 22:33:51 +0800
Li Qiang  wrote:

> What do you think of ‘g_assert_not_reached();’. For example:
> 
> else {
>  g_assert_not_reached();
> }

Sounds good. But please return anyway in the unplug case, so that the
code is fine if asserts have been configured out.

Re: [Qemu-devel] [qemu-s390x] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Cornelia Huck]

On Fri, 4 Jan 2019 16:05:15 +0100
Halil Pasic  wrote:

> On Fri, 4 Jan 2019 15:10:05 +0100
> Cornelia Huck  wrote:
> 
> > On Thu,  3 Jan 2019 07:16:12 -0800
> > Li Qiang  wrote:
> >   
> > > When getting the 'pbdev', the if...else has no default branch.
> > > From Coverity, the 'pbdev' maybe null when the 'dev' is not
> > > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> > > This patch adds a default branch for device plug and unplug.
> > > 
> > > Spotted by Coverity: CID 1398593
> > > 
> > > Signed-off-by: Li Qiang 
> > > ---
> > > Adds a default branch for device plug per Cornelia's review.
> > > 
> > >  hw/s390x/s390-pci-bus.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > index 15759b6514..fe48a36ff6 100644
> > > --- a/hw/s390x/s390-pci-bus.c
> > > +++ b/hw/s390x/s390-pci-bus.c
> > > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler 
> > > *hotplug_dev, DeviceState *dev,
> > >  pbdev->fh = pbdev->idx;
> > >  QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
> > >  g_hash_table_insert(s->zpci_table, >idx, pbdev);
> > > +} else {
> > > +error_setg(errp, "s390: device plug request for not supported 
> > > device"
> > > +   " type: %s", object_get_typename(OBJECT(dev)));  
> > 
> > Maybe make this "s390/pci: plugging device type <%s> is not supported"?
> >   
> 
> Under what circumstances could/does this happen? I mean how can this
> be triggered by the user?

Probably only if a new type that can be plugged has been added, but the
s390 pci code has not been updated. We could also assert, not sure what
would make it easier to figure out what went wrong.

Re: [Qemu-devel] [qemu-s390x] 答复: [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[David Hildenbrand]

On 04.01.19 15:33, Li Qiang wrote:
> What do you think of ‘g_assert_not_reached();’. For example:
> 
>  
> 
> else {
> 
>  g_assert_not_reached();
> 
> }
> 

I agree, if thisever happens, it is a serious programming error, not an
error to report to the user. (after all, he did nothing wrong)

I'd prefer asserts.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Halil Pasic]

On Fri, 4 Jan 2019 15:10:05 +0100
Cornelia Huck  wrote:

> On Thu,  3 Jan 2019 07:16:12 -0800
> Li Qiang  wrote:
> 
> > When getting the 'pbdev', the if...else has no default branch.
> > From Coverity, the 'pbdev' maybe null when the 'dev' is not
> > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> > This patch adds a default branch for device plug and unplug.
> > 
> > Spotted by Coverity: CID 1398593
> > 
> > Signed-off-by: Li Qiang 
> > ---
> > Adds a default branch for device plug per Cornelia's review.
> > 
> >  hw/s390x/s390-pci-bus.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 15759b6514..fe48a36ff6 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >  pbdev->fh = pbdev->idx;
> >  QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
> >  g_hash_table_insert(s->zpci_table, >idx, pbdev);
> > +} else {
> > +error_setg(errp, "s390: device plug request for not supported 
> > device"
> > +   " type: %s", object_get_typename(OBJECT(dev)));
> 
> Maybe make this "s390/pci: plugging device type <%s> is not supported"?
> 

Under what circumstances could/does this happen? I mean how can this
be triggered by the user?

Regards,
Halil

> >  }
> >  }
> >  
> > @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> >  pbdev = S390_PCI_DEVICE(dev);
> >  pci_dev = pbdev->pdev;
> > +} else {
> > +error_setg(errp, "s390: device unplug request for not supported 
> > device"
> > +   " type: %s", object_get_typename(OBJECT(dev)));
> 
> Halil has a point when he suggests an assert here. If we fence the
> device type in the plug function, I can't think of a way we would end
> up here.
> 
> > +return;
> >  }
> >  
> >  switch (pbdev->state) {
> 
> 

[Qemu-devel] 答复: [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Li Qiang]

What do you think of ‘g_assert_not_reached();’. For example:

else {
 g_assert_not_reached();
}

Thanks,
Li Qiang


发件人: Cornelia Huck
发送时间: 2019年1月4日 22:10
收件人: Li Qiang
抄送: wall...@linux.ibm.com; r...@twiddle.net; da...@redhat.com; 
pa...@linux.ibm.com; borntrae...@de.ibm.com; qemu-s3...@nongnu.org; 
qemu-devel@nongnu.org; peter.mayd...@linaro.org
主题: Re: [PATCH v2] s390: avoid potential null dereference 
ins390_pcihost_unplug()

On Thu,  3 Jan 2019 07:16:12 -0800
Li Qiang  wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
> Adds a default branch for device plug per Cornelia's review.
> 
>  hw/s390x/s390-pci-bus.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..fe48a36ff6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  pbdev->fh = pbdev->idx;
>  QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
>  g_hash_table_insert(s->zpci_table, >idx, pbdev);
> +} else {
> +error_setg(errp, "s390: device plug request for not supported device"
> +   " type: %s", object_get_typename(OBJECT(dev)));

Maybe make this "s390/pci: plugging device type <%s> is not supported"?

>  }
>  }
>  
> @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>  pbdev = S390_PCI_DEVICE(dev);
>  pci_dev = pbdev->pdev;
> +} else {
> +error_setg(errp, "s390: device unplug request for not supported 
> device"
> +   " type: %s", object_get_typename(OBJECT(dev)));

Halil has a point when he suggests an assert here. If we fence the
device type in the plug function, I can't think of a way we would end
up here.

> +return;
>  }
>  
>  switch (pbdev->state) {

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Cornelia Huck]

On Thu,  3 Jan 2019 07:16:12 -0800
Li Qiang  wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
> Adds a default branch for device plug per Cornelia's review.
> 
>  hw/s390x/s390-pci-bus.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..fe48a36ff6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  pbdev->fh = pbdev->idx;
>  QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
>  g_hash_table_insert(s->zpci_table, >idx, pbdev);
> +} else {
> +error_setg(errp, "s390: device plug request for not supported device"
> +   " type: %s", object_get_typename(OBJECT(dev)));

Maybe make this "s390/pci: plugging device type <%s> is not supported"?

>  }
>  }
>  
> @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>  pbdev = S390_PCI_DEVICE(dev);
>  pci_dev = pbdev->pdev;
> +} else {
> +error_setg(errp, "s390: device unplug request for not supported 
> device"
> +   " type: %s", object_get_typename(OBJECT(dev)));

Halil has a point when he suggests an assert here. If we fence the
device type in the plug function, I can't think of a way we would end
up here.

> +return;
>  }
>  
>  switch (pbdev->state) {

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390: avoid potential null dereference in s390_pcihost_unplug()

[Halil Pasic]

On Thu, 3 Jan 2019 15:54:38 +0100
Cornelia Huck  wrote:

> On Thu,  3 Jan 2019 06:02:46 -0800
> Li Qiang  wrote:
> 
> > When getting the 'pbdev', the if...else has no default branch.
> > From Coverity, the 'pbdev' maybe null when the 'dev' is not
> > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> > 
> > Spotted by Coverity: CID 1398593
> > 
> > Signed-off-by: Li Qiang 
> > ---
> >  hw/s390x/s390-pci-bus.c | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 15759b6514..b3122268a3 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -956,6 +956,10 @@ static void s390_pcihost_unplug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> >  pbdev = S390_PCI_DEVICE(dev);
> >  pci_dev = pbdev->pdev;
> > +} else {
> > +error_setg(errp, "s390: device unplug for not supported device"
> > +   " type: %s", object_get_typename(OBJECT(dev)));
> > +return;
> 
> I think that is something that really Should Not Happen. However,
> looking at s390_pcihost_plug, we just do nothing if the plugged device
> does not match any of the expected three values. Maybe we should also
> set an error in that case there?
> 

Really Should Not Happen is in this case a programming error AFAIU.
Wouldn't something more assert like reflect that better?

> >  }
> >  
> >  switch (pbdev->state) {
> 
> 

[Qemu-devel] 答复: [PATCH] s390: avoid potential null dereference ins390_pcihost_unplug()

[Li Qiang]

I’m inspired by the function ‘ich9_pm_device_unplug_cb’.
>From the ‘plug’ ich9_pm_device_plug_cb it set an error.
So I think we can also set this is this s390 device plug.

I will send out revised patch soon.

Thanks,
Li Qiang

发件人: Cornelia Huck
发送时间: 2019年1月3日 22:54
收件人: Li Qiang
抄送: wall...@linux.ibm.com; r...@twiddle.net; da...@redhat.com; 
pa...@linux.ibm.com; borntrae...@de.ibm.com; qemu-s3...@nongnu.org; 
qemu-devel@nongnu.org; peter.mayd...@linaro.org; Pierre Morel
主题: Re: [PATCH] s390: avoid potential null dereference ins390_pcihost_unplug()

On Thu,  3 Jan 2019 06:02:46 -0800
Li Qiang  wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
>  hw/s390x/s390-pci-bus.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..b3122268a3 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -956,6 +956,10 @@ static void s390_pcihost_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>  pbdev = S390_PCI_DEVICE(dev);
>  pci_dev = pbdev->pdev;
> +} else {
> +error_setg(errp, "s390: device unplug for not supported device"
> +   " type: %s", object_get_typename(OBJECT(dev)));
> +return;

I think that is something that really Should Not Happen. However,
looking at s390_pcihost_plug, we just do nothing if the plugged device
does not match any of the expected three values. Maybe we should also
set an error in that case there?

>  }
>  
>  switch (pbdev->state) {

[Qemu-devel] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Li Qiang]

When getting the 'pbdev', the if...else has no default branch.
>From Coverity, the 'pbdev' maybe null when the 'dev' is not
the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
This patch adds a default branch for device plug and unplug.

Spotted by Coverity: CID 1398593

Signed-off-by: Li Qiang 
---
Adds a default branch for device plug per Cornelia's review.

 hw/s390x/s390-pci-bus.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 15759b6514..fe48a36ff6 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 pbdev->fh = pbdev->idx;
 QTAILQ_INSERT_TAIL(>zpci_devs, pbdev, link);
 g_hash_table_insert(s->zpci_table, >idx, pbdev);
+} else {
+error_setg(errp, "s390: device plug request for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
 }
 }
 
@@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
 pbdev = S390_PCI_DEVICE(dev);
 pci_dev = pbdev->pdev;
+} else {
+error_setg(errp, "s390: device unplug request for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
+return;
 }
 
 switch (pbdev->state) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH] s390: avoid potential null dereference in s390_pcihost_unplug()

[Cornelia Huck]

On Thu,  3 Jan 2019 06:02:46 -0800
Li Qiang  wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang 
> ---
>  hw/s390x/s390-pci-bus.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..b3122268a3 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -956,6 +956,10 @@ static void s390_pcihost_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>  pbdev = S390_PCI_DEVICE(dev);
>  pci_dev = pbdev->pdev;
> +} else {
> +error_setg(errp, "s390: device unplug for not supported device"
> +   " type: %s", object_get_typename(OBJECT(dev)));
> +return;

I think that is something that really Should Not Happen. However,
looking at s390_pcihost_plug, we just do nothing if the plugged device
does not match any of the expected three values. Maybe we should also
set an error in that case there?

>  }
>  
>  switch (pbdev->state) {

[Qemu-devel] [PATCH] s390: avoid potential null dereference in s390_pcihost_unplug()

[Li Qiang]

When getting the 'pbdev', the if...else has no default branch.
>From Coverity, the 'pbdev' maybe null when the 'dev' is not
the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.

Spotted by Coverity: CID 1398593

Signed-off-by: Li Qiang 
---
 hw/s390x/s390-pci-bus.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 15759b6514..b3122268a3 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -956,6 +956,10 @@ static void s390_pcihost_unplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
 pbdev = S390_PCI_DEVICE(dev);
 pci_dev = pbdev->pdev;
+} else {
+error_setg(errp, "s390: device unplug for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
+return;
 }
 
 switch (pbdev->state) {
-- 
2.17.1