diff --git a/libfprint/drivers/goodixtls/goodix.c b/libfprint/drivers/goodixtls/goodix.c index d5cac78..c70169d 100644 --- a/libfprint/drivers/goodixtls/goodix.c +++ b/libfprint/drivers/goodixtls/goodix.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +55,7 @@ typedef struct { GoodixCallbackInfo* tls_ready_callback; GCancellable* transfer_cancel_tkn; + GSource* read_retry; gboolean inited; } FpiDeviceGoodixTlsPrivate; @@ -81,12 +83,18 @@ void goodix_receive_done(FpDevice *dev, guint8 *data, guint16 length, GoodixCmdCallback callback = priv->callback; gpointer user_data = priv->user_data; - if (!(priv->ack || priv->reply)) return; + if (!(priv->ack || priv->reply)) { + g_clear_error(&error); + return; + } goodix_reset_state(dev); if (!error) fp_dbg("Completed command: 0x%02x", priv->cmd); - if (callback) callback(dev, data, length, user_data, error); + if (callback) + callback(dev, data, length, user_data, error); + else + g_clear_error(&error); } void goodix_receive_none(FpDevice *dev, guint8 *data, guint16 length, @@ -352,7 +360,20 @@ void goodix_receive_pack(FpDevice *dev, guint8 *data, guint32 length) { case GOODIX_FLAGS_TLS_DATA: fp_dbg("Got TLS data msg"); + // A reply left over from a command that timed out must not be taken + // for the answer to the current one. + if (priv->cmd != GOODIX_CMD_MCU_GET_IMAGE) { + fp_warn("Unexpected TLS data during command: 0x%02x", priv->cmd); + break; + } // GOODIX 52xd: Remove first 9 to get valid TLS content + if (payload_len < 9) { + goodix_receive_done(dev, NULL, 0, + g_error_new(G_IO_ERROR, G_IO_ERROR_INVALID_DATA, + "Short TLS data message: %d", + payload_len)); + break; + } goodix_receive_done(dev, payload+9, payload_len-9, NULL); break; @@ -365,23 +386,42 @@ void goodix_receive_pack(FpDevice *dev, guint8 *data, guint32 length) { priv->length = 0; } +static void goodix_receive_retry_cb(FpDevice *dev, gpointer user_data) { + FpiDeviceGoodixTls *self = FPI_DEVICE_GOODIXTLS(dev); + FpiDeviceGoodixTlsPrivate *priv = + fpi_device_goodixtls_get_instance_private(self); + + priv->read_retry = NULL; + goodix_receive_data(dev); +} + void goodix_receive_data_cb(FpiUsbTransfer *transfer, FpDevice *dev, gpointer user_data, GError *error) { FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - if (g_cancellable_is_cancelled(priv->transfer_cancel_tkn)) { + // Match the error rather than priv->transfer_cancel_tkn: by now that may + // be gone, or a new one if the device was reopened. + if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { fp_dbg("transfer cancelled, aborting read loop..."); + g_error_free(error); return; } if (error) { - // Warn about error and free it. fp_warn("Receive data error: %s", error->message); + + // There will be nothing else to read. Fail what's waiting for a reply. + if (g_error_matches(error, G_USB_DEVICE_ERROR, G_USB_DEVICE_ERROR_NO_DEVICE)) { + priv->inited = FALSE; + goodix_receive_done(dev, NULL, 0, error); + return; + } g_error_free(error); - // Retry receiving data and return. - goodix_receive_data(dev); + // Retry receiving data, but don't spin on an error that persists. + priv->read_retry = fpi_device_add_timeout( + dev, GOODIX_READ_RETRY_MS, goodix_receive_retry_cb, NULL, NULL); return; } @@ -486,7 +526,7 @@ gboolean goodix_send_pack(FpDevice * dev, guint8 flags, guint8 * payload, return goodix_send_data(dev, data, data_len, g_free, error); } -void goodix_send_protocol( +gboolean goodix_send_protocol( FpDevice * dev, guint8 cmd, guint8 * payload, guint16 length, GDestroyNotify free_func, gboolean calc_checksum, guint timeout_ms, gboolean reply, GoodixCmdCallback callback, gpointer user_data) @@ -503,7 +543,12 @@ void goodix_send_protocol( fp_warn("A command is already running: 0x%02x", priv->cmd); if (free_func) free_func(payload); - return; + if (callback) + callback(dev, NULL, 0, user_data, + g_error_new(G_IO_ERROR, G_IO_ERROR_BUSY, + "A command is already running: 0x%02x", + priv->cmd)); + return FALSE; } fp_dbg("Running command: 0x%02x", cmd); @@ -525,8 +570,9 @@ void goodix_send_protocol( if (!goodix_send_pack(dev, GOODIX_FLAGS_MSG_PROTOCOL, data, data_len, g_free, &error)) { goodix_receive_done(dev, NULL, 0, error); - return; + return FALSE; }; + return TRUE; } void goodix_send_nop(FpDevice *dev, GoodixNoneCallback callback, gpointer user_data) { @@ -539,16 +585,17 @@ void goodix_send_nop(FpDevice *dev, GoodixNoneCallback callback, cb_info->callback = G_CALLBACK(callback); cb_info->user_data = user_data; - goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, - sizeof(payload), NULL, FALSE, GOODIX_TIMEOUT, FALSE, - goodix_receive_none, cb_info); - goodix_receive_done(dev, NULL, 0, NULL); + if (goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, + sizeof(payload), NULL, FALSE, GOODIX_TIMEOUT, FALSE, + goodix_receive_none, cb_info)) + goodix_receive_done(dev, NULL, 0, NULL); return; } - goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, sizeof(payload), - NULL, FALSE, GOODIX_TIMEOUT, FALSE, NULL, NULL); - goodix_receive_done(dev, NULL, 0, NULL); + if (goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, + sizeof(payload), NULL, FALSE, GOODIX_TIMEOUT, FALSE, + NULL, NULL)) + goodix_receive_done(dev, NULL, 0, NULL); } void goodix_send_mcu_get_image(FpDevice* dev, guint8* payload, guint16 length, GoodixImageCallback callback, @@ -589,13 +636,13 @@ void goodix_send_mcu_switch_to_fdt_down(FpDevice *dev, guint8 *mode, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_DOWN, mode, length, - free_func, TRUE, 0, reply, goodix_receive_default, + free_func, TRUE, GOODIX_TIMEOUT, reply, goodix_receive_default, cb_info); return; } goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_DOWN, mode, length, - free_func, TRUE, 0, reply, NULL, NULL); + free_func, TRUE, GOODIX_TIMEOUT, reply, NULL, NULL); } void goodix_send_mcu_switch_to_fdt_up(FpDevice *dev, guint8 *mode, @@ -611,13 +658,13 @@ void goodix_send_mcu_switch_to_fdt_up(FpDevice *dev, guint8 *mode, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_UP, mode, length, - free_func, TRUE, 0, reply, goodix_receive_default, + free_func, TRUE, GOODIX_TIMEOUT, reply, goodix_receive_default, cb_info); return; } goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_UP, mode, length, - free_func, TRUE, 0, reply, NULL, NULL); + free_func, TRUE, GOODIX_TIMEOUT, reply, NULL, NULL); } void goodix_send_mcu_switch_to_fdt_mode(FpDevice *dev, guint8 *mode, @@ -634,13 +681,13 @@ void goodix_send_mcu_switch_to_fdt_mode(FpDevice *dev, guint8 *mode, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_MODE, mode, length, - free_func, TRUE, 0, reply, goodix_receive_default, + free_func, TRUE, GOODIX_TIMEOUT, reply, goodix_receive_default, cb_info); return; } goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_MODE, mode, length, - free_func, TRUE, 0, reply, NULL, NULL); + free_func, TRUE, GOODIX_TIMEOUT, reply, NULL, NULL); } void goodix_send_nav_0(FpDevice *dev, GoodixDefaultCallback callback, @@ -897,8 +944,9 @@ void goodix_send_request_tls_connection(FpDevice* dev, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_REQUEST_TLS_CONNECTION, - (guint8*) &payload, sizeof(payload), NULL, TRUE, 0, - TRUE, goodix_receive_default, cb_info); + (guint8*) &payload, sizeof(payload), NULL, TRUE, + GOODIX_TLS_IO_TIMEOUT_MS, TRUE, + goodix_receive_default, cb_info); return; } @@ -1033,6 +1081,7 @@ gboolean goodix_dev_init(FpDevice *dev, GError **error) { priv->user_data = NULL; priv->data = NULL; priv->length = 0; + g_clear_object(&priv->transfer_cancel_tkn); priv->transfer_cancel_tkn = g_cancellable_new(); return g_usb_device_claim_interface(fpi_device_get_usb_device(dev), @@ -1050,6 +1099,18 @@ void goodix_reset_state(FpDevice* dev) priv->reply = FALSE; priv->callback = NULL; priv->user_data = NULL; + // Whatever was being assembled belongs to the command that just ended. + g_clear_pointer(&priv->data, g_free); + priv->length = 0; +} + +gboolean goodix_command_in_flight(FpDevice* dev, guint8 cmd) +{ + FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); + FpiDeviceGoodixTlsPrivate* priv = + fpi_device_goodixtls_get_instance_private(self); + + return priv->cmd == cmd && (priv->ack || priv->reply); } gboolean goodix_dev_deinit(FpDevice *dev, GError **error) { @@ -1058,10 +1119,13 @@ gboolean goodix_dev_deinit(FpDevice *dev, GError **error) { FpiDeviceGoodixTlsPrivate *priv = fpi_device_goodixtls_get_instance_private(self); - if (priv->timeout) g_source_destroy(priv->timeout); - g_free(priv->data); + g_clear_pointer(&priv->read_retry, g_source_destroy); + g_clear_pointer(&priv->data, g_free); + priv->length = 0; g_cancellable_cancel(priv->transfer_cancel_tkn); + g_clear_object(&priv->transfer_cancel_tkn); goodix_shutdown_tls(dev, error); + g_clear_pointer(&priv->tls_ready_callback, g_free); goodix_reset_state(dev); priv->inited = FALSE; @@ -1084,6 +1148,8 @@ void goodix_read_tls(FpDevice* dev, GoodixTlsCallback callback, FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); + priv->timeout = fpi_device_add_timeout(dev, GOODIX_TLS_IO_TIMEOUT_MS, + goodix_receive_timeout_cb, NULL, NULL); priv->callback = callback; priv->user_data = user_data; priv->reply = TRUE; @@ -1099,8 +1165,12 @@ enum tls_states { static void on_goodix_tls_server_ready(GoodixTlsServer* server, GError* err, gpointer dev) { + // Runs on the server thread. A failure here is also seen by the main loop, + // as a handshake step that errors out or times out; and it's expected when + // the server is torn down in the middle of a handshake. if (err) { - fp_err("server ready failed: %s", err->message); + fp_dbg("TLS server stopped: %s", err->message); + g_error_free(err); return; } fp_dbg("TLS connection ready"); @@ -1142,6 +1212,22 @@ enum goodix_tls_handshake_stages { TLS_HANDSHAKE_STAGE_NUM, }; +// Tell goodix_tls()'s caller how it went: exactly once, whichever way it fails. +static void goodix_tls_complete(FpDevice* dev, GError* error) +{ + FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); + FpiDeviceGoodixTlsPrivate* priv = + fpi_device_goodixtls_get_instance_private(self); + g_autofree GoodixCallbackInfo* info = + g_steal_pointer(&priv->tls_ready_callback); + + if (!info) { + g_clear_error(&error); + return; + } + ((GoodixNoneCallback) info->callback)(dev, info->user_data, error); +} + static void on_tls_successfully_established(FpDevice* dev, gpointer user_data, GError* error) { @@ -1149,14 +1235,17 @@ static void on_tls_successfully_established(FpDevice* dev, gpointer user_data, FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - ((GoodixNoneCallback) priv->tls_ready_callback->callback)( - dev, priv->tls_ready_callback->user_data, NULL); + if (!error) + goodix_tls_server_set_nonblocking(priv->tls_hop, &error); + goodix_tls_complete(dev, error); } static void tls_handshake_done(FpiSsm* ssm, FpDevice* dev, GError* error) { if (error) { fp_dbg("failed to do tls handshake: %s (code: %d)", error->message, error->code); + goodix_tls_complete(dev, error); + return; } goodix_send_tls_successfully_established( dev, on_tls_successfully_established, NULL); @@ -1172,7 +1261,7 @@ static void tls_handshake_run(FpiSsm* ssm, FpDevice* dev) if (stage == TLS_HANDSHAKE_STAGE_HELLO_S) { guint8 buff[1024]; int size = goodix_tls_client_recv(priv->tls_hop, buff, sizeof(buff)); - if (size < 0) { + if (size <= 0) { fpi_ssm_mark_failed(ssm, g_error_new(g_io_error_quark(), size, "failed to read tls server " "hello")); @@ -1194,7 +1283,7 @@ static void tls_handshake_run(FpiSsm* ssm, FpDevice* dev) fp_dbg("Reading to proxy back"); guint8 buff[1024]; int size = goodix_tls_client_recv(priv->tls_hop, buff, sizeof(buff)); - if (size < 0) { + if (size <= 0) { fpi_ssm_mark_failed(ssm, g_error_new(g_io_error_quark(), size, "failed to read server " "handshake")); @@ -1221,28 +1310,22 @@ static void on_goodix_request_tls_connection(FpDevice* dev, guint8* data, GError* error) { if (error) { - fp_err("failed to get tls handshake: %s", error->message); - goodix_send_tls_successfully_established(FP_DEVICE(dev), NULL, NULL); + goodix_tls_complete(dev, error); return; } FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(user_data); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - goodix_tls_client_send(priv->tls_hop, data, length); - - do_tls_handshake(dev); -} - -static void goodix_tls_ready(GoodixTlsServer* server, GError* err, gpointer dev) -{ - if (err) { - fp_err("failed to init tls server: %s, code: %d", err->message, - err->code); + if (goodix_tls_client_send(priv->tls_hop, data, length) < 0) { + goodix_tls_complete(dev, + g_error_new(G_IO_ERROR, g_io_error_from_errno(errno), + "failed to pass the client hello to the " + "TLS server: %s", g_strerror(errno))); return; } - goodix_send_request_tls_connection(FP_DEVICE(dev), - on_goodix_request_tls_connection, dev); + + do_tls_handshake(dev); } void goodix_tls(FpDevice* dev, GoodixNoneCallback callback, gpointer user_data) @@ -1251,12 +1334,14 @@ void goodix_tls(FpDevice* dev, GoodixNoneCallback callback, gpointer user_data) FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - g_assert(priv->tls_hop == NULL); - priv->tls_hop = malloc(sizeof(GoodixTlsServer)); - - if (!priv->tls_ready_callback) { - priv->tls_ready_callback = malloc(sizeof(GoodixCallbackInfo)); + if (priv->tls_hop) { + fp_warn("A TLS server is still around, replacing it"); + goodix_shutdown_tls(dev, NULL); } + priv->tls_hop = g_new0(GoodixTlsServer, 1); + + g_clear_pointer(&priv->tls_ready_callback, g_free); + priv->tls_ready_callback = g_new0(GoodixCallbackInfo, 1); priv->tls_ready_callback->callback = G_CALLBACK(callback); priv->tls_ready_callback->user_data = user_data; GoodixTlsServer* s = priv->tls_hop; @@ -1264,12 +1349,12 @@ void goodix_tls(FpDevice* dev, GoodixNoneCallback callback, gpointer user_data) s->user_data = self; GError* err = NULL; if (!goodix_tls_server_init(priv->tls_hop, &err)) { - fp_err("failed to init tls server, error: %s, code: %d", err->message, - err->code); + goodix_tls_complete(dev, err); return; } - goodix_tls_ready(s, err, self); + goodix_send_request_tls_connection(dev, on_goodix_request_tls_connection, + self); } gboolean goodix_shutdown_tls(FpDevice* dev, GError** error) @@ -1301,10 +1386,17 @@ static void goodix_tls_ready_image_handler(FpDevice* dev, guint8* data, FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - goodix_tls_client_send(priv->tls_hop, data, length); + if (goodix_tls_client_send(priv->tls_hop, data, length) < 0) { + callback(dev, NULL, 0, cb_info->user_data, + g_error_new(G_IO_ERROR, g_io_error_from_errno(errno), + "failed to pass the image to the TLS server: %s", + g_strerror(errno))); + g_free(cb_info); + return; + } const guint16 size = -1; - guint8* buff = malloc(size); + g_autofree guint8* buff = g_malloc(size); GError* err = NULL; int read_size = goodix_tls_server_receive(priv->tls_hop, buff, size, &err); if (read_size <= 0) { diff --git a/libfprint/drivers/goodixtls/goodix.h b/libfprint/drivers/goodixtls/goodix.h index 1b75997..0345535 100644 --- a/libfprint/drivers/goodixtls/goodix.h +++ b/libfprint/drivers/goodixtls/goodix.h @@ -22,6 +22,9 @@ // 1 seconds USB timeout #define GOODIX_TIMEOUT (1000) +// Pause before reading again after a failed USB transfer +#define GOODIX_READ_RETRY_MS (100) + G_DECLARE_DERIVABLE_TYPE(FpiDeviceGoodixTls, fpi_device_goodixtls, FPI, DEVICE_GOODIXTLS, FpDevice) @@ -128,7 +131,10 @@ gboolean goodix_send_pack(FpDevice *dev, guint8 flags, guint8 *payload, guint16 length, GDestroyNotify free_func, GError **error); -void goodix_send_protocol(FpDevice *dev, guint8 cmd, guint8 *payload, +// Returns TRUE if the command went out. Otherwise, the callback has already +// been called with the error: a command that doesn't start must not leave its +// caller waiting. +gboolean goodix_send_protocol(FpDevice *dev, guint8 cmd, guint8 *payload, guint16 length, GDestroyNotify free_func, gboolean calc_checksum, guint timeout_ms, gboolean reply, GoodixCmdCallback callback, @@ -229,6 +235,9 @@ gboolean goodix_dev_deinit(FpDevice *dev, GError **error); void goodix_reset_state(FpDevice* dev); +// Whether cmd was sent and its ack or reply are still awaited. +gboolean goodix_command_in_flight(FpDevice* dev, guint8 cmd); + // ---- DEV SECTION END ---- // ----------------------------------------------------------------------------- diff --git a/libfprint/drivers/goodixtls/goodix53xd.c b/libfprint/drivers/goodixtls/goodix53xd.c index ac0486b..e01d849 100644 --- a/libfprint/drivers/goodixtls/goodix53xd.c +++ b/libfprint/drivers/goodixtls/goodix53xd.c @@ -120,6 +120,9 @@ struct _FpiDeviceGoodixTls53XD { // before calling fpi_ssm_mark_failed(), which asserts none is pending. gboolean poll_timeout_pending; + // TRUE from dev_cancel() until the action has actually been failed. + gboolean cancel_pending; + // Deferred verify/identify result reporting gboolean verify_wait_finger_up; gboolean pending_result_report; @@ -403,6 +406,15 @@ static void open_ssm_done(FpiSsm* ssm, FpDevice* dev, GError* error) FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); self->task_ssm = NULL; + if (error) { + // A failed open doesn't get a dev_close(). + g_autoptr(GError) deinit_error = NULL; + + g_clear_pointer(&self->otp, g_free); + g_clear_pointer(&self->calib, g_array_unref); + if (!goodix_dev_deinit(dev, &deinit_error)) + fp_dbg("cleanup after failed open: %s", deinit_error->message); + } fpi_device_open_complete(dev, error); } @@ -499,9 +511,18 @@ static void dump_empty_sample(FpiDeviceGoodixTls53XD* self) dump_sample(self, "empty"); } +static void finish_cancel(FpDevice* dev); + static void on_capture_image(FpDevice* dev, guint8* data, guint16 len, gpointer ssm, GError* err) { + FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); + + if (self->cancel_pending) { + g_clear_error(&err); + finish_cancel(dev); + return; + } if (err) { fpi_ssm_mark_failed(ssm, err); return; @@ -513,7 +534,6 @@ static void on_capture_image(FpDevice* dev, guint8* data, guint16 len, return; } - FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); Goodix53xdPix* frame = self->captured_raw; decode_frame(frame, data); @@ -585,11 +605,10 @@ static void on_fdt_down_second(FpDevice* dev, guint8* data, guint16 len, gpointer ssm, GError* err) { if (err) { - // Treat a transfer timeout as "no finger yet" and re-arm so the user - // gets more than one USB timeout window to press. (Needs hardware - // tuning; a real finger-up/finger-down FDT event would be cleaner.) - if (g_error_matches(err, G_USB_DEVICE_ERROR, - G_USB_DEVICE_ERROR_TIMED_OUT)) { + // Treat a command timeout as "no finger yet" and re-arm so the user + // gets more than one timeout window to press. (Needs hardware tuning; + // a real finger-up/finger-down FDT event would be cleaner.) + if (g_error_matches(err, G_IO_ERROR, G_IO_ERROR_TIMED_OUT)) { g_clear_error(&err); capture_fdt_down_arm(dev, ssm); return; @@ -1123,6 +1142,8 @@ static void dev_open(FpDevice* dev) FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); GError* error = NULL; + self->cancel_pending = FALSE; + g_autofree char* path = calib_path(); g_autoptr(GError) calib_error = NULL; g_clear_pointer(&self->calib, g_array_unref); @@ -1166,6 +1187,7 @@ static void dev_enroll(FpDevice* dev) { FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); + self->cancel_pending = FALSE; self->enroll_stage = 0; g_clear_pointer(&self->enroll_images, g_ptr_array_unref); self->enroll_images = g_ptr_array_new_with_free_func(g_free); @@ -1202,11 +1224,47 @@ static void dev_verify(FpDevice* dev) clear_pending_result(self); self->action_result_reported = FALSE; self->verify_wait_finger_up = FALSE; + self->cancel_pending = FALSE; self->task_ssm = fpi_ssm_new(dev, verify_run_state, VERIFY_NUM_STATES); fpi_ssm_start(self->task_ssm, verify_ssm_done); } +static void on_cancel_idle_done(FpDevice* dev, gpointer user_data, GError* error) +{ + FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); + + if (error) { + fp_dbg("MCU didn't confirm idle mode on cancel: %s", error->message); + g_error_free(error); + } + self->cancel_pending = FALSE; + if (self->task_ssm) + fpi_ssm_mark_failed(self->task_ssm, + g_error_new(G_IO_ERROR, G_IO_ERROR_CANCELLED, + "Cancelled")); +} + +static void finish_cancel(FpDevice* dev) +{ + goodix_reset_state(dev); + + // A FDT_DOWN "wait for finger" (0x32) may still be outstanding on + // the MCU itself when we get here: goodix_reset_state() only clears + // our own ack/reply/timeout bookkeeping, it can't un-send bytes + // already on the wire. Left alone, the MCU eventually replies to + // that stale FDT_DOWN once a *new* command is already in flight, and + // its cmd byte (0x32) no longer matches priv->cmd -> permanent + // "Invalid protocol/ACK command" desync for the rest of this + // fprintd process's life (confirmed in the field: only a full + // fprintd restart recovers). Explicitly switching the MCU to idle + // closes out any pending FDT sequence on its side before the next + // verify attempt starts. The action only ends on its ack or + // GOODIX_TIMEOUT: the next action's first command would otherwise + // find this one still running. + goodix_send_mcu_switch_to_idle_mode(dev, 20, on_cancel_idle_done, NULL); +} + static void dev_cancel(FpDevice* dev) { FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); @@ -1221,26 +1279,16 @@ static void dev_cancel(FpDevice* dev) fpi_ssm_cancel_delayed_state_change(self->task_ssm); self->poll_timeout_pending = FALSE; } - goodix_reset_state(dev); - - // A FDT_DOWN "wait for finger" (0x32) may still be outstanding on - // the MCU itself when we get here: goodix_reset_state() only clears - // our own ack/reply/timeout bookkeeping, it can't un-send bytes - // already on the wire. Left alone, the MCU eventually replies to - // that stale FDT_DOWN once a *new* command is already in flight, and - // its cmd byte (0x32) no longer matches priv->cmd -> permanent - // "Invalid protocol/ACK command" desync for the rest of this - // fprintd process's life (confirmed in the field: only a full - // fprintd restart recovers). Explicitly switching the MCU to idle - // closes out any pending FDT sequence on its side before the next - // verify attempt starts. Fire-and-forget: priv state is already - // clear so it can't collide with the next command, and it - // self-resolves via ack-or-GOODIX_TIMEOUT either way. - goodix_send_mcu_switch_to_idle_mode(dev, 20, NULL, NULL); + self->cancel_pending = TRUE; - fpi_ssm_mark_failed(self->task_ssm, - g_error_new(G_IO_ERROR, G_IO_ERROR_CANCELLED, - "Cancelled")); + // Every image is a TLS record, numbered in sequence: dropping the one + // on its way would fail all the captures after it, until the device + // is reopened. Let on_capture_image() take it to the TLS server and + // carry on from there; the command's timeout guarantees it gets called. + if (goodix_command_in_flight(dev, GOODIX_CMD_MCU_GET_IMAGE)) + return; + + finish_cancel(dev); } } diff --git a/libfprint/drivers/goodixtls/goodixtls.c b/libfprint/drivers/goodixtls/goodixtls.c index 3596420..ad363ba 100644 --- a/libfprint/drivers/goodixtls/goodixtls.c +++ b/libfprint/drivers/goodixtls/goodixtls.c @@ -19,6 +19,8 @@ #include #include +#include +#include #include #include #include @@ -30,6 +32,7 @@ #include #include #include +#include #include "drivers_api.h" #include "fp-device.h" @@ -38,15 +41,14 @@ #include "goodix.h" #include "goodixtls.h" -static GError* err_from_ssl(void) +// The queue can be empty, and then there is no reason string. +static GError* err_from_ssl(const char* what) { - GError* err = malloc(sizeof(GError)); unsigned long code = ERR_get_error(); - err->code = code; - const char* msg = ERR_reason_error_string(code); - err->message = malloc(strlen(msg) + 1); - strcpy(err->message, msg); - return err; + const char* reason = code ? ERR_reason_error_string(code) : NULL; + + return g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED, "%s: %s", what, + reason ? reason : "no OpenSSL error queued"); } static unsigned int tls_server_psk_server_callback(SSL *ssl, @@ -99,15 +101,32 @@ int goodix_tls_client_send(GoodixTlsServer* self, guint8* data, guint16 length) return write(self->client_fd, data, length * sizeof(guint8)); } int goodix_tls_client_recv(GoodixTlsServer* self, guint8* data, guint16 length) { + struct pollfd pfd = {.fd = self->client_fd, .events = POLLIN}; + int ready = poll(&pfd, 1, GOODIX_TLS_IO_TIMEOUT_MS); + + if (ready <= 0) { + if (ready == 0) + errno = ETIMEDOUT; + return -1; + } return read(self->client_fd, data, length * sizeof(guint8)); } int goodix_tls_server_receive(GoodixTlsServer* self, guint8* data, guint32 length, GError** error) { + ERR_clear_error(); int retr = SSL_read(self->ssl_layer, data, length * sizeof(guint8)); if (retr <= 0) { - *error = err_from_ssl(); + // Every USB packet carries whole records and nothing else feeds the + // socket, so waiting for the rest of one would never end. + int ssl_error = SSL_get_error(self->ssl_layer, retr); + if (ssl_error == SSL_ERROR_WANT_READ || + ssl_error == SSL_ERROR_WANT_WRITE) + *error = g_error_new(G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT, + "Incomplete TLS record"); + else + *error = err_from_ssl("TLS read failed"); } return retr; } @@ -128,7 +147,8 @@ static void* goodix_tls_init_serve(void* me) int retr = SSL_accept(self->ssl_layer); fp_dbg("TLS server accept done"); if (retr <= 0) { - self->connection_callback(self, err_from_ssl(), self->user_data); + self->connection_callback(self, err_from_ssl("TLS handshake failed"), + self->user_data); } else { self->connection_callback(self, NULL, self->user_data); @@ -136,15 +156,47 @@ static void* goodix_tls_init_serve(void* me) return NULL; } +static gboolean set_nonblocking(int fd) +{ + int flags = fcntl(fd, F_GETFL); + + return flags != -1 && fcntl(fd, F_SETFL, flags | O_NONBLOCK) != -1; +} + +gboolean goodix_tls_server_set_nonblocking(GoodixTlsServer* self, GError** error) +{ + if (set_nonblocking(self->sock_fd) && set_nonblocking(self->client_fd)) + return TRUE; + + g_set_error(error, G_IO_ERROR, g_io_error_from_errno(errno), + "failed to make TLS sockets non-blocking: %s", g_strerror(errno)); + return FALSE; +} + gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error) { - SSL_shutdown(self->ssl_layer); - SSL_free(self->ssl_layer); + // End of file gets a handshake still waiting for its client out of + // SSL_accept(), and then it can't use ssl_layer anymore. + if (self->client_fd >= 0) + shutdown(self->client_fd, SHUT_WR); + if (self->serve_thread_started) + pthread_join(self->serve_thread, NULL); + + if (self->ssl_layer) { + // Nobody reads the close_notify, don't wait for room for it. + set_nonblocking(self->sock_fd); + SSL_shutdown(self->ssl_layer); + SSL_free(self->ssl_layer); + ERR_clear_error(); + } - close(self->client_fd); - close(self->sock_fd); + if (self->client_fd >= 0) + close(self->client_fd); + if (self->sock_fd >= 0) + close(self->sock_fd); - SSL_CTX_free(self->ssl_ctx); + if (self->ssl_ctx) + SSL_CTX_free(self->ssl_ctx); return TRUE; } @@ -152,10 +204,20 @@ gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error) gboolean goodix_tls_server_init(GoodixTlsServer* self, GError** error) { g_assert(self->connection_callback); + self->sock_fd = -1; + self->client_fd = -1; SSL_load_error_strings(); OpenSSL_add_ssl_algorithms(); SSL_library_init(); self->ssl_ctx = tls_server_create_ctx(); + if (self->ssl_ctx == NULL) { + fp_dbg("Unable to create TLS server context\n"); + *error = fpi_device_error_new_msg(FP_DEVICE_ERROR_GENERAL, "Unable to " + "create TLS " + "server " + "context"); + return FALSE; + } tls_server_config_ctx(self->ssl_ctx); int socks[2] = {0, 0}; @@ -167,19 +229,23 @@ gboolean goodix_tls_server_init(GoodixTlsServer* self, GError** error) self->sock_fd = socks[0]; self->client_fd = socks[1]; - if (self->ssl_ctx == NULL) { - fp_dbg("Unable to create TLS server context\n"); - *error = fpi_device_error_new_msg(FP_DEVICE_ERROR_GENERAL, "Unable to " - "create TLS " - "server " - "context"); + self->ssl_layer = SSL_new(self->ssl_ctx); + if (self->ssl_layer == NULL) { + *error = err_from_ssl("failed to create the TLS connection"); return FALSE; } - self->ssl_layer = SSL_new(self->ssl_ctx); tls_config_ssl(self->ssl_layer); SSL_set_fd(self->ssl_layer, self->sock_fd); - pthread_create(&self->serve_thread, 0, goodix_tls_init_serve, self); + int thread_error = + pthread_create(&self->serve_thread, 0, goodix_tls_init_serve, self); + if (thread_error != 0) { + g_set_error(error, G_IO_ERROR, g_io_error_from_errno(thread_error), + "failed to start the TLS server thread: %s", + g_strerror(thread_error)); + return FALSE; + } + self->serve_thread_started = TRUE; return TRUE; } diff --git a/libfprint/drivers/goodixtls/goodixtls.h b/libfprint/drivers/goodixtls/goodixtls.h index a7d5b94..710b3de 100644 --- a/libfprint/drivers/goodixtls/goodixtls.h +++ b/libfprint/drivers/goodixtls/goodixtls.h @@ -25,6 +25,9 @@ #define GOODIX_TLS_SERVER_PORT 4433 +// How long the handshake waits for the TLS server thread to produce output +#define GOODIX_TLS_IO_TIMEOUT_MS 2000 + // static const guint8 goodix_511_psk_0[64] = {0}; static const guint8 goodix_511_psk_0[] = { 0xba, 0x1a, 0x86, 0x03, 0x7c, 0x1d, 0x3c, 0x71, 0xc3, 0xaf, 0x34, @@ -68,6 +71,7 @@ typedef struct _GoodixTlsServer { // SSL* cli_ssl_layer; int client_fd; pthread_t serve_thread; + gboolean serve_thread_started; } GoodixTlsServer; // This is called only once to init the TLS server. @@ -76,6 +80,10 @@ gboolean goodix_tls_server_init(GoodixTlsServer* self, GError** error); gboolean goodix_tls_init_cli(GoodixTlsServer* self, GError** err); +// Called once the handshake is done. The sockets are only used from the main +// loop from then on, which must never block on them. +gboolean goodix_tls_server_set_nonblocking(GoodixTlsServer* self, GError** error); + // This can be called multiple times. It is called when the device send a TLS // packet. int goodix_tls_server_receive(GoodixTlsServer* self, guint8* data, @@ -85,6 +93,7 @@ int goodix_tls_client_send(GoodixTlsServer* self, guint8* data, guint16 length); int goodix_tls_client_recv(GoodixTlsServer* self, guint8* data, guint16 length); -// This is called only once to deinit the TLS server. +// This is called only once to deinit the TLS server, even if init failed or +// the handshake is still running. // Return TRUE on success, FALSE otherwise and error should be set. gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error); diff --git a/tests/meson.build b/tests/meson.build index aad0675..c8f8f5a 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -321,6 +321,29 @@ if 'virtual_image' in drivers ] endif +if 'goodixtls53xd' in drivers + # Built from the driver's source: the drivers aren't in a library that + # tests can link to, and this part doesn't need a device. + goodixtls_server_test = executable('test-goodixtls-server', + sources: [ + 'test-goodixtls-server.c', + '../libfprint/drivers/goodixtls/goodixtls.c', + ], + include_directories: include_directories( + '../libfprint/drivers', + '../libfprint/drivers/goodixtls', + ), + dependencies: [ libfprint_private_dep, openssl_dep, threads_dep ], + c_args: common_cflags, + install: false, + ) + test('goodixtls-server', + goodixtls_server_test, + suite: ['unit-tests'], + env: envs, + ) +endif + unit_tests_deps = { 'fpi-assembling' : [cairo_dep] } foreach test_name: unit_tests diff --git a/tests/test-goodixtls-server.c b/tests/test-goodixtls-server.c new file mode 100644 index 0000000..cad1ba3 --- /dev/null +++ b/tests/test-goodixtls-server.c @@ -0,0 +1,235 @@ +/* + * Unit tests for the goodixtls TLS server: what the main loop gets when the + * data coming from the sensor is not what it should be. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "goodixtls.h" + +/* A test that blocks on a socket would otherwise hang instead of failing. */ +#define HANG_LIMIT_SECONDS 30 + +typedef struct +{ + GoodixTlsServer server; + SSL_CTX *client_ctx; + SSL *client; +} Fixture; + +static void +on_server_ready (GoodixTlsServer *server, GError *error, gpointer user_data) +{ + g_clear_error (&error); +} + +static unsigned int +client_psk_cb (SSL *ssl, const char *hint, char *identity, + unsigned int max_identity_len, unsigned char *psk, + unsigned int max_psk_len) +{ + g_assert_cmpuint (max_psk_len, >=, 32); + memset (psk, 0, 32); + g_strlcpy (identity, "test", max_identity_len); + return 32; +} + +static void +fixture_init_server (Fixture *f) +{ + g_autoptr(GError) error = NULL; + + memset (f, 0, sizeof (*f)); + f->server.connection_callback = on_server_ready; + g_assert_true (goodix_tls_server_init (&f->server, &error)); + g_assert_no_error (error); +} + +static void +fixture_setup_connected (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + + fixture_init_server (f); + + f->client_ctx = SSL_CTX_new (TLS_client_method ()); + g_assert_nonnull (f->client_ctx); + SSL_CTX_set_min_proto_version (f->client_ctx, TLS1_2_VERSION); + SSL_CTX_set_max_proto_version (f->client_ctx, TLS1_2_VERSION); + g_assert_cmpint (SSL_CTX_set_cipher_list (f->client_ctx, "PSK"), ==, 1); + SSL_CTX_set_psk_client_callback (f->client_ctx, client_psk_cb); + + f->client = SSL_new (f->client_ctx); + SSL_set_fd (f->client, f->server.client_fd); + g_assert_cmpint (SSL_connect (f->client), ==, 1); + + g_assert_true (goodix_tls_server_set_nonblocking (&f->server, &error)); + g_assert_no_error (error); +} + +static void +fixture_teardown (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + + g_assert_true (goodix_tls_server_deinit (&f->server, &error)); + g_assert_no_error (error); + g_clear_pointer (&f->client, SSL_free); + g_clear_pointer (&f->client_ctx, SSL_CTX_free); +} + +static void +test_whole_record (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + const char message[] = "image"; + guint8 buffer[64]; + + g_assert_cmpint (SSL_write (f->client, message, sizeof (message)), ==, sizeof (message)); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + ==, sizeof (message)); + g_assert_no_error (error); + g_assert_cmpstr ((const char *) buffer, ==, message); +} + +/* This used to block forever, and fprintd with it. */ +static void +test_incomplete_record (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + const guint8 start_of_record[] = { 0x17, 0x03, 0x03, 0x10, 0x00, 0xaa }; + guint8 buffer[64]; + + g_assert_cmpint (goodix_tls_client_send (&f->server, (guint8 *) start_of_record, + sizeof (start_of_record)), + ==, sizeof (start_of_record)); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT); +} + +static void +test_nothing_to_read (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + guint8 buffer[64]; + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT); +} + +static void +test_end_of_file (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + guint8 buffer[64]; + + g_assert_cmpint (shutdown (f->server.client_fd, SHUT_WR), ==, 0); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED); + g_assert_nonnull (error->message); +} + +/* A record the TLS session can't authenticate, as after one was dropped. */ +static void +test_corrupt_record (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + guint8 record[5 + 64] = { 0x17, 0x03, 0x03, 0x00, 64 }; + guint8 buffer[64]; + + g_assert_cmpint (goodix_tls_client_send (&f->server, record, sizeof (record)), + ==, sizeof (record)); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED); +} + +static void +test_client_recv_timeout (Fixture *f, gconstpointer data) +{ + guint8 buffer[64]; + + errno = 0; + g_assert_cmpint (goodix_tls_client_recv (&f->server, buffer, sizeof (buffer)), ==, -1); + g_assert_cmpint (errno, ==, ETIMEDOUT); +} + +/* The server thread is waiting for a client hello that will never come. */ +static void +test_deinit_during_handshake (void) +{ + g_autoptr(GError) error = NULL; + Fixture f; + + fixture_init_server (&f); + + g_assert_true (goodix_tls_server_deinit (&f.server, &error)); + g_assert_no_error (error); +} + +static void +test_deinit_mid_handshake (void) +{ + g_autoptr(GError) error = NULL; + const guint8 start_of_hello[] = { 0x16, 0x03, 0x01, 0x10, 0x00, 0x01 }; + Fixture f; + + fixture_init_server (&f); + g_assert_cmpint (goodix_tls_client_send (&f.server, (guint8 *) start_of_hello, + sizeof (start_of_hello)), + ==, sizeof (start_of_hello)); + + g_assert_true (goodix_tls_server_deinit (&f.server, &error)); + g_assert_no_error (error); +} + +#define CONNECTED_TEST(path, func) \ + g_test_add (path, Fixture, NULL, fixture_setup_connected, func, fixture_teardown) + +int +main (int argc, char *argv[]) +{ + g_test_init (&argc, &argv, NULL); + + alarm (HANG_LIMIT_SECONDS); + + CONNECTED_TEST ("/goodixtls/receive/whole-record", test_whole_record); + CONNECTED_TEST ("/goodixtls/receive/incomplete-record", test_incomplete_record); + CONNECTED_TEST ("/goodixtls/receive/nothing-to-read", test_nothing_to_read); + CONNECTED_TEST ("/goodixtls/receive/end-of-file", test_end_of_file); + CONNECTED_TEST ("/goodixtls/receive/corrupt-record", test_corrupt_record); + CONNECTED_TEST ("/goodixtls/client-recv/timeout", test_client_recv_timeout); + g_test_add_func ("/goodixtls/deinit/during-handshake", test_deinit_during_handshake); + g_test_add_func ("/goodixtls/deinit/mid-handshake", test_deinit_mid_handshake); + + return g_test_run (); +}