[Spice-devel] [PATCH spice-gtk 4/5] Add a suid root helper to open usb device nodes

[Marc-André Lureau]

On Tue, Nov 15, 2011 at 4:31 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> +AC_ARG_ENABLE([polkit],
> +  AS_HELP_STRING([--enable-polkit=@<:@yes/no@:>@],
> +                 [Enable policykit support (for the usb acl helper)@<:@default=yes@:>@]),
> +  [],
> +  [enable_polkit="yes"])

No need for that (Apparently. Anyway, it shouldn't need to be
optionnal for usbredir).

> +       -lacl                                   \
> +       $(NULL)

Shouldn't libacl be checked in configure.ac too?

> @@ -145,10 +153,39 @@ static gboolean spice_usbredir_channel_open_device(
>     }
>
>     spice_channel_connect(SPICE_CHANNEL(channel));
> +    priv->connected = TRUE;
>
>     return TRUE;
>  }
>
> +#if USE_POLKIT
> +static void spice_usbredir_channel_open_acl_cb(
> +    GObject *gobject, GAsyncResult *acl_res, gpointer user_data)
> +{
> +    SpiceUsbAclHelper *acl_helper = SPICE_USB_ACL_HELPER(gobject);
> +    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(user_data);
> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> +    GError *err = NULL;
> +
> +    g_return_if_fail(acl_helper == priv->acl_helper);
> +
> +    if (spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &err))
> +        spice_usbredir_channel_open_device(channel, &err);
> +
> +    if (err) {
> +        g_simple_async_result_take_error(priv->result, err);
> +        g_clear_object(&priv->context);
> +        g_clear_object(&priv->device);
> +    }
> +
> +    spice_usb_acl_helper_close_acl(priv->acl_helper);
> +    g_clear_object(&priv->acl_helper);
> +
> +    g_simple_async_result_complete_in_idle(priv->result);
> +    g_clear_object(&priv->result);
> +}
> +#endif
> +
>  G_GNUC_INTERNAL
>  void spice_usbredir_channel_connect(SpiceUsbredirChannel *channel,
>                                     GUsbContext          *context,
> @@ -180,9 +217,21 @@ void spice_usbredir_channel_connect(SpiceUsbredirChannel *channel,
>     priv->context = g_object_ref(context);
>     priv->device  = g_object_ref(device);
>     if (!spice_usbredir_channel_open_device(channel, &err)) {
> +#if USE_POLKIT
> +        priv->result = result;
> +        priv->acl_helper = spice_usb_acl_helper_new();

They should be closed & disposed in dispose imho.

> +        spice_usb_acl_helper_open_acl(priv->acl_helper,
> +                                      g_usb_device_get_bus(device),
> +                                      g_usb_device_get_address(device),
> +                                      cancellable,
> +                                      spice_usbredir_channel_open_acl_cb,
> +                                      channel);

Since the SpiceUsbAclHelper keep running when it's unref, we should
give away a reference to the channel.  And probably close & dispose on
channel dispose. See below

> +#else
>         g_simple_async_result_take_error(result, err);
>         g_clear_object(&priv->context);
>         g_clear_object(&priv->device);
> +#endif

I don't think we need a fallback, we are not interested in weird
setups, and people shouldn't run spice-gtk as root.

> +#if USE_POLKIT
> +    /*
> +     * If we're still waiting for the acl helper cancel it and don't clear
> +     * priv->device and context, keeping this channel "busy" until
> +     * spice_usbredir_channel_open_acl_cb() has run from the main loop,
> +     * avoiding a new connect happening before the old connect has finished.
> +     */

If it's called from finalize() I don't see how you could keep the
channel "busy". Instead we should break depedency in dispose(),
"break" the reference and cancel the helper asap or in idle. This
would also avoid the "busy" situation if it's called in a different
case, not from finalize().

> +    if (priv->acl_helper) {
> +        spice_usb_acl_helper_close_acl(priv->acl_helper);
> +    } else

Do we need a else?

> +#endif
> +    {
>         g_clear_object(&priv->device);
>         g_clear_object(&priv->context);
>     }


the rests looks ok, but we might want to be extra careful about the
setuid root... Did you discuss that issue with Kay? Couldn't we have a
group/user for it / USB devices? (I am just sayin', I don't really
know what that imply)

-- 
Marc-André Lureau