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.

Bug: 36130225

CRs-Fixed: 2106434
Test: build & boot
Test: cts-tradefed run cts-dev --module CtsMediaTestCases --compatibility:module-arg
CtsMediaTestCases:include-annotation:android.platform.test.annotations.RequiresDevice

Change-Id: I4eb44837f9b3f8f1b8b2b4c879d8c3dd470bb52f
Author:    Uma Mehta <umamehta@codeaurora.org>
This commit is contained in:
Santhosh Behara 2017-09-14 16:53:45 +05:30 committed by Dongwon Kang
parent 9f1dee37d4
commit d53750a9db
3 changed files with 19 additions and 8 deletions

View file

@ -702,6 +702,7 @@ class omx_video: public qc_omx_component
bool allocate_native_handle; bool allocate_native_handle;
uint64_t m_out_bm_count; uint64_t m_out_bm_count;
uint64_t m_client_out_bm_count;
uint64_t m_inp_bm_count; uint64_t m_inp_bm_count;
uint64_t m_flags; uint64_t m_flags;
uint64_t m_etb_count; uint64_t m_etb_count;

View file

@ -289,6 +289,7 @@ omx_video::omx_video():
pending_output_buffers(0), pending_output_buffers(0),
allocate_native_handle(false), allocate_native_handle(false),
m_out_bm_count(0), m_out_bm_count(0),
m_client_out_bm_count(0),
m_inp_bm_count(0), m_inp_bm_count(0),
m_flags(0), m_flags(0),
m_etb_count(0), m_etb_count(0),
@ -2751,7 +2752,6 @@ OMX_ERRORTYPE omx_video::use_output_buffer(
return OMX_ErrorBadParameter; return OMX_ErrorBadParameter;
} }
auto_lock l(m_buf_lock);
if (!m_out_mem_ptr) { if (!m_out_mem_ptr) {
output_use_buffer = true; output_use_buffer = true;
int nBufHdrSize = 0; int nBufHdrSize = 0;
@ -2902,6 +2902,7 @@ OMX_ERRORTYPE omx_video::use_output_buffer(
} }
BITMASK_SET(&m_out_bm_count,i); BITMASK_SET(&m_out_bm_count,i);
BITMASK_SET(&m_client_out_bm_count,i);
} else { } else {
DEBUG_PRINT_ERROR("ERROR: All o/p Buffers have been Used, invalid use_buf call for " DEBUG_PRINT_ERROR("ERROR: All o/p Buffers have been Used, invalid use_buf call for "
"index = %u", i); "index = %u", i);
@ -2939,6 +2940,8 @@ OMX_ERRORTYPE omx_video::use_buffer(
DEBUG_PRINT_ERROR("ERROR: Use Buffer in Invalid State"); DEBUG_PRINT_ERROR("ERROR: Use Buffer in Invalid State");
return OMX_ErrorInvalidState; return OMX_ErrorInvalidState;
} }
auto_lock l(m_buf_lock);
if (port == PORT_INDEX_IN) { if (port == PORT_INDEX_IN) {
auto_lock l(m_lock); auto_lock l(m_lock);
eRet = use_input_buffer(hComp,bufferHdr,port,appData,bytes,buffer); eRet = use_input_buffer(hComp,bufferHdr,port,appData,bytes,buffer);
@ -2948,7 +2951,6 @@ OMX_ERRORTYPE omx_video::use_buffer(
DEBUG_PRINT_ERROR("ERROR: Invalid Port Index received %d",(int)port); DEBUG_PRINT_ERROR("ERROR: Invalid Port Index received %d",(int)port);
eRet = OMX_ErrorBadPortIndex; eRet = OMX_ErrorBadPortIndex;
} }
if (eRet == OMX_ErrorNone) { if (eRet == OMX_ErrorNone) {
if (allocate_done()) { if (allocate_done()) {
if (BITMASK_PRESENT(&m_flags,OMX_COMPONENT_IDLE_PENDING)) { if (BITMASK_PRESENT(&m_flags,OMX_COMPONENT_IDLE_PENDING)) {
@ -3011,7 +3013,6 @@ OMX_ERRORTYPE omx_video::free_input_buffer(OMX_BUFFERHEADERTYPE *bufferHdr)
} }
if (index < m_sInPortDef.nBufferCountActual && m_pInput_pmem) { if (index < m_sInPortDef.nBufferCountActual && m_pInput_pmem) {
auto_lock l(m_lock);
if (mUseProxyColorFormat) { if (mUseProxyColorFormat) {
if (m_opq_pmem_q.m_size) { if (m_opq_pmem_q.m_size) {
@ -3565,7 +3566,7 @@ OMX_ERRORTYPE omx_video::allocate_buffer(OMX_IN OMX_HANDLETYPE h
DEBUG_PRINT_ERROR("ERROR: Allocate Buf in Invalid State"); DEBUG_PRINT_ERROR("ERROR: Allocate Buf in Invalid State");
return OMX_ErrorInvalidState; return OMX_ErrorInvalidState;
} }
auto_lock l(m_buf_lock);
// What if the client calls again. // What if the client calls again.
if (port == PORT_INDEX_IN) { if (port == PORT_INDEX_IN) {
auto_lock l(m_lock); auto_lock l(m_lock);
@ -3637,7 +3638,12 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp,
unsigned int nPortIndex; unsigned int nPortIndex;
DEBUG_PRINT_LOW("In for encoder free_buffer"); 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 && if (m_state == OMX_StateIdle &&
(BITMASK_PRESENT(&m_flags ,OMX_COMPONENT_LOADING_PENDING))) { (BITMASK_PRESENT(&m_flags ,OMX_COMPONENT_LOADING_PENDING))) {
DEBUG_PRINT_LOW(" free buffer while Component in Loading pending"); DEBUG_PRINT_LOW(" free buffer while Component in Loading pending");
@ -3717,7 +3723,6 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp,
nPortIndex, (unsigned int)m_sOutPortDef.nBufferCountActual); nPortIndex, (unsigned int)m_sOutPortDef.nBufferCountActual);
if (nPortIndex < m_sOutPortDef.nBufferCountActual && if (nPortIndex < m_sOutPortDef.nBufferCountActual &&
BITMASK_PRESENT(&m_out_bm_count, nPortIndex)) { BITMASK_PRESENT(&m_out_bm_count, nPortIndex)) {
auto_lock l(m_buf_lock);
// Clear the bit associated with it. // Clear the bit associated with it.
BITMASK_CLEAR(&m_out_bm_count,nPortIndex); BITMASK_CLEAR(&m_out_bm_count,nPortIndex);
m_sOutPortDef.bPopulated = OMX_FALSE; m_sOutPortDef.bPopulated = OMX_FALSE;
@ -3997,7 +4002,7 @@ OMX_ERRORTYPE omx_video::empty_this_buffer_proxy(OMX_IN OMX_HANDLETYPE hComp,
{ {
DEBUG_PRINT_LOW("Heap UseBuffer case, so memcpy the data"); 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; 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_inp_bm_count, nBufIndex)) {
memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset), memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset),

View file

@ -2361,11 +2361,15 @@ OMX_ERRORTYPE omx_venc::component_deinit(OMX_IN OMX_HANDLETYPE hComp)
DEBUG_PRINT_ERROR("WARNING:Rxd DeInit,OMX not in LOADED state %d",\ DEBUG_PRINT_ERROR("WARNING:Rxd DeInit,OMX not in LOADED state %d",\
m_state); m_state);
} }
auto_lock l(m_buf_lock);
if (m_out_mem_ptr) { if (m_out_mem_ptr) {
DEBUG_PRINT_LOW("Freeing the Output Memory"); DEBUG_PRINT_LOW("Freeing the Output Memory");
for (i=0; i< m_sOutPortDef.nBufferCountActual; i++ ) { for (i=0; i< m_sOutPortDef.nBufferCountActual; i++ ) {
if (BITMASK_PRESENT(&m_out_bm_count, i)) { if (BITMASK_PRESENT(&m_out_bm_count, i)) {
BITMASK_CLEAR(&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]); free_output_buffer (&m_out_mem_ptr[i]);
} }
@ -2725,7 +2729,8 @@ int omx_venc::async_message_process (void *context, void* message)
omxhdr->nFlags = m_sVenc_msg->buf.flags; omxhdr->nFlags = m_sVenc_msg->buf.flags;
/*Use buffer case*/ /*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"); DEBUG_PRINT_LOW("memcpy() for o/p Heap UseBuffer");
memcpy(omxhdr->pBuffer, memcpy(omxhdr->pBuffer,
(m_sVenc_msg->buf.ptrbuffer), (m_sVenc_msg->buf.ptrbuffer),