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

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

[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-ff07eba1b090@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 = &s->dev[i];
                  ide_bus_new(&ad->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 <thuth@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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(&ad->port));
     }
 
     g_free(s->dev);
-- 
2.7.4

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

[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-ff07eba1b090@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 = &s->dev[i];
>                    ide_bus_new(&ad->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 <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   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(&ad->port));
>       }
>   
>       g_free(s->dev);
> 

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

[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-ff07eba1b090@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 = &s->dev[i];
>                   ide_bus_new(&ad->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 <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  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(&ad->port));
>      }
>  
>      g_free(s->dev);
> 

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

Tested-by: Thomas Huth <thuth@redhat.com>

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

[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-ff07eba1b090@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 = &s->dev[i];
>                   ide_bus_new(&ad->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 <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

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(&ad->port));
>      }
>  
>      g_free(s->dev);
> -- 
> 2.7.4

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

[John Snow]

On 08/28/2017 12: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-ff07eba1b090@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 = &s->dev[i];
>                   ide_bus_new(&ad->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 <thuth@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  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(&ad->port));
>      }
>  
>      g_free(s->dev);
> 

Nice, Thank you.

Reviewed-by: John Snow <jsnow@redhat.com>

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js