Commit 0ace5f64 authored by Bastien Nocera's avatar Bastien Nocera

elan: Fix use-after-free if USB transfer is cancelled

parent e532524c
Pipeline #4935 passed with stage
in 3 minutes and 7 seconds
......@@ -324,11 +324,18 @@ static void elan_cmd_cb(struct libusb_transfer *transfer,
fpi_ssm *ssm,
void *user_data)
{
struct fp_img_dev *dev = FP_IMG_DEV(_dev);
struct elan_dev *elandev = FP_INSTANCE_DATA(_dev);
struct fp_img_dev *dev;
struct elan_dev *elandev;
G_DEBUG_HERE();
if (transfer->status == LIBUSB_TRANSFER_CANCELLED) {
fp_dbg("transfer cancelled");
return;
}
dev = FP_IMG_DEV(_dev);
elandev = FP_INSTANCE_DATA(_dev);
elandev->cur_transfer = NULL;
switch (transfer->status) {
......@@ -349,11 +356,6 @@ static void elan_cmd_cb(struct libusb_transfer *transfer,
elan_cmd_read(ssm, dev);
}
break;
case LIBUSB_TRANSFER_CANCELLED:
fp_dbg("transfer cancelled");
fpi_ssm_mark_failed(ssm, -ECANCELED);
elan_deactivate(dev);
break;
case LIBUSB_TRANSFER_TIMED_OUT:
fp_dbg("transfer timed out");
fpi_ssm_mark_failed(ssm, -ETIMEDOUT);
......
  • This broke deactivation. It used to go like

    fpi_ssm_mark_failed(ssm, -ECANCELED) -> fpi_ssm_free(ssm) -> <return>
    elan_deactivate(dev) -> fpi_imgdev_deactivate_complete(dev) -> <free elandev etc.> -> fpi_drvcb_close_complete()

    Now transfer cancellation leaves the device in unresponsive state after elan_cmd_cb() returns:

    $ fprintd-enroll 
    Using device /net/reactivated/Fprint/Device/0
    Enrolling right-index-finger finger.
    ^C
    $ fprintd-enroll 
    Using device /net/reactivated/Fprint/Device/0
    failed to claim device: Device was already claimed

    It must be power-cycled before it can be used again. What exactly was used after free? Maybe I could add some checks to work around it because deactivation needs to finish properly.

    Edited by Igor Filatov
  • I think the idea here is that the transfer callback may be called after the device is closed already. And in that case the pointers are invalid.

    It is a convention in GLib/GObject style async handling to always first check for cancellation and then only access any data if the operation was not cancelled. This is because in the usual case the reason for the cancellation is or at least may be that the object owning the operation is being destroyed.

    In libfprint what usually seems to happen though is that the deactivate call cancels a USB transfer. And then deactivation succeeds once the state machine finishes (in this case by marking it as failed). So my guess–without having looked more closely–is that the old code was correct.

Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment