debian/patches: Properly handle aes3k devices, resubmitting commands

This also needs proper commands cancellation LP: #1897613
2026-06-11 10:34:18 +00:00 · 2020-11-25 18:10:45 +01:00
parent e4d2eaeff7
commit 32767f4e16
6 changed files with 1309 additions and 0 deletions
@@ -0,0 +1,80 @@
+From: Benjamin Berg <bberg@redhat.com>
+Date: Mon, 28 Sep 2020 19:13:32 +0200
+Subject: aes3k: Fix transfer error path and cancellable lifetime
+
+The cancellable needs to be free'ed at deactivation. Also free it if we
+run into a fatal error, which then in turn indicates that the device is
+deactivated already.
+
+Origin: https://gitlab.freedesktop.org/libfprint/libfprint/-/commit/8c4ff253c
+
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/1897613
+---
+ libfprint/drivers/aes3k.c | 19 +++++++++++++++----
+ 1 file changed, 15 insertions(+), 4 deletions(-)
+
+diff --git a/libfprint/drivers/aes3k.c b/libfprint/drivers/aes3k.c
+index db0d370..be3645e 100644
+--- a/libfprint/drivers/aes3k.c
+++ b/libfprint/drivers/aes3k.c
+@@ -76,6 +76,7 @@ img_cb (FpiUsbTransfer *transfer, FpDevice *device,
+ {
+   FpImageDevice *dev = FP_IMAGE_DEVICE (device);
+   FpiDeviceAes3k *self = FPI_DEVICE_AES3K (device);
+  FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (self);
+   FpiDeviceAes3kClass *cls = FPI_DEVICE_AES3K_GET_CLASS (self);
+   unsigned char *ptr = transfer->buffer;
+   FpImage *tmp;
+@@ -90,11 +91,14 @@ img_cb (FpiUsbTransfer *transfer, FpDevice *device,
+         {
+           /* Cancellation implies we are deactivating. */
+           g_error_free (error);
+          g_clear_object (&priv->img_trf_cancel);
+           fpi_image_device_deactivate_complete (dev, NULL);
+           return;
+         }
+ 
+       fpi_image_device_session_error (dev, error);
+      g_clear_object (&priv->img_trf_cancel);
+      return;
+     }
+ 
+   fpi_image_device_report_finger_status (dev, TRUE);
+@@ -144,9 +148,14 @@ do_capture (FpImageDevice *dev)
+ static void
+ init_reqs_cb (FpImageDevice *dev, GError *result, void *user_data)
+ {
+  FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (FPI_DEVICE_AES3K (dev));
+
+   fpi_image_device_activate_complete (dev, result);
+   if (!result)
+-    do_capture (dev);
+    {
+      priv->img_trf_cancel = g_cancellable_new ();
+      do_capture (dev);
+    }
+ }
+ 
+ static void
+@@ -157,7 +166,6 @@ aes3k_dev_activate (FpImageDevice *dev)
+   FpiDeviceAes3kClass *cls = FPI_DEVICE_AES3K_GET_CLASS (self);
+ 
+   g_assert (!priv->img_trf_cancel);
+-  priv->img_trf_cancel = g_cancellable_new ();
+   aes_write_regv (dev, cls->init_reqs, cls->init_reqs_len, init_reqs_cb, NULL);
+ }
+ 
+@@ -167,8 +175,11 @@ aes3k_dev_deactivate (FpImageDevice *dev)
+   FpiDeviceAes3k *self = FPI_DEVICE_AES3K (dev);
+   FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (self);
+ 
+-  /* Deactivation always finishes from the cancellation handler */
+-  g_cancellable_cancel (priv->img_trf_cancel);
+  /* Deactivation finishes from the cancellation handler */
+  if (priv->img_trf_cancel)
+    g_cancellable_cancel (priv->img_trf_cancel);
+  else
+    fpi_image_device_deactivate_complete (dev, NULL);
+ }
+ 
+ static void
@@ -0,0 +1,231 @@
+From: Benjamin Berg <bberg@redhat.com>
+Date: Thu, 1 Oct 2020 18:38:07 +0200
+Subject: aes3k: Re-send some commands for each capture
+
+The driver seems to have relied on the device to be fully
+deactivated and activated again for each capture. This is not the case
+during enroll, and as such in can cause issues.
+
+Try fixing this by splitting out the last few commands send to the
+device and assuming that this starts the capture.
+
+Fixes: #306
+
+Origin: https://gitlab.freedesktop.org/libfprint/libfprint/-/commit/81e0f4dfe
+
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/1897613
+---
+ libfprint/drivers/aes3500.c |  4 +++
+ libfprint/drivers/aes3k.c   | 75 ++++++++++++++++++++++++++++++++++-----------
+ libfprint/drivers/aes3k.h   |  2 ++
+ libfprint/drivers/aes4000.c |  4 +++
+ 4 files changed, 67 insertions(+), 18 deletions(-)
+
+diff --git a/libfprint/drivers/aes3500.c b/libfprint/drivers/aes3500.c
+index 3b4860b..58a9be1 100644
+--- a/libfprint/drivers/aes3500.c
+++ b/libfprint/drivers/aes3500.c
+@@ -106,7 +106,9 @@ static struct aes_regwrite init_reqs[] = {
+   { 0x9e, 0x53 },       /* clear challenge word bits */
+   { 0x9f, 0x6b },       /* set some challenge word bits */
+   { 0, 0 },
+};
+ 
+static struct aes_regwrite capture_reqs[] = {
+   { 0x80, 0x00 },
+   { 0x81, 0x00 },
+   { 0, 0 },
+@@ -155,4 +157,6 @@ fpi_device_aes3500_class_init (FpiDeviceAes3500Class *klass)
+   aes_class->enlarge_factor = ENLARGE_FACTOR;
+   aes_class->init_reqs = init_reqs;
+   aes_class->init_reqs_len = G_N_ELEMENTS (init_reqs);
+  aes_class->capture_reqs = capture_reqs;
+  aes_class->capture_reqs_len = G_N_ELEMENTS (capture_reqs);
+ }
+diff --git a/libfprint/drivers/aes3k.c b/libfprint/drivers/aes3k.c
+index be3645e..bab6815 100644
+--- a/libfprint/drivers/aes3k.c
+++ b/libfprint/drivers/aes3k.c
+@@ -42,7 +42,10 @@
+ 
+ typedef struct
+ {
+-  GCancellable *img_trf_cancel;
+  /* This is used both as a flag that we are in a capture operation
+   * and for cancellation.
+   */
+  GCancellable *img_capture_cancel;
+ } FpiDeviceAes3kPrivate;
+ 
+ #define CTRL_TIMEOUT 1000
+@@ -83,6 +86,9 @@ img_cb (FpiUsbTransfer *transfer, FpDevice *device,
+   FpImage *img;
+   int i;
+ 
+  /* Image capture operation is finished (error/completed) */
+  g_clear_object (&priv->img_capture_cancel);
+
+   if (error)
+     {
+       if (g_error_matches (error,
+@@ -91,13 +97,11 @@ img_cb (FpiUsbTransfer *transfer, FpDevice *device,
+         {
+           /* Cancellation implies we are deactivating. */
+           g_error_free (error);
+-          g_clear_object (&priv->img_trf_cancel);
+           fpi_image_device_deactivate_complete (dev, NULL);
+           return;
+         }
+ 
+       fpi_image_device_session_error (dev, error);
+-      g_clear_object (&priv->img_trf_cancel);
+       return;
+     }
+ 
+@@ -122,11 +126,10 @@ img_cb (FpiUsbTransfer *transfer, FpDevice *device,
+   fpi_image_device_image_captured (dev, img);
+ 
+   /* FIXME: rather than assuming finger has gone, we should poll regs until
+-   * it really has, then restart the capture */
+   * it really has. */
+   fpi_image_device_report_finger_status (dev, FALSE);
+ 
+-  /* Note: We always restart the transfer, it may already be cancelled though. */
+-  do_capture (dev);
+  /* Note: The transfer is re-started when we switch to the AWAIT_FINGER_ON state. */
+ }
+ 
+ static void
+@@ -141,31 +144,51 @@ do_capture (FpImageDevice *dev)
+   fpi_usb_transfer_fill_bulk (img_trf, EP_IN, cls->data_buflen);
+   img_trf->short_is_error = TRUE;
+   fpi_usb_transfer_submit (g_steal_pointer (&img_trf), 0,
+-                           priv->img_trf_cancel,
+                           priv->img_capture_cancel,
+                            img_cb, NULL);
+ }
+ 
+ static void
+-init_reqs_cb (FpImageDevice *dev, GError *result, void *user_data)
+capture_reqs_cb (FpImageDevice *dev, GError *result, void *user_data)
+ {
+-  FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (FPI_DEVICE_AES3K (dev));
+  FpiDeviceAes3k *self = FPI_DEVICE_AES3K (dev);
+  FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (self);
+ 
+-  fpi_image_device_activate_complete (dev, result);
+-  if (!result)
+  if (result)
+     {
+-      priv->img_trf_cancel = g_cancellable_new ();
+-      do_capture (dev);
+      g_clear_object (&priv->img_capture_cancel);
+      fpi_image_device_session_error (dev, result);
+      return;
+     }
+
+  /* FIXME: we never cancel a pending capture. So we are likely leaving the
+   * hardware in a bad state should we abort the capture operation and the
+   * user does not touch the device.
+   * But, we don't know how we might cancel, so just leave it as is. */
+  do_capture (dev);
+}
+
+static void
+do_capture_start (FpImageDevice *dev)
+{
+  FpiDeviceAes3k *self = FPI_DEVICE_AES3K (dev);
+  FpiDeviceAes3kClass *cls = FPI_DEVICE_AES3K_GET_CLASS (self);
+
+  aes_write_regv (dev, cls->capture_reqs, cls->capture_reqs_len, capture_reqs_cb, NULL);
+}
+
+static void
+init_reqs_cb (FpImageDevice *dev, GError *result, void *user_data)
+{
+  fpi_image_device_activate_complete (dev, result);
+ }
+ 
+ static void
+ aes3k_dev_activate (FpImageDevice *dev)
+ {
+   FpiDeviceAes3k *self = FPI_DEVICE_AES3K (dev);
+-  FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (self);
+   FpiDeviceAes3kClass *cls = FPI_DEVICE_AES3K_GET_CLASS (self);
+ 
+-  g_assert (!priv->img_trf_cancel);
+   aes_write_regv (dev, cls->init_reqs, cls->init_reqs_len, init_reqs_cb, NULL);
+ }
+ 
+@@ -175,13 +198,28 @@ aes3k_dev_deactivate (FpImageDevice *dev)
+   FpiDeviceAes3k *self = FPI_DEVICE_AES3K (dev);
+   FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (self);
+ 
+-  /* Deactivation finishes from the cancellation handler */
+-  if (priv->img_trf_cancel)
+-    g_cancellable_cancel (priv->img_trf_cancel);
+  /* If a capture is running, then deactivation finishes from the cancellation handler */
+  if (priv->img_capture_cancel)
+    g_cancellable_cancel (priv->img_capture_cancel);
+   else
+     fpi_image_device_deactivate_complete (dev, NULL);
+ }
+ 
+static void
+aes3k_dev_change_state (FpImageDevice *dev, FpiImageDeviceState state)
+{
+  FpiDeviceAes3k *self = FPI_DEVICE_AES3K (dev);
+  FpiDeviceAes3kPrivate *priv = fpi_device_aes3k_get_instance_private (self);
+
+  if (state == FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON)
+    {
+      g_assert (!priv->img_capture_cancel);
+      priv->img_capture_cancel = g_cancellable_new ();
+
+      do_capture_start (dev);
+    }
+}
+
+ static void
+ fpi_device_aes3k_init (FpiDeviceAes3k *self)
+ {
+@@ -224,6 +262,7 @@ fpi_device_aes3k_class_init (FpiDeviceAes3kClass *klass)
+   img_class->img_open = aes3k_dev_init;
+   img_class->img_close = aes3k_dev_deinit;
+   img_class->activate = aes3k_dev_activate;
+  img_class->change_state = aes3k_dev_change_state;
+   img_class->deactivate = aes3k_dev_deactivate;
+ 
+   /* Extremely low due to low image quality. */
+diff --git a/libfprint/drivers/aes3k.h b/libfprint/drivers/aes3k.h
+index 539f4e1..ae19bf1 100644
+--- a/libfprint/drivers/aes3k.h
+++ b/libfprint/drivers/aes3k.h
+@@ -57,4 +57,6 @@ struct _FpiDeviceAes3kClass
+   gsize                data_buflen;    /* buffer length of usb bulk transfer */
+   struct aes_regwrite *init_reqs;       /* initial values sent to device */
+   gsize                init_reqs_len;
+  struct aes_regwrite *capture_reqs;       /* capture values sent to device */
+  gsize                capture_reqs_len;
+ };
+diff --git a/libfprint/drivers/aes4000.c b/libfprint/drivers/aes4000.c
+index 0a801f6..e5770b9 100644
+--- a/libfprint/drivers/aes4000.c
+++ b/libfprint/drivers/aes4000.c
+@@ -103,7 +103,9 @@ static struct aes_regwrite init_reqs[] = {
+   { 0x9e, 0x53 },       /* clear challenge word bits */
+   { 0x9f, 0x6b },       /* set some challenge word bits */
+   { 0, 0 },
+};
+ 
+static struct aes_regwrite capture_reqs[] = {
+   { 0x80, 0x00 },
+   { 0x81, 0x00 },
+   { 0, 0 },
+@@ -152,4 +154,6 @@ fpi_device_aes4000_class_init (FpiDeviceAes4000Class *klass)
+   aes_class->enlarge_factor = ENLARGE_FACTOR;
+   aes_class->init_reqs = init_reqs;
+   aes_class->init_reqs_len = G_N_ELEMENTS (init_reqs);
+  aes_class->capture_reqs = capture_reqs;
+  aes_class->capture_reqs_len = G_N_ELEMENTS (capture_reqs);
+ }
@@ -0,0 +1,27 @@
+From: Benjamin Berg <bberg@redhat.com>
+Date: Mon, 28 Sep 2020 19:12:28 +0200
+Subject: image-device: Don't deactivate if deactivation was already started
+
+The test only prevent deactivation after it completed. Also guard
+against trying to deactivate a second time.
+
+Origin: https://gitlab.freedesktop.org/libfprint/libfprint/-/commit/3ce6a1554
+
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/1897613
+---
+ libfprint/fpi-image-device.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/libfprint/fpi-image-device.c b/libfprint/fpi-image-device.c
+index 8fc1b92..0cef485 100644
+--- a/libfprint/fpi-image-device.c
+++ b/libfprint/fpi-image-device.c
+@@ -71,7 +71,7 @@ fpi_image_device_deactivate (FpImageDevice *self)
+   FpImageDevicePrivate *priv = fp_image_device_get_instance_private (self);
+   FpImageDeviceClass *cls = FP_IMAGE_DEVICE_GET_CLASS (device);
+ 
+-  if (!priv->active)
+  if (!priv->active || priv->state == FPI_IMAGE_DEVICE_STATE_INACTIVE)
+     {
+       /* XXX: We currently deactivate both from minutiae scan result
+        *      and finger off report. */
@@ -1,2 +1,7 @@
 # Debian specifics
 udev-rules-creation-add-Debian-specifics.patch
+image-device-Don-t-deactivate-if-deactivation-was-already.patch
+aes3k-Fix-transfer-error-path-and-cancellable-lifetime.patch
+tests-Add-AES3500-test-case.patch
+usb-transfer-Work-around-libgusb-cancellation-issue.patch
+aes3k-Re-send-some-commands-for-each-capture.patch
@@ -0,0 +1,69 @@
+From: Benjamin Berg <bberg@redhat.com>
+Date: Thu, 1 Oct 2020 00:14:48 +0200
+Subject: usb-transfer: Work around libgusb cancellation issue
+
+We have plenty of code paths where a transfer may be cancelled before it
+is submitted. Unfortunately, libgusb up to and including version 0.3.6
+are not handling that case correctly (due to libusb ignoring
+cancellation on transfers that are not yet submitted).
+
+Work around this, but do so in a somewhat lazy fashion that is not
+entirely race free.
+
+Closes: #306
+
+Origin: https://gitlab.freedesktop.org/libfprint/libfprint/-/commit/c7cab77fc
+
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/1897613
+---
+ libfprint/fpi-usb-transfer.c | 30 ++++++++++++++++++++++++++++++
+ 1 file changed, 30 insertions(+)
+
+diff --git a/libfprint/fpi-usb-transfer.c b/libfprint/fpi-usb-transfer.c
+index 37033c8..fe49168 100644
+--- a/libfprint/fpi-usb-transfer.c
+++ b/libfprint/fpi-usb-transfer.c
+@@ -354,6 +354,24 @@ transfer_finish_cb (GObject *source_object, GAsyncResult *res, gpointer user_dat
+   fpi_usb_transfer_unref (transfer);
+ }
+ 
+static gboolean
+transfer_cancel_cb (FpiUsbTransfer *transfer)
+{
+  GError *error;
+  FpiUsbTransferCallback callback;
+
+  error = g_error_new_literal (G_IO_ERROR,
+                               G_IO_ERROR_CANCELLED,
+                               "Transfer was cancelled before being started");
+  callback = transfer->callback;
+  transfer->callback = NULL;
+  transfer->actual_length = -1;
+  callback (transfer, transfer->device, transfer->user_data, error);
+
+  fpi_usb_transfer_unref (transfer);
+
+  return G_SOURCE_REMOVE;
+}
+ 
+ /**
+  * fpi_usb_transfer_submit:
+@@ -387,6 +405,18 @@ fpi_usb_transfer_submit (FpiUsbTransfer        *transfer,
+ 
+   log_transfer (transfer, TRUE, NULL);
+ 
+  /* Work around libgusb cancellation issue, see
+   *   https://github.com/hughsie/libgusb/pull/42
+   * should be fixed with libgusb 0.3.7.
+   * Note that this is not race free, we rely on libfprint and API users
+   * not cancelling from a different thread here.
+   */
+  if (cancellable && g_cancellable_is_cancelled (cancellable))
+    {
+      g_idle_add ((GSourceFunc) transfer_cancel_cb, transfer);
+      return;
+    }
+
+   switch (transfer->type)
+     {
+     case FP_TRANSFER_BULK: