Add goodixtls5e0a driver for Goodix 27c6:5e0a - #42
Draft
jitendradara12 wants to merge 17 commits into
Draft
Conversation
Image-type driver for the Goodix 27c6:5e0a TLS sensor (e.g. Realme Book): bulk USB transport, TLS 1.2 PSK session, hardware finger-detect gating, 64x80 native frames decoded to 128x160 inverted images for the in-tree NBIS/Bozorth3 matcher, plus suspend/resume and clean teardown. Derivation: clean-room reverse engineering from passive USB captures of Windows driver traffic. No vendor code included. PSK note: the driver carries a static 32-byte host PSK observed in the captures (flags 0xbb020001). The 0xe4-readable slot reports factory bytes (not the TLS key) and 0xe0 writes are rejected, so there is no on-device provisioning. Per-unit scope of this key is unconfirmed. Build: driver 'goodixtls5e0a' with 'goodixtls,openssl' helpers, registered in default drivers; udev hwdb regenerated (5E0A moves supported).
There was a problem hiding this comment.
🟡 Changes recommended
It introduces confirmed correctness issues in device open/close handling and error/IO paths that can cause silent failures, hangs, or out-of-bounds access.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new in-tree image driver (goodixtls5e0a) for the Goodix 27c6:5e0a TLS-based USB fingerprint sensor, including the shared Goodix TLS transport/protocol layer and build + hwdb wiring so the device is treated as supported by libfprint.
Changes:
- Introduce Goodix TLS transport/protocol + 5xx base implementation and the 27c6:5e0a device driver.
- Wire the new driver into Meson driver selection and add OpenSSL as a helper dependency for it.
- Move 27c6:5e0a from unsupported/whitelist listings into supported autosuspend hwdb entries.
File summaries
| File | Description |
|---|---|
| README.md | Notes 27c6:5e0a as in-progress work. |
| meson.build | Adds goodixtls5e0a to default drivers; adds OpenSSL helper dependency handling. |
| libfprint/meson.build | Registers driver sources for goodixtls5e0a and helper sources for Goodix TLS layer. |
| libfprint/fprint-list-udev-hwdb.c | Removes 27c6:5e0a from the unsupported whitelist table. |
| libfprint/drivers/goodixtls/goodixtls.h | Declares TLS server wrapper used to terminate PSK TLS from the device. |
| libfprint/drivers/goodixtls/goodixtls.c | Implements the OpenSSL-backed TLS hop (server-side decrypt). |
| libfprint/drivers/goodixtls/goodix5xx.h | Declares the common “5xx” base API used by Goodix TLS devices. |
| libfprint/drivers/goodixtls/goodix5xx.c | Implements shared scan/config/calibration helpers + image capture plumbing. |
| libfprint/drivers/goodixtls/goodix5e0a.h | Adds 5e0a constants, static PSK/config blobs, and ID table. |
| libfprint/drivers/goodixtls/goodix5e0a.c | Implements activation + scan state machines and 64x80→128x160 image processing for 5e0a. |
| libfprint/drivers/goodixtls/goodix.h | Declares Goodix protocol/TLS/dev APIs and callbacks. |
| libfprint/drivers/goodixtls/goodix.c | Implements USB bulk transport, protocol encoding/decoding, TLS handshake proxying, and image decrypt flow. |
| libfprint/drivers/goodixtls/goodix_proto.h | Defines protocol/packet structures and command IDs. |
| libfprint/drivers/goodixtls/goodix_proto.c | Implements protocol/packet encoding/decoding and checksums. |
| data/autosuspend.hwdb | Adds 27c6:5e0a to supported autosuspend entries and removes it from the unsupported block. |
Review details
Suppressed comments (1)
libfprint/drivers/goodixtls/goodix5xx.c:548
- dev_init() treats a successful goodix_dev_init() as an error path and reports completion early; on failure it incorrectly reports success. This will mask USB claim failures and break subsequent I/O.
if (goodix_dev_init (dev, &error))
{
fpi_image_device_open_complete (img_dev, error);
return;
}
- Files reviewed: 15/15 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The proxy SSM completing never proved the openssl server accepted: on accept failure (e.g. peer Finished bad-record-mac when the device negotiates with an unexpected key) the serve thread logged and exited while activation proceeded on a dead session, hanging later at FDT with a command timeout. Record the accept outcome in GoodixTlsServer and gate tls_handshake_done on it, failing activation with a clear TLS error through the normal ready-callback path.
- goodix_send_data: bound chunk length on final partial USB packet to prevent out-of-bounds reads - goodix_tls_init: handle goodix_tls_server_init failure by reporting error to callback and releasing resources cleanly - dev_init/dev_deinit: fix inverted boolean check on goodix_dev_init and goodix_dev_deinit to properly propagate errors - goodix_dev_deinit: avoid double-free of priv->data and unref priv->transfer_cancel_tkn - goodix_tls_server_read: check for NULL error pointer before assigning - goodix_tls_server_init: use g_set_error and validate pthread_create - goodixtls: track serve_thread with boolean flag instead of scalar cast - goodixtls: clean up headers, add missing includes (stdlib.h, unistd.h, openssl/ssl.h, pthread.h) and remove unused ones
There was a problem hiding this comment.
🟡 Changes recommended
The new Goodix TLS transport code has confirmed crash/leak risks (NULL cancellable deref, collision user_data leak) and record-framing issues (short read/write on stream sockets) that can break TLS/proxy operation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
libfprint/drivers/goodixtls/goodix.c:621
- On command collision, goodix_send_protocol() frees the payload but leaks the user_data trampoline (GoodixCallbackInfo) allocated by callers like send_cmd_reply()/send_cmd_noreply(). This can accumulate if the device/chatty firmware causes overlapping commands.
libfprint/drivers/goodixtls/goodix5xx.h:34 - Spelling/grammar: "For an example out goodix511.c." should be corrected (e.g. "For an example see goodix511.c.").
libfprint/drivers/goodixtls/goodixtls.h:63 - Spelling: "Initalise" should be "Initialise" in the API doc comment.
libfprint/drivers/goodixtls/goodix.c:500
- goodix_start_read_loop() calls g_cancellable_is_cancelled(priv->transfer_cancel_tkn) without a NULL guard; if the read loop is started after a partial init/teardown this can crash.
if (g_cancellable_is_cancelled (priv->transfer_cancel_tkn))
g_cancellable_reset (priv->transfer_cancel_tkn);
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
- goodix_receive_data_cb / goodix_start_read_loop: guard against NULL transfer_cancel_tkn during device deinit/re-init - goodix_tls_client_write: loop to handle short writes and retry on EINTR - goodix_tls_client_read: guard descriptors and retry on EINTR while preserving stream socket read semantics for multi-record TLS flights
There was a problem hiding this comment.
🟡 Changes recommended
The new scan polling path can hang indefinitely due to disabled command timeouts, and there is at least one unchecked allocation that can crash.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
libfprint/drivers/goodixtls/goodix5e0a.c:422
- calloc() is not checked for failure; if allocation fails, goodix5e0a_decode_frame() and the subsequent loop dereference raw_frame and will crash.
libfprint/drivers/goodixtls/goodix.c:673 - goodix5e0a_capture_payload is a file-local constant but is currently a global with external linkage, which can trigger -Wmissing-declarations noise and makes it easier to accidentally use/override from other compilation units.
libfprint/drivers/goodixtls/goodix5e0a.c:539
- Same issue as the timeout callback path: SCAN_5E0A_FDT_DOWN uses timeout_ms=0, so a missing reply can block the scan SSM indefinitely.
case SCAN_5E0A_FDT_DOWN:
send_cmd_reply (dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_DOWN,
goodix_5e0a_down_s12, sizeof (goodix_5e0a_down_s12),
0, goodix5e0a_on_fdt_down_reply, ssm);
break;
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
Passing timeout 0 disabled the USB command timeout in goodix_send_protocol. If the MCU dropped a packet, crashed, or was unplugged, the scan state machine hung indefinitely. Use GOODIX_TIMEOUT (1000ms) as a watchdog on FDT_DOWN command replies without altering the 50ms polling cadence.
…and conditional reset
Consolidate callback allocation behind make_cb_info(), mark file-internal helpers static, and remove unused command wrappers exposed by the static hardening (reset, psk_write, pov, drv_state, idle_mode, sensor-register and powerdown helpers, plus the unwired FDT-DOWN retry table). Make MCU_GET_IMAGE capture payload polymorphic via base-class capture_payload fields instead of a driver-string check, move wire tables and PSK from goodix5e0a.h to file-static, migrate g_memdup to g_memdup2, use g_malloc consistently, hoist declarations, type SSM helpers as FpiSsm, and narrow USB re-enumeration detection to bus+port+vid+pid. Also propagate TLS proxy write errors and guard the TLS server read.
Frame decoder (goodix_proto.c/h, goodix.c): parse headers with memcpy instead of unaligned struct casts, guard against NULL out-params and the zero-length underflow, use byte ops instead of bitfields for ACK flags, bound the debug helper, and log (but do not enforce) checksum mismatches, which the hardware does not produce reliably. TLS handshake relay (goodix.c, goodixtls.c/h): read whole back-to-back TLS records until the socket goes idle instead of a single short read that truncated the server flight and desynchronised the relay. Drop the now-unused single-read helper, map WANT_READ/WRITE to G_IO_ERROR_WOULD_BLOCK, and handle an empty OpenSSL error queue. Logging: use fp_dbg/fp_warn consistently instead of g_message and g_warning in library code, drop per-frame hex dumps and wire-layout spam, and keep comments factual.
Give finger-detect awaits their own 2000ms timeout and retry a timed out await once before failing the scan; a late reply simply arrives during the second await. Fast replies behave exactly as before.
Soften two comments to stated facts, keep the error text in the retry debug line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this adds driver support for Goodix 27c6:5e0a fingerprint sensors found on devices like the Realme Book.
What is in this PR
libfprint/drivers/goodixtls/goodix5e0a.[ch].Testing
Tested on hardware (Realme Book, USB ID 27c6:5e0a):
fprintd-enrollandfprintd-verify.ninja test).btw #34 is already open and it was too late when I found it so leaving this as a draft until I'm confident enough.