| Message ID | ca491aaa-cfc4-4a84-b7fc-b64f3adc6550@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] media: mediatek: vcodec: fix potential double free | expand |
Hi, Just a reminder to Yunfei that this change was originally addressed to him, and a review was to be expected. Over a month is a bit too long for fixes. Le mercredi 14 juin 2023 à 16:05 +0300, Dan Carpenter a écrit : > The "lat_buf->private_data" needs to be set to NULL to prevent a > double free. How this would happen is if vdec_msg_queue_init() failed > twice in a row and on the second time it failed earlier than on the > first time. > > The vdec_msg_queue_init() function has a loop which does: > for (i = 0; i < NUM_BUFFER_COUNT; i++) { > > Each iteration initializes one element in the msg_queue->lat_buf[] array > and then the clean up function vdec_msg_queue_deinit() frees each > element of the msg_queue->lat_buf[] array. This clean up code relies > on the assumption that every element is either initialized or zeroed. > Leaving a freed pointer which is non-zero breaks the assumption. > > Fixes: b199fe46f35c ("media: mtk-vcodec: Add msg queue feature for lat and core architecture") Unbalanced calls to deinit/init would be unfortunate, but I like keeping it safe and aligned with the fact everything is clears to 0/null otherwise. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c > index f555341ae708..92ac82eb444e 100644 > --- a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c > +++ b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c > @@ -231,6 +231,7 @@ void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue, > mtk_vcodec_mem_free(ctx, mem); > > kfree(lat_buf->private_data); > + lat_buf->private_data = NULL; > } > > cancel_work_sync(&msg_queue->core_work);
diff --git a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c index f555341ae708..92ac82eb444e 100644 --- a/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c +++ b/drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c @@ -231,6 +231,7 @@ void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue, mtk_vcodec_mem_free(ctx, mem); kfree(lat_buf->private_data); + lat_buf->private_data = NULL; } cancel_work_sync(&msg_queue->core_work);
The "lat_buf->private_data" needs to be set to NULL to prevent a double free. How this would happen is if vdec_msg_queue_init() failed twice in a row and on the second time it failed earlier than on the first time. The vdec_msg_queue_init() function has a loop which does: for (i = 0; i < NUM_BUFFER_COUNT; i++) { Each iteration initializes one element in the msg_queue->lat_buf[] array and then the clean up function vdec_msg_queue_deinit() frees each element of the msg_queue->lat_buf[] array. This clean up code relies on the assumption that every element is either initialized or zeroed. Leaving a freed pointer which is non-zero breaks the assumption. Fixes: b199fe46f35c ("media: mtk-vcodec: Add msg queue feature for lat and core architecture") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/media/platform/mediatek/vcodec/vdec_msg_queue.c | 1 + 1 file changed, 1 insertion(+)