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

["Michael S. Tsirkin" <mst@redhat.com>]

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