From e3a1d205cb2c62bd6f7f01cb3b2ff4b10ac4fb88 Mon Sep 17 00:00:00 2001 From: Uma Mehta Date: Thu, 14 Sep 2017 16:53:45 +0530 Subject: mm-video-v4l2: venc: Avoid buffer access after free. client expects buffer to be free if free_buffer is called, but if omx is in executing state free buffer call will error out. When async thread tries to copy data to client buffer which is already freed,it leads to crash. Added a bitmask to avoid copy to buffer after free. Change-Id: Id439aac54ee64a65ea68b6431a9f5150255a6980 --- mm-video-v4l2/vidc/venc/inc/omx_video_base.h | 1 + mm-video-v4l2/vidc/venc/src/omx_video_base.cpp | 24 +++++++++++------------ mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp | 7 ++++++- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/mm-video-v4l2/vidc/venc/inc/omx_video_base.h index 5b2917f2..fae8e2a8 100644 --- a/mm-video-v4l2/vidc/venc/inc/omx_video_base.h +++ b/mm-video-v4l2/vidc/venc/inc/omx_video_base.h @@ -664,6 +664,7 @@ class omx_video: public qc_omx_component bool allocate_native_handle; uint64_t m_out_bm_count; + uint64_t m_client_out_bm_count; uint64_t m_inp_bm_count; uint64_t m_flags; uint64_t m_etb_count; diff --git a/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp index 477c2f55..7361181b 100644 --- a/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp +++ b/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp @@ -273,6 +273,7 @@ omx_video::omx_video(): pending_output_buffers(0), allocate_native_handle(false), m_out_bm_count(0), + m_client_out_bm_count(0), m_inp_bm_count(0), m_flags(0), m_etb_count(0), @@ -2469,7 +2470,6 @@ OMX_ERRORTYPE omx_video::use_output_buffer( return OMX_ErrorBadParameter; } - auto_lock l(m_buf_lock); if (!m_out_mem_ptr) { output_use_buffer = true; int nBufHdrSize = 0; @@ -2619,6 +2619,7 @@ OMX_ERRORTYPE omx_video::use_output_buffer( } BITMASK_SET(&m_out_bm_count,i); + BITMASK_SET(&m_client_out_bm_count,i); } else { DEBUG_PRINT_ERROR("ERROR: All o/p Buffers have been Used, invalid use_buf call for " "index = %u", i); @@ -2656,8 +2657,9 @@ OMX_ERRORTYPE omx_video::use_buffer( DEBUG_PRINT_ERROR("ERROR: Use Buffer in Invalid State"); return OMX_ErrorInvalidState; } + + auto_lock l(m_buf_lock); if (port == PORT_INDEX_IN) { - auto_lock l(m_lock); eRet = use_input_buffer(hComp,bufferHdr,port,appData,bytes,buffer); } else if (port == PORT_INDEX_OUT) { eRet = use_output_buffer(hComp,bufferHdr,port,appData,bytes,buffer); @@ -2665,7 +2667,6 @@ OMX_ERRORTYPE omx_video::use_buffer( DEBUG_PRINT_ERROR("ERROR: Invalid Port Index received %d",(int)port); eRet = OMX_ErrorBadPortIndex; } - if (eRet == OMX_ErrorNone) { if (allocate_done()) { if (BITMASK_PRESENT(&m_flags,OMX_COMPONENT_IDLE_PENDING)) { @@ -2728,7 +2729,6 @@ OMX_ERRORTYPE omx_video::free_input_buffer(OMX_BUFFERHEADERTYPE *bufferHdr) } if (index < m_sInPortDef.nBufferCountActual && m_pInput_pmem) { - auto_lock l(m_lock); if (m_pInput_pmem[index].fd > 0 && input_use_buffer == false) { DEBUG_PRINT_LOW("FreeBuffer:: i/p AllocateBuffer case"); @@ -3272,10 +3272,9 @@ OMX_ERRORTYPE omx_video::allocate_buffer(OMX_IN OMX_HANDLETYPE h DEBUG_PRINT_ERROR("ERROR: Allocate Buf in Invalid State"); return OMX_ErrorInvalidState; } - + auto_lock l(m_buf_lock); // What if the client calls again. if (port == PORT_INDEX_IN) { - auto_lock l(m_lock); #ifdef _ANDROID_ICS_ if (meta_mode_enable) eRet = allocate_input_meta_buffer(hComp,bufferHdr,appData,bytes); @@ -3344,7 +3343,12 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, unsigned int nPortIndex; DEBUG_PRINT_LOW("In for encoder free_buffer"); - + auto_lock l(m_buf_lock); + if (port == PORT_INDEX_OUT) { //client called freebuffer, clearing client buffer bitmask right away to avoid use after free + nPortIndex = buffer - (OMX_BUFFERHEADERTYPE*)m_out_mem_ptr; + if(BITMASK_PRESENT(&m_client_out_bm_count, nPortIndex)) + BITMASK_CLEAR(&m_client_out_bm_count,nPortIndex); + } if (m_state == OMX_StateIdle && (BITMASK_PRESENT(&m_flags ,OMX_COMPONENT_LOADING_PENDING))) { DEBUG_PRINT_LOW(" free buffer while Component in Loading pending"); @@ -3370,12 +3374,10 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, DEBUG_PRINT_LOW("free_buffer on i/p port - Port idx %u, actual cnt %u", nPortIndex, (unsigned int)m_sInPortDef.nBufferCountActual); - pthread_mutex_lock(&m_lock); if (nPortIndex < m_sInPortDef.nBufferCountActual && BITMASK_PRESENT(&m_inp_bm_count, nPortIndex)) { // Clear the bit associated with it. BITMASK_CLEAR(&m_inp_bm_count,nPortIndex); - pthread_mutex_unlock(&m_lock); free_input_buffer (buffer); m_sInPortDef.bPopulated = OMX_FALSE; @@ -3403,7 +3405,6 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, #endif } } else { - pthread_mutex_unlock(&m_lock); DEBUG_PRINT_ERROR("ERROR: free_buffer ,Port Index Invalid"); eRet = OMX_ErrorBadPortIndex; } @@ -3424,7 +3425,6 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, nPortIndex, (unsigned int)m_sOutPortDef.nBufferCountActual); if (nPortIndex < m_sOutPortDef.nBufferCountActual && BITMASK_PRESENT(&m_out_bm_count, nPortIndex)) { - auto_lock l(m_buf_lock); // Clear the bit associated with it. BITMASK_CLEAR(&m_out_bm_count,nPortIndex); m_sOutPortDef.bPopulated = OMX_FALSE; @@ -3683,7 +3683,7 @@ OMX_ERRORTYPE omx_video::empty_this_buffer_proxy(OMX_IN OMX_HANDLETYPE hComp, { DEBUG_PRINT_LOW("Heap UseBuffer case, so memcpy the data"); - auto_lock l(m_lock); + auto_lock l(m_buf_lock); pmem_data_buf = (OMX_U8 *)m_pInput_pmem[nBufIndex].buffer; if (pmem_data_buf && BITMASK_PRESENT(&m_inp_bm_count, nBufIndex)) { memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset), diff --git a/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp b/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp index b52572d8..f6f8f4d5 100644 --- a/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp +++ b/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp @@ -2095,11 +2095,15 @@ OMX_ERRORTYPE omx_venc::component_deinit(OMX_IN OMX_HANDLETYPE hComp) DEBUG_PRINT_ERROR("WARNING:Rxd DeInit,OMX not in LOADED state %d",\ m_state); } + + auto_lock l(m_buf_lock); if (m_out_mem_ptr) { DEBUG_PRINT_LOW("Freeing the Output Memory"); for (i=0; i< m_sOutPortDef.nBufferCountActual; i++ ) { if (BITMASK_PRESENT(&m_out_bm_count, i)) { BITMASK_CLEAR(&m_out_bm_count, i); + if (BITMASK_PRESENT(&m_client_out_bm_count, i)) + BITMASK_CLEAR(&m_client_out_bm_count, i); free_output_buffer (&m_out_mem_ptr[i]); } @@ -2421,7 +2425,8 @@ int omx_venc::async_message_process (void *context, void* message) omxhdr->nFlags = m_sVenc_msg->buf.flags; /*Use buffer case*/ - if (omx->output_use_buffer && !omx->m_use_output_pmem && !omx->is_secure_session()) { + if (BITMASK_PRESENT(&(omx->m_client_out_bm_count), bufIndex) && + omx->output_use_buffer && !omx->m_use_output_pmem && !omx->is_secure_session()) { DEBUG_PRINT_LOW("memcpy() for o/p Heap UseBuffer"); memcpy(omxhdr->pBuffer, (m_sVenc_msg->buf.ptrbuffer), -- cgit v1.2.3 From d1ec6540652a640adf313b41120bd1eae5f40dca Mon Sep 17 00:00:00 2001 From: Vikash Garodia Date: Tue, 19 Sep 2017 12:43:02 +0530 Subject: mm-video-v4l2: venc: Use client allocated memory if available. IL client may free the buffer and calls for free buffer on IL component to free the buffer header. It may happen that the IL component may reject the free buffer due to various reasons. In such scenario, client might have already freed the memory allocated by client (such scenario will appear in use buffer mode of buffer allocation). Now accessing client buffer in such scenario may lead to use after free vulnerability. Added a flag to indicate if the client buffer is available to perform any operation on the client allocated memory. If not, restrict from doing any operation on client memory. Change-Id: I45e4f117e98588ee7c888ec5c1cb2424bc7e5fa3 --- mm-video-v4l2/vidc/venc/inc/omx_video_base.h | 1 + mm-video-v4l2/vidc/venc/src/omx_video_base.cpp | 8 +++++++- mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp | 2 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/mm-video-v4l2/vidc/venc/inc/omx_video_base.h index fae8e2a8..93d05da1 100644 --- a/mm-video-v4l2/vidc/venc/inc/omx_video_base.h +++ b/mm-video-v4l2/vidc/venc/inc/omx_video_base.h @@ -665,6 +665,7 @@ class omx_video: public qc_omx_component uint64_t m_out_bm_count; uint64_t m_client_out_bm_count; + uint64_t m_client_in_bm_count; uint64_t m_inp_bm_count; uint64_t m_flags; uint64_t m_etb_count; diff --git a/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp index 7361181b..fefb7e47 100644 --- a/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp +++ b/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp @@ -274,6 +274,7 @@ omx_video::omx_video(): allocate_native_handle(false), m_out_bm_count(0), m_client_out_bm_count(0), + m_client_in_bm_count(0), m_inp_bm_count(0), m_flags(0), m_etb_count(0), @@ -2343,6 +2344,7 @@ OMX_ERRORTYPE omx_video::use_input_buffer( *bufferHdr = (m_inp_mem_ptr + i); BITMASK_SET(&m_inp_bm_count,i); + BITMASK_SET(&m_client_in_bm_count,i); (*bufferHdr)->pBuffer = (OMX_U8 *)buffer; (*bufferHdr)->nSize = sizeof(OMX_BUFFERHEADERTYPE); @@ -3348,6 +3350,10 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, nPortIndex = buffer - (OMX_BUFFERHEADERTYPE*)m_out_mem_ptr; if(BITMASK_PRESENT(&m_client_out_bm_count, nPortIndex)) BITMASK_CLEAR(&m_client_out_bm_count,nPortIndex); + } else if (port == PORT_INDEX_IN) { + nPortIndex = buffer - (meta_mode_enable?meta_buffer_hdr:m_inp_mem_ptr); + if(BITMASK_PRESENT(&m_client_in_bm_count, nPortIndex)) + BITMASK_CLEAR(&m_client_in_bm_count,nPortIndex); } if (m_state == OMX_StateIdle && (BITMASK_PRESENT(&m_flags ,OMX_COMPONENT_LOADING_PENDING))) { @@ -3685,7 +3691,7 @@ OMX_ERRORTYPE omx_video::empty_this_buffer_proxy(OMX_IN OMX_HANDLETYPE hComp, auto_lock l(m_buf_lock); pmem_data_buf = (OMX_U8 *)m_pInput_pmem[nBufIndex].buffer; - if (pmem_data_buf && BITMASK_PRESENT(&m_inp_bm_count, nBufIndex)) { + if (pmem_data_buf && BITMASK_PRESENT(&m_client_in_bm_count, nBufIndex)) { memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset), buffer->nFilledLen); } diff --git a/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp b/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp index f6f8f4d5..0bdf02b0 100644 --- a/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp +++ b/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp @@ -2125,6 +2125,8 @@ OMX_ERRORTYPE omx_venc::component_deinit(OMX_IN OMX_HANDLETYPE hComp) for (i=0; i Date: Thu, 28 Sep 2017 14:51:14 +0530 Subject: mm-video-v4l2: Avoid buffer access after free buffer call. Avoid accessing buffers after free buffer calls are made from the client Bug: 64750179 CRs-Fixed: 2115779 Change-Id: Ifde8d4e170b8dbeb9f7485d0222b05c3b2a960f3 --- mm-video-v4l2/vidc/venc/inc/omx_video_base.h | 1 + mm-video-v4l2/vidc/venc/src/omx_video_base.cpp | 21 ++++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/mm-video-v4l2/vidc/venc/inc/omx_video_base.h index 93d05da1..21c6a7b3 100644 --- a/mm-video-v4l2/vidc/venc/inc/omx_video_base.h +++ b/mm-video-v4l2/vidc/venc/inc/omx_video_base.h @@ -680,6 +680,7 @@ class omx_video: public qc_omx_component extra_data_handler extra_data_handle; bool hw_overload; + bool m_buffer_freed; }; #endif // __OMX_VIDEO_BASE_H__ diff --git a/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp index fefb7e47..2675dac7 100644 --- a/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp +++ b/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp @@ -280,7 +280,8 @@ omx_video::omx_video(): m_etb_count(0), m_fbd_count(0), m_event_port_settings_sent(false), - hw_overload(false) + hw_overload(false), + m_buffer_freed(0) { DEBUG_PRINT_HIGH("omx_video(): Inside Constructor()"); memset(&m_cmp,0,sizeof(m_cmp)); @@ -410,6 +411,9 @@ void omx_video::process_event_cb(void *ctxt, unsigned char id) case OMX_CommandStateSet: pThis->m_state = (OMX_STATETYPE) p2; DEBUG_PRINT_LOW("Process -> state set to %d", pThis->m_state); + if (pThis->m_state == OMX_StateLoaded) { + m_buffer_freed = false; + } pThis->m_pCallbacks.EventHandler(&pThis->m_cmp, pThis->m_app_data, OMX_EventCmdComplete, p1, p2, NULL); break; @@ -3363,12 +3367,14 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, DEBUG_PRINT_LOW("Free Buffer while port %u disabled", (unsigned int)port); } else if (m_state == OMX_StateExecuting || m_state == OMX_StatePause) { DEBUG_PRINT_ERROR("ERROR: Invalid state to free buffer,ports need to be disabled"); + m_buffer_freed = true; post_event(OMX_EventError, OMX_ErrorPortUnpopulated, OMX_COMPONENT_GENERATE_EVENT); return eRet; } else { DEBUG_PRINT_ERROR("ERROR: Invalid state to free buffer,port lost Buffers"); + m_buffer_freed = true; post_event(OMX_EventError, OMX_ErrorPortUnpopulated, OMX_COMPONENT_GENERATE_EVENT); @@ -3490,6 +3496,9 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, m_out_bm_count, m_inp_bm_count); } } + if (eRet != OMX_ErrorNone) { + m_buffer_freed = true; + } return eRet; } @@ -3806,9 +3815,15 @@ OMX_ERRORTYPE omx_video::fill_this_buffer_proxy( (void)hComp; OMX_U8 *pmem_data_buf = NULL; OMX_ERRORTYPE nRet = OMX_ErrorNone; + auto_lock l(m_buf_lock); + if (m_buffer_freed == true) { + DEBUG_PRINT_ERROR("ERROR: FTBProxy: Invalid call. Called after freebuffer"); + return OMX_ErrorBadParameter; + } - DEBUG_PRINT_LOW("FTBProxy: bufferAdd->pBuffer[%p]", bufferAdd->pBuffer); - + if (bufferAdd != NULL) { + DEBUG_PRINT_LOW("FTBProxy: bufferAdd->pBuffer[%p]", bufferAdd->pBuffer); + } if (bufferAdd == NULL || ((bufferAdd - m_out_mem_ptr) >= (int)m_sOutPortDef.nBufferCountActual) ) { DEBUG_PRINT_ERROR("ERROR: FTBProxy: Invalid i/p params"); return OMX_ErrorBadParameter; -- cgit v1.2.3