Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory

2017-08-28 Thread Michael S. Tsirkin 
On Mon, Aug 28, 2017 at 06:34:45PM +0200, Igor Mammedov wrote:
> Fixes read after freeing error reported
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
>   Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
> 
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
> 
>qdev_device_add()
>  object_property_set_bool('realized', true)
>device_set_realized()
>   ...
>   pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
>   AHCIDevice *ad = >dev[i];
>   ide_bus_new(>port, sizeof(ad->port), qdev, i, 1);
>   ^^^ creates bus in memory allocated by above gnew()
>   and adds it as child propety to ahci device
>   ...
>   hotplug_handler_plug(); -> goto post_realize_fail;
>   pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
>   ...
>g_free(s->dev);
>^^^ free memory that holds children busses
> 
>   return with error from device_set_realized()
> 
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
> 
> Reported-by: Thomas Huth 
> Signed-off-by: Igor Mammedov 

Reviewed-by: Michael S. Tsirkin 

Pls merge through ide tree.

> ---
>  hw/ide/ahci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>  
>  ide_exit(s);
>  }
> +object_unparent(OBJECT(>port));
>  }
>  
>  g_free(s->dev);
> -- 
> 2.7.4

Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory

2017-08-28 Thread Thomas Huth 
On 28.08.2017 18:34, Igor Mammedov wrote:
> Fixes read after freeing error reported
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
>   Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
> 
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
> 
>qdev_device_add()
>  object_property_set_bool('realized', true)
>device_set_realized()
>   ...
>   pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
>   AHCIDevice *ad = >dev[i];
>   ide_bus_new(>port, sizeof(ad->port), qdev, i, 1);
>   ^^^ creates bus in memory allocated by above gnew()
>   and adds it as child propety to ahci device
>   ...
>   hotplug_handler_plug(); -> goto post_realize_fail;
>   pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
>   ...
>g_free(s->dev);
>^^^ free memory that holds children busses
> 
>   return with error from device_set_realized()
> 
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
> 
> Reported-by: Thomas Huth 
> Signed-off-by: Igor Mammedov 
> ---
>  hw/ide/ahci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>  
>  ide_exit(s);
>  }
> +object_unparent(OBJECT(>port));
>  }
>  
>  g_free(s->dev);
> 

Thanks, this fixes the problem for me with both, x86_64 and mips64el!

Tested-by: Thomas Huth

Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory

2017-08-28 Thread Philippe Mathieu-Daudé 

On 08/28/2017 01:34 PM, Igor Mammedov wrote:

Fixes read after freeing error reported
   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
   Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>

ich9-ahci device creates ide buses and attaches them as QOM children
at realize time, however it forgets to properly clean them up
at unrealize time and frees memory containing these children,
with following call-chain:

qdev_device_add()
  object_property_set_bool('realized', true)
device_set_realized()
   ...
   pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
...
s->dev = g_new0(AHCIDevice, ports);
...
   AHCIDevice *ad = >dev[i];
   ide_bus_new(>port, sizeof(ad->port), qdev, i, 1);
   ^^^ creates bus in memory allocated by above gnew()
   and adds it as child propety to ahci device
   ...
   hotplug_handler_plug(); -> goto post_realize_fail;
   pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
   ...
g_free(s->dev);
^^^ free memory that holds children busses

   return with error from device_set_realized()

As result later when qdev_device_add() tries to unparent ich9-ahci
after failed device_set_realized(),
 object_unparent() -> object_property_del_child()
iterates over existing QOM children including buses added by
ide_bus_new() and tries to unparent them, which causes access to
freed memory where they where located.

Reported-by: Thomas Huth 
Signed-off-by: Igor Mammedov 


Reviewed-by: Philippe Mathieu-Daudé 


---
  hw/ide/ahci.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 406a1b5..ccbe091 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
  
  ide_exit(s);

  }
+object_unparent(OBJECT(>port));
  }
  
  g_free(s->dev);

[Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory

2017-08-28 Thread Igor Mammedov 
Fixes read after freeing error reported
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
  Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>

ich9-ahci device creates ide buses and attaches them as QOM children
at realize time, however it forgets to properly clean them up
at unrealize time and frees memory containing these children,
with following call-chain:

   qdev_device_add()
 object_property_set_bool('realized', true)
   device_set_realized()
  ...
  pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
   ...
   s->dev = g_new0(AHCIDevice, ports);
   ...
  AHCIDevice *ad = >dev[i];
  ide_bus_new(>port, sizeof(ad->port), qdev, i, 1);
  ^^^ creates bus in memory allocated by above gnew()
  and adds it as child propety to ahci device
  ...
  hotplug_handler_plug(); -> goto post_realize_fail;
  pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
  ...
   g_free(s->dev);
   ^^^ free memory that holds children busses

  return with error from device_set_realized()

As result later when qdev_device_add() tries to unparent ich9-ahci
after failed device_set_realized(),
object_unparent() -> object_property_del_child()
iterates over existing QOM children including buses added by
ide_bus_new() and tries to unparent them, which causes access to
freed memory where they where located.

Reported-by: Thomas Huth 
Signed-off-by: Igor Mammedov 
---
 hw/ide/ahci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 406a1b5..ccbe091 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
 
 ide_exit(s);
 }
+object_unparent(OBJECT(>port));
 }
 
 g_free(s->dev);
-- 
2.7.4