Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5821575f authored by Jakub Pawlowski's avatar Jakub Pawlowski
Browse files

Simplify read/write start 

When read or write is started with insuficcient encryption, and the
encryption fails, it will either return immediate error
GATT_NO_RESOURCES, or the operation will be enqueued. In case the
enqueued operation fails again on the BTM_SetEncryption, no callback
will never be called, and no operation error will be returned. The
gatt_end_operation will never get called for the CLCB, and it will never
be freed, causing memory leak.

This patch fixes this by making sure that the call to
gatt_security_check_start always consumes the CLCB - either by executing
operation, queuing it, or returning error. This also ensures that if
BTM_SetEncryption fails, it will always be handled the same way - by
calling the callback.

Test: manual
Change-Id: Ibc20c1101cc7b5b6043e75df93b23a5b24b791e9
parent 6253e1c6

system/stack/gatt/gatt_api.cc

+2 −9
Original line number Diff line number Diff line
@@ -903,10 +903,7 @@ tGATT_STATUS GATTC_Read(uint16_t conn_id, tGATT_READ_TYPE type, 
        break;

    }

    /* start security check */

    if (gatt_security_check_start(p_clcb) == false) {

      status = GATT_NO_RESOURCES;

      gatt_clcb_dealloc(p_clcb);

    }

    gatt_security_check_start(p_clcb);

  } else {

    status = GATT_NO_RESOURCES;

  }
@@ -965,11 +962,7 @@ tGATT_STATUS GATTC_Write(uint16_t conn_id, tGATT_WRITE_TYPE type, 
      p->offset = 0;

    }



    if (gatt_security_check_start(p_clcb) == false) {

      status = GATT_NO_RESOURCES;

    }



    if (status == GATT_NO_RESOURCES) gatt_clcb_dealloc(p_clcb);

    gatt_security_check_start(p_clcb);

  } else {

    status = GATT_NO_RESOURCES;

  }

system/stack/gatt/gatt_auth.cc

+16 −32
Original line number Diff line number Diff line
@@ -390,51 +390,42 @@ static bool gatt_convert_sec_action(tGATT_SEC_ACTION gatt_sec_act, 


  return status;

}

/*******************************************************************************

 *

 * Function         gatt_check_enc_req

 *

 * Description      check link security.

 *

 * Returns          true if encrypted, otherwise false.

 *

 ******************************************************************************/

bool gatt_security_check_start(tGATT_CLCB* p_clcb) {



/** check link security */

void gatt_security_check_start(tGATT_CLCB* p_clcb) {

  tGATT_TCB* p_tcb = p_clcb->p_tcb;

  tGATT_SEC_ACTION gatt_sec_act;

  tBTM_BLE_SEC_ACT btm_ble_sec_act;

  bool status = true;

  tBTM_STATUS btm_status;

  tGATT_SEC_ACTION sec_act_old = gatt_get_sec_act(p_tcb);



  gatt_sec_act = gatt_determine_sec_act(p_clcb);

  tGATT_SEC_ACTION gatt_sec_act = gatt_determine_sec_act(p_clcb);



  if (sec_act_old == GATT_SEC_NONE) gatt_set_sec_act(p_tcb, gatt_sec_act);



  switch (gatt_sec_act) {

    case GATT_SEC_SIGN_DATA:

      GATT_TRACE_DEBUG("gatt_security_check_start: Do data signing");

      GATT_TRACE_DEBUG("%s: Do data signing", __func__);

      gatt_sign_data(p_clcb);

      break;

    case GATT_SEC_ENCRYPT:

    case GATT_SEC_ENCRYPT_NO_MITM:

    case GATT_SEC_ENCRYPT_MITM:

      if (sec_act_old < GATT_SEC_ENCRYPT) {

        GATT_TRACE_DEBUG(

            "gatt_security_check_start: Encrypt now or key upgreade first");

        GATT_TRACE_DEBUG("%s: Encrypt now or key upgreade first", __func__);

        tBTM_BLE_SEC_ACT btm_ble_sec_act;

        gatt_convert_sec_action(gatt_sec_act, &btm_ble_sec_act);

        btm_status =

        tBTM_STATUS btm_status =

            BTM_SetEncryption(p_tcb->peer_bda, p_tcb->transport,

                              gatt_enc_cmpl_cback, NULL, btm_ble_sec_act);

        if ((btm_status != BTM_SUCCESS) && (btm_status != BTM_CMD_STARTED)) {

          GATT_TRACE_ERROR(

              "gatt_security_check_start BTM_SetEncryption failed "

              "btm_status=%d",

              btm_status);

          status = false;

          GATT_TRACE_ERROR("%s BTM_SetEncryption failed btm_status=%d",

                           __func__, btm_status);

          gatt_set_sec_act(p_tcb, GATT_SEC_NONE);

          gatt_set_ch_state(p_tcb, GATT_CH_OPEN);



          gatt_end_operation(p_clcb, GATT_INSUF_ENCRYPTION, NULL);

          return;

        }

      }

      if (status) p_tcb->pending_enc_clcb.push(p_clcb);

      p_tcb->pending_enc_clcb.push(p_clcb);

      break;

    case GATT_SEC_ENC_PENDING:

      p_tcb->pending_enc_clcb.push(p_clcb);
@@ -444,11 +435,4 @@ bool gatt_security_check_start(tGATT_CLCB* p_clcb) { 
      gatt_sec_check_complete(true, p_clcb, gatt_sec_act);

      break;

  }



  if (status == false) {

    gatt_set_sec_act(p_tcb, GATT_SEC_NONE);

    gatt_set_ch_state(p_tcb, GATT_CH_OPEN);

  }



  return status;

}

system/stack/gatt/gatt_int.h

+1 −2
Original line number Diff line number Diff line
@@ -549,9 +549,8 @@ extern void gatt_send_queue_write_cancel(tGATT_TCB& tcb, tGATT_CLCB* p_clcb, 
                                         tGATT_EXEC_FLAG flag);



/* gatt_auth.cc */

extern bool gatt_security_check_start(tGATT_CLCB* p_clcb);

extern void gatt_security_check_start(tGATT_CLCB* p_clcb);

extern void gatt_verify_signature(tGATT_TCB& tcb, BT_HDR* p_buf);

extern tGATT_SEC_ACTION gatt_determine_sec_act(tGATT_CLCB* p_clcb);

extern tGATT_STATUS gatt_get_link_encrypt_status(tGATT_TCB& tcb);

extern tGATT_SEC_ACTION gatt_get_sec_act(tGATT_TCB* p_tcb);

extern void gatt_set_sec_act(tGATT_TCB* p_tcb, tGATT_SEC_ACTION sec_act);