qmp: object-add: Validate class before creating object

Currently it is very easy to crash QEMU by issuing an object-add command
using an abstract class or a class that doesn't support
TYPE_USER_CREATABLE as parameter.

Example: with the following QMP command:

    (QEMU) object-add qom-type=cpu id=foo

QEMU aborts at:

    ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: (type->abstract == false)

This patch moves the check for TYPE_USER_CREATABLE before object_new(),
and adds a check to prevent the code from trying to instantiate abstract
classes.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qmp.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

On 04/15/2014 04:01 PM, Eduardo Habkost wrote:
> Currently it is very easy to crash QEMU by issuing an object-add command
> using an abstract class or a class that doesn't support
> TYPE_USER_CREATABLE as parameter.
> 
> Example: with the following QMP command:
> 
>     (QEMU) object-add qom-type=cpu id=foo
> 
> QEMU aborts at:
> 
>     ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: (type->abstract == false)
> 
> This patch moves the check for TYPE_USER_CREATABLE before object_new(),
> and adds a check to prevent the code from trying to instantiate abstract
> classes.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qmp.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/qmp.c b/qmp.c
> index 87a28f7..e7ecbb7 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id, const QDict *qdict,
>                  Visitor *v, Error **errp)
>  {
>      Object *obj;
> +    ObjectClass *klass;
>      const QDictEntry *e;
>      Error *local_err = NULL;
> 
> -    if (!object_class_by_name(type)) {
> +    klass = object_class_by_name(type);
> +    if (!klass) {
>          error_setg(errp, "invalid class name");
>          return;
>      }
> 
> +    if (object_class_is_abstract(klass)) {
> +        error_setg(errp, "object type '%s' is abstract", type);
> +        return;
> +    }
> +
> +    if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
> +        error_setg(errp, "object type '%s' isn't supported by object-add",
> +                   type);
> +        return;
> +    }
> +
>      obj = object_new(type);
>      if (qdict) {
>          for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id, const QDict *qdict,
>          }
>      }
> 
> -    if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> -        error_setg(&local_err, "object type '%s' isn't supported by object-add",
> -                   type);
> -        goto out;
> -    }
> -
>      user_creatable_complete(obj, &local_err);
>      if (local_err) {
>          goto out;
> 

I'm no expert here, but this seems straightforward:

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Also applied and tested both error paths, so feel to consider it:

Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Eduardo Habkost <ehabkost@redhat.com> writes:

> Currently it is very easy to crash QEMU by issuing an object-add command
> using an abstract class or a class that doesn't support
> TYPE_USER_CREATABLE as parameter.
>
> Example: with the following QMP command:
>
>     (QEMU) object-add qom-type=cpu id=foo
>
> QEMU aborts at:
>
>     ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: (type->abstract == false)
>
> This patch moves the check for TYPE_USER_CREATABLE before object_new(),
> and adds a check to prevent the code from trying to instantiate abstract
> classes.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Related device_add fixes:

061e84f qdev-monitor: Avoid device_add crashing on non-device driver name
2fa4e56 qdev-monitor: Fix crash when device_add is called with abstract driver
7ea5e78 qdev: Do not let the user try to device_add when it cannot work

> ---
>  qmp.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/qmp.c b/qmp.c
> index 87a28f7..e7ecbb7 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id, const QDict *qdict,
>                  Visitor *v, Error **errp)
>  {
>      Object *obj;
> +    ObjectClass *klass;
>      const QDictEntry *e;
>      Error *local_err = NULL;
>  
> -    if (!object_class_by_name(type)) {
> +    klass = object_class_by_name(type);
> +    if (!klass) {
>          error_setg(errp, "invalid class name");
>          return;
>      }
>  
> +    if (object_class_is_abstract(klass)) {
> +        error_setg(errp, "object type '%s' is abstract", type);
> +        return;
> +    }
> +
> +    if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
> +        error_setg(errp, "object type '%s' isn't supported by object-add",
> +                   type);
> +        return;
> +    }
> +
>      obj = object_new(type);
>      if (qdict) {
>          for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {

For comparison, this is qdev_device_add():

    oc = object_class_by_name(driver);
    if (!oc) {
        const char *typename = find_typename_by_alias(driver);

        if (typename) {
            driver = typename;
            oc = object_class_by_name(driver);
        }
    }

    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
        qerror_report(ERROR_CLASS_GENERIC_ERROR,
                      "'%s' is not a valid device model name", driver);
        return NULL;
    }

    if (object_class_is_abstract(oc)) {
        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
                      "non-abstract device type");
        return NULL;
    }

    dc = DEVICE_CLASS(oc);
    if (dc->cannot_instantiate_with_device_add_yet) {
        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
                      "pluggable device type");
        return NULL;
    }

Got the "subtype of" and the "not abstract" check in the opposite order,
and the additional cannot_instantiate_with_device_add_yet check.

Does the different order matter?

> @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id, const QDict *qdict,
>          }
>      }
>  
> -    if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> -        error_setg(&local_err, "object type '%s' isn't supported by object-add",
> -                   type);
> -        goto out;
> -    }
> -
>      user_creatable_complete(obj, &local_err);
>      if (local_err) {
>          goto out;

On Wed, Apr 16, 2014 at 08:53:23AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > Currently it is very easy to crash QEMU by issuing an object-add command
> > using an abstract class or a class that doesn't support
> > TYPE_USER_CREATABLE as parameter.
> >
> > Example: with the following QMP command:
> >
> >     (QEMU) object-add qom-type=cpu id=foo
> >
> > QEMU aborts at:
> >
> >     ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: (type->abstract == false)
> >
> > This patch moves the check for TYPE_USER_CREATABLE before object_new(),
> > and adds a check to prevent the code from trying to instantiate abstract
> > classes.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Related device_add fixes:
> 
> 061e84f qdev-monitor: Avoid device_add crashing on non-device driver name
> 2fa4e56 qdev-monitor: Fix crash when device_add is called with abstract driver
> 7ea5e78 qdev: Do not let the user try to device_add when it cannot work
> 
> > ---
> >  qmp.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/qmp.c b/qmp.c
> > index 87a28f7..e7ecbb7 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id, const QDict *qdict,
> >                  Visitor *v, Error **errp)
> >  {
> >      Object *obj;
> > +    ObjectClass *klass;
> >      const QDictEntry *e;
> >      Error *local_err = NULL;
> >  
> > -    if (!object_class_by_name(type)) {
> > +    klass = object_class_by_name(type);
> > +    if (!klass) {
> >          error_setg(errp, "invalid class name");
> >          return;
> >      }
> >  
> > +    if (object_class_is_abstract(klass)) {
> > +        error_setg(errp, "object type '%s' is abstract", type);
> > +        return;
> > +    }
> > +
> > +    if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
> > +        error_setg(errp, "object type '%s' isn't supported by object-add",
> > +                   type);
> > +        return;
> > +    }
> > +
> >      obj = object_new(type);
> >      if (qdict) {
> >          for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> 
> For comparison, this is qdev_device_add():
> 
>     oc = object_class_by_name(driver);
>     if (!oc) {
>         const char *typename = find_typename_by_alias(driver);
> 
>         if (typename) {
>             driver = typename;
>             oc = object_class_by_name(driver);
>         }
>     }
> 
>     if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
>         qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                       "'%s' is not a valid device model name", driver);
>         return NULL;
>     }
> 
>     if (object_class_is_abstract(oc)) {
>         qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
>                       "non-abstract device type");
>         return NULL;
>     }
> 
>     dc = DEVICE_CLASS(oc);
>     if (dc->cannot_instantiate_with_device_add_yet) {
>         qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
>                       "pluggable device type");
>         return NULL;
>     }
> 
> Got the "subtype of" and the "not abstract" check in the opposite order,
> and the additional cannot_instantiate_with_device_add_yet check.
> 
> Does the different order matter?

The order probably doesn't matter very much. But it makes sense to keep
it similar to device_add for consistency. I will send v2.


> > @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id, const QDict *qdict,
> >          }
> >      }
> >  
> > -    if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > -        error_setg(&local_err, "object type '%s' isn't supported by object-add",
> > -                   type);
> > -        goto out;
> > -    }
> > -
> >      user_creatable_complete(obj, &local_err);
> >      if (local_err) {
> >          goto out;