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

Commit 353955dd authored by Dinesh K Garg's avatar Dinesh K Garg
Browse files

mink: Race condition for local object processing 



There is a race condition in local object processing. When inbound req
is handed over to server thread, an issue could arise if invoke client
is killed just before server pick up request for marshalling. cb_txn
is refcounted but not local objs inside cb_txn. So it causes issue in
marshaling as killing of invoke client releases local obj.

Change-Id: I6ee101f210ea57358b329aeca972f4c6976b6255
Signed-off-by: default avatarDinesh K Garg <dineshg@codeaurora.org>
parent c511abcf

drivers/soc/qcom/smcinvoke.c

+55 −36
Original line number Diff line number Diff line
@@ -398,10 +398,10 @@ static void free_pending_cbobj_locked(struct kref *kref) 


static int get_pending_cbobj_locked(uint16_t srvr_id, int16_t obj_id)

{

	struct smcinvoke_server_info *server = find_cb_server_locked(srvr_id);

	struct list_head *head = NULL;

	struct smcinvoke_cbobj *cbobj = NULL;

	struct smcinvoke_cbobj *obj = NULL;

	struct smcinvoke_server_info *server = find_cb_server_locked(srvr_id);



	if (!server)

		return OBJECT_ERROR_BADOBJ;
@@ -469,7 +469,11 @@ static void delete_cb_txn(struct kref *kref) 
	struct smcinvoke_cb_txn *cb_txn = container_of(kref,

					struct smcinvoke_cb_txn, ref_cnt);



	if (OBJECT_OP_METHODID(cb_txn->cb_req->hdr.op) == OBJECT_OP_RELEASE)

		release_tzhandle_locked(cb_txn->cb_req->hdr.tzhandle);



	kfree(cb_txn->cb_req);

	hash_del(&cb_txn->hash);

	kfree(cb_txn);

}
@@ -846,19 +850,13 @@ static void process_tzcb_req(void *buf, size_t buf_len, struct file **arr_filp) 
	/* ret is going to TZ. Provide values from OBJECT_ERROR_<> */

	int ret = OBJECT_ERROR_DEFUNCT;

	struct smcinvoke_cb_txn *cb_txn = NULL;

	struct smcinvoke_tzcb_req *cb_req = NULL;

	struct smcinvoke_tzcb_req *cb_req = NULL, *tmp_cb_req = NULL;

	struct smcinvoke_server_info *srvr_info = NULL;



	if (buf_len < sizeof(struct smcinvoke_tzcb_req))

		return;



	cb_req = kmemdup(buf, buf_len, GFP_KERNEL);

	if (!cb_req) {

		/* we need to return error to caller so fill up result */

	cb_req = buf;

		cb_req->result = OBJECT_ERROR_KMEM;

		return;

	}



	/* check whether it is to be served by kernel or userspace */

	if (TZHANDLE_IS_KERNEL_OBJ(cb_req->hdr.tzhandle)) {
@@ -870,11 +868,26 @@ static void process_tzcb_req(void *buf, size_t buf_len, struct file **arr_filp) 
		return;

	}



	/*

	 * We need a copy of req that could be sent to server. Otherwise, if

	 * someone kills invoke caller, buf would go away and server would be

	 * working on already freed buffer, causing a device crash.

	 */

	tmp_cb_req = kmemdup(buf, buf_len, GFP_KERNEL);

	if (!tmp_cb_req) {

		/* we need to return error to caller so fill up result */

		cb_req->result = OBJECT_ERROR_KMEM;

		return;

	}



	cb_txn = kzalloc(sizeof(*cb_txn), GFP_KERNEL);

	if (!cb_txn) {

		ret = OBJECT_ERROR_KMEM;

		goto out;

		cb_req->result = OBJECT_ERROR_KMEM;

		kfree(tmp_cb_req);

		return;

	}

	/* no need for memcpy as we did kmemdup() above */

	cb_req  = tmp_cb_req;



	cb_txn->state = SMCINVOKE_REQ_PLACED;

	cb_txn->cb_req = cb_req;
@@ -886,6 +899,7 @@ static void process_tzcb_req(void *buf, size_t buf_len, struct file **arr_filp) 
	srvr_info = find_cb_server_locked(

				TZHANDLE_GET_SERVER(cb_req->hdr.tzhandle));

	if (!srvr_info || srvr_info->state == SMCINVOKE_SERVER_STATE_DEFUNCT) {

		/* ret equals Object_ERROR_DEFUNCT, at this point go to out */

		mutex_unlock(&g_smcinvoke_lock);

		goto out;

	}
@@ -893,33 +907,36 @@ static void process_tzcb_req(void *buf, size_t buf_len, struct file **arr_filp) 
	cb_txn->txn_id = ++srvr_info->txn_id;

	hash_add(srvr_info->reqs_table, &cb_txn->hash, cb_txn->txn_id);

	mutex_unlock(&g_smcinvoke_lock);

	/*

	 * we need not worry that server_info will be deleted because as long

	 * as this CBObj is served by this server, srvr_info will be valid.

	 */

	wake_up_interruptible(&srvr_info->req_wait_q);

	ret = wait_event_interruptible(srvr_info->rsp_wait_q,

			(cb_txn->state == SMCINVOKE_REQ_PROCESSED) ||

			(srvr_info->state == SMCINVOKE_SERVER_STATE_DEFUNCT));

	if (ret)

		pr_err("%s wait_event interrupted: ret = %d\n", __func__, ret);

out:

	/*

	 * If we are here, either req is processed or not

	 * if processed, result would have been set by txn processor

	 * if not processed, we should set result with ret which should have

	 * correct value that TZ/TA can understand

	 * we could be here because of either: a. Req is PROCESSED

	 * b. Server was killed                c. Invoke thread is killed

	 * sometime invoke thread and server are part of same process.

	 */

	mutex_lock(&g_smcinvoke_lock);

	if (!cb_txn || (cb_txn->state != SMCINVOKE_REQ_PROCESSED)) {

		cb_req->result = ret;

		if (srvr_info &&

		    srvr_info->state == SMCINVOKE_SERVER_STATE_DEFUNCT &&

		    OBJECT_OP_METHODID(cb_req->hdr.op) == OBJECT_OP_RELEASE) {

			release_tzhandle_locked(cb_req->hdr.tzhandle);

		}

	}

	if (cb_txn) {

	hash_del(&cb_txn->hash);

	if (cb_txn->state == SMCINVOKE_REQ_PROCESSED) {

		/*

		 * it is possible that server was killed immediately

		 * after CB Req was processed but who cares now!

		 */

	} else if (!srvr_info ||

		srvr_info->state == SMCINVOKE_SERVER_STATE_DEFUNCT) {

		cb_req->result = OBJECT_ERROR_DEFUNCT;

	} else {

		pr_debug("%s wait_event interrupted ret = %d\n", __func__, ret);

		cb_req->result = OBJECT_ERROR_ABORT;

	}

	memcpy(buf, cb_req, buf_len);

	kref_put(&cb_txn->ref_cnt, delete_cb_txn);

	}

	mutex_unlock(&g_smcinvoke_lock);

}
@@ -1447,24 +1464,26 @@ static long process_accept_req(struct file *filp, unsigned int cmd, 
		cb_txn = find_cbtxn_locked(server_info, user_args.txn_id,

					SMCINVOKE_REQ_PROCESSING);

		mutex_unlock(&g_smcinvoke_lock);

		/* cb_txn can be null if userspace provides wrong txn id. */

		/*

		 * cb_txn can be null if userspace provides wrong txn id OR

		 * invoke thread died while server was processing cb req.

		 * if invoke thread dies, it would remove req from Q. So

		 * no matching cb_txn would be on Q and hence NULL cb_txn.

		 */

		if (!cb_txn) {

			pr_err("%s: Invalid txn received  = %d\n",

			pr_err("%s txn %d either invalid or removed from Q\n",

					__func__, user_args.txn_id);

			goto out;

		}

		ret = marshal_out_tzcb_req(&user_args, cb_txn,

						cb_txn->filp_to_release);

		/*

		 * if client did not set error and we get error locally

		 * if client did not set error and we get error locally,

		 * we return local error to TA

		 */

		if (ret && cb_txn->cb_req->result == 0)

			cb_txn->cb_req->result = OBJECT_ERROR_UNAVAIL;



		if (OBJECT_OP_METHODID(user_args.op) == OBJECT_OP_RELEASE)

			release_tzhandles(&cb_txn->cb_req->hdr.tzhandle, 1);



		cb_txn->state = SMCINVOKE_REQ_PROCESSED;

		kref_put(&cb_txn->ref_cnt, delete_cb_txn);

		wake_up(&server_info->rsp_wait_q);
@@ -1483,7 +1502,7 @@ static long process_accept_req(struct file *filp, unsigned int cmd, 
		ret = wait_event_interruptible(server_info->req_wait_q,

				!hash_empty(server_info->reqs_table));

		if (ret) {

			pr_err("%s wait_event interrupted: ret = %d\n",

			pr_debug("%s wait_event interrupted: ret = %d\n",

							__func__, ret);

			goto out;

		}