[v3,4/4] qdev: add device policy [RfC]

Add policies for devices which are deprecated or not secure.
There are three options: allow, warn and deny.

It's implemented for devices only.  Devices will probably be the main
user of this.  Also object_new() can't fail as of today so it's a bit
hard to implement policy checking at object level, especially the 'deny'
part of it.

TODO: add a command line option to actually set these policies.

Comments are welcome.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/core/qdev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

On Thu, 6 Jun 2024 at 15:31, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Add policies for devices which are deprecated or not secure.
> There are three options: allow, warn and deny.
>
> It's implemented for devices only.  Devices will probably be the main
> user of this.  Also object_new() can't fail as of today so it's a bit
> hard to implement policy checking at object level, especially the 'deny'
> part of it.
>
> TODO: add a command line option to actually set these policies.
>
> Comments are welcome.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

> @@ -162,14 +208,26 @@ DeviceState *qdev_new(const char *name)
>          error_report("unknown type '%s'", name);
>          abort();
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        exit(1);
> +    }
> +
>      return DEVICE(object_new(name));
>  }
>
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!module_object_class_by_name(name)) {
> +    ObjectClass *oc = module_object_class_by_name(name);
> +
> +    if (!oc) {
>          return NULL;
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        return NULL;
> +    }
> +
>      return DEVICE(object_new(name));
>  }

It's valid to create a qdev device via object_new(), so
this doesn't work as a place to put the check. My suggestion
would be to restrict the deprecation handling to qdev only,
not to objects in general. Then you can do it in the
qdev device base class realize method, and fail realize
if it's not supported.

(qdev_try_new() is one of those "we use this in just 4
places" APIs that always tempts me to wonder if we should
really have it...)

thanks
-- PMM

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add policies for devices which are deprecated or not secure.
> There are three options: allow, warn and deny.
>
> It's implemented for devices only.  Devices will probably be the main
> user of this.  Also object_new() can't fail as of today so it's a bit
> hard to implement policy checking at object level, especially the 'deny'
> part of it.
>
> TODO: add a command line option to actually set these policies.
>
> Comments are welcome.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/core/qdev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f3a996f57dee..0c4e5cec743c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -43,6 +43,15 @@
>  static bool qdev_hot_added = false;
>  bool qdev_hot_removed = false;
>  
> +enum qdev_policy {
> +    QDEV_ALLOW = 0,
> +    QDEV_WARN  = 1,
> +    QDEV_DENY  = 2,
> +};
> +
> +static enum qdev_policy qdev_deprecated_policy;
> +static enum qdev_policy qdev_not_secure_policy;
> +
>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> @@ -144,6 +153,43 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
>      return true;
>  }
>  
> +static bool qdev_class_check(const char *name, ObjectClass *oc)
> +{
> +    bool allow = true;
> +
> +    if (oc->deprecated) {
> +        switch (qdev_deprecated_policy) {
> +        case QDEV_WARN:
> +            warn_report("device \"%s\" is deprecated", name);
> +            break;
> +        case QDEV_DENY:
> +            error_report("device \"%s\" is deprecated", name);
> +            allow = false;
> +            break;
> +        default:
> +            /* nothing */
> +            break;
> +        }
> +    }
> +
> +    if (oc->not_secure) {
> +        switch (qdev_not_secure_policy) {
> +        case QDEV_WARN:
> +            warn_report("device \"%s\" is not secure", name);
> +            break;
> +        case QDEV_DENY:
> +            error_report("device \"%s\" is not secure", name);
> +            allow = false;
> +            break;
> +        default:
> +            /* nothing */
> +            break;
> +        }
> +    }
> +
> +    return allow;
> +}
> +
>  DeviceState *qdev_new(const char *name)
>  {
>      ObjectClass *oc = object_class_by_name(name);
> @@ -162,14 +208,26 @@ DeviceState *qdev_new(const char *name)
>          error_report("unknown type '%s'", name);
>          abort();
>      }
> +
> +    if (!qdev_class_check(name, oc)) {

Anti-pattern: use of error_report() from within a function that returns
an error via Error **errp argument.

One such call chain: QMP core -> qmp_device_add() -> qdev_device_add()
-> qdev_device_add_from_qdict() -> qdev_new().  Your error message goes
to stderr, which is wrong.

> +        exit(1);

Worse, QMP command device_add can now kill the guest instantly.

You need to lift the check up the call chains some.

> +    }
> +
>      return DEVICE(object_new(name));
>  }
>  
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!module_object_class_by_name(name)) {
> +    ObjectClass *oc = module_object_class_by_name(name);
> +
> +    if (!oc) {
>          return NULL;
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        return NULL;
> +    }
> +
>      return DEVICE(object_new(name));
>  }