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

Commit 2c94b982 authored by Praneeth Paladugu's avatar Praneeth Paladugu Committed by Vikash Garodia
Browse files

msm: vidc: Fix a race condition in error handling 



Currently video driver handles session init and clean in one
thread and response handling from HW in another thread. Due to
race conditions response handler thread may access the session
which was already freed by forward thread. Hence synchronize the
forward thread and response handler with session_lock.

Ex : When driver detects SYS_ERROR, it informs client and client
tries to clean the session. In the meantime if HW sends a response,
the response handler thread tries to access the inst struct and this
struct might have freed as part of session close. This causes
use-after-free exception.

CRs-Fixed: 765521
Change-Id: I11519087e9c0dcb95587c6a048853e47032bd1be
Signed-off-by: default avatarPraneeth Paladugu <ppaladug@codeaurora.org>
parent 2fe7e721

drivers/media/platform/msm/vidc/hfi_response_handler.c

+7 −6
Original line number Diff line number Diff line
@@ -1374,7 +1374,8 @@ u32 hfi_process_msg_packet(msm_vidc_callback callback, u32 device_id, 
	session_pkt_func_def session_pkt_func = NULL;

	sys_pkt_func_def sys_pkt_func = NULL;



	if (!callback || !msg_hdr || msg_hdr->size < VIDC_IFACEQ_MIN_PKT_SIZE) {

	if (!callback || !session_lock || !msg_hdr ||

		msg_hdr->size < VIDC_IFACEQ_MIN_PKT_SIZE) {

		dprintk(VIDC_ERR, "%s: bad packet/packet size\n",

			__func__);

		rc = -EINVAL;
@@ -1461,19 +1462,17 @@ u32 hfi_process_msg_packet(msm_vidc_callback callback, u32 device_id, 
		dprintk(VIDC_DBG, "UNKNOWN_MSG_TYPE : %d\n", msg_hdr->packet);

		break;

	}



	if (session_pkt_func && session_lock) {

	mutex_lock(session_lock);

	if (session_pkt_func) {

		struct vidc_hal_session_cmd_pkt *pkt =

			(struct vidc_hal_session_cmd_pkt *)msg_hdr;

		mutex_lock(session_lock);

		session = hfi_process_get_session(sessions, pkt->session_id);

		mutex_unlock(session_lock);

		/* Event of type HFI_EVENT_SYS_ERROR will not have any session

		 * associated with it */

		if (!session && (msg_hdr->packet != HFI_MSG_EVENT_NOTIFY)) {

			dprintk(VIDC_ERR, "%s Got invalid session id: %d\n",

					__func__, pkt->session_id);

			return rc;

			goto invalid_session;

		}

	}
@@ -1482,5 +1481,7 @@ u32 hfi_process_msg_packet(msm_vidc_callback callback, u32 device_id, 
	if (sys_pkt_func)

		sys_pkt_func(callback, device_id, msg_hdr);



invalid_session:

	mutex_unlock(session_lock);

	return rc;

}

drivers/media/platform/msm/vidc/msm_vidc.c

+2 −1
Original line number Diff line number Diff line
@@ -1431,7 +1431,6 @@ int msm_vidc_close(void *instance) 
	mutex_unlock(&inst->registeredbufs.lock);



	core = inst->core;

	msm_comm_session_clean(inst);



	mutex_lock(&core->lock);

	list_for_each_entry_safe(temp, inst_dummy, &core->instances, list) {
@@ -1458,6 +1457,8 @@ int msm_vidc_close(void *instance) 
		dprintk(VIDC_ERR,

			"Failed to move video instance to uninit state\n");



	msm_comm_session_clean(inst);



	msm_smem_delete_client(inst->mem_client);



	pr_info(VIDC_DBG_TAG "Closed video instance: %p\n",

drivers/media/platform/msm/vidc/msm_vidc_common.c

+0 −1
Original line number Diff line number Diff line
@@ -1231,7 +1231,6 @@ static void handle_session_close(enum command_response cmd, void *data) 
			dprintk(VIDC_ERR, "%s invalid params\n", __func__);

			return;

		}

		msm_comm_session_clean(inst);

		signal_session_msg_receipt(cmd, inst);

		show_stats(inst);

	} else {

drivers/media/platform/msm/vidc/venus_hfi.c

+15 −10
Original line number Diff line number Diff line
@@ -130,12 +130,17 @@ static void venus_hfi_sim_modify_cmd_packet(u8 *packet, 


	fw_bias = device->hal_data->firmware_base;

	sys_init = (struct hfi_cmd_sys_session_init_packet *)packet;

	if (&device->session_lock) {

		mutex_lock(&device->session_lock);



	/* Ideally we should acquire device->session_lock. If we acquire

	 * we may go to deadlock with inst->*_lock between two threads.

	 * Ex : in the forward path we acquire inst->internalbufs.lock and

	 * session_lock and in the reverse path, we acquire session_lock and

	 * internalbufs.lock. So this may introduce deadlock. So we are not

	 * doing that. On virtio it is less likely to run two sessions

	 * concurrently. So it should be fine */



	session = hfi_process_get_session(

			&device->sess_head, sys_init->session_id);

		mutex_unlock(&device->session_lock);

	}

	if (!session) {

		dprintk(VIDC_DBG, "%s :Invalid session id : %x\n",

				__func__, sys_init->session_id);
@@ -2674,20 +2679,20 @@ static int venus_hfi_session_abort(void *session) 
static int venus_hfi_session_clean(void *session)

{

	struct hal_session *sess_close;

	struct venus_hfi_device *device;

	if (!session) {

		dprintk(VIDC_ERR, "Invalid Params %s\n", __func__);

		return -EINVAL;

	}

	sess_close = session;

	device = sess_close->device;

	venus_hfi_flush_debug_queue(sess_close->device, NULL);

	dprintk(VIDC_DBG, "deleted the session: 0x%p\n",

			sess_close);

	mutex_lock(&((struct venus_hfi_device *)

			sess_close->device)->session_lock);

	mutex_lock(&device->session_lock);

	list_del(&sess_close->list);

	mutex_unlock(&((struct venus_hfi_device *)

			sess_close->device)->session_lock);

	kfree(sess_close);

	mutex_unlock(&device->session_lock);

	return 0;

}