From 6c606f60096f8ad69708c7dd5322ed38f9e4f41c Mon Sep 17 00:00:00 2001 From: Govindaraj Rajagopal Date: Fri, 13 Dec 2019 16:15:51 +0530 Subject: [PATCH 1/4] msm: vidc: remove additional checks in response_handler possibility of OOB access on device->response_pkt in __response_handler. for e.x if msg queue contains 1000 messages and all 1000 were read and queue is empty. So __get_q_size api will return zero and _iface_msgq_read will go in an infinite loop, even if packet_count == max_packets. Change-Id: I3c0fb095feff0ba5d4d6dab65ed9d5111f1b6f05 Signed-off-by: Govindaraj Rajagopal --- drivers/media/platform/msm/vidc/venus_hfi.c | 31 +-------------------- 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/drivers/media/platform/msm/vidc/venus_hfi.c b/drivers/media/platform/msm/vidc/venus_hfi.c index e5fad84020bf..18ad1584c8b3 100644 --- a/drivers/media/platform/msm/vidc/venus_hfi.c +++ b/drivers/media/platform/msm/vidc/venus_hfi.c @@ -2371,34 +2371,6 @@ static int venus_hfi_core_release(void *dev) return rc; } -static int __get_q_size(struct venus_hfi_device *dev, unsigned int q_index) -{ - struct hfi_queue_header *queue; - struct vidc_iface_q_info *q_info; - u32 write_ptr, read_ptr; - - if (q_index >= VIDC_IFACEQ_NUMQ) { - dprintk(VIDC_ERR, "Invalid q index: %d\n", q_index); - return -ENOENT; - } - - q_info = &dev->iface_queues[q_index]; - if (!q_info) { - dprintk(VIDC_ERR, "cannot read shared Q's\n"); - return -ENOENT; - } - - queue = (struct hfi_queue_header *)q_info->q_hdr; - if (!queue) { - dprintk(VIDC_ERR, "queue not present\n"); - return -ENOENT; - } - - write_ptr = (u32)queue->qhdr_write_idx; - read_ptr = (u32)queue->qhdr_read_idx; - return read_ptr - write_ptr; -} - static void __core_clear_interrupt(struct venus_hfi_device *device) { u32 intr_status = 0; @@ -3671,8 +3643,7 @@ static int __response_handler(struct venus_hfi_device *device) *session_id = session->session_id; } - if (packet_count >= max_packets && - __get_q_size(device, VIDC_IFACEQ_MSGQ_IDX)) { + if (packet_count >= max_packets) { dprintk(VIDC_WARN, "Too many packets in message queue to handle at once, deferring read\n"); break; From 80cdbd88098f76190a9837c20a89bc6ad3c63403 Mon Sep 17 00:00:00 2001 From: Jhansi Konathala Date: Fri, 29 Nov 2019 17:32:07 +0530 Subject: [PATCH 2/4] asoc: msm-compress: Add lock in controls _put() and _get() callback Few mixer controls _put and _get methods uses runtime private data that can be freed by close() callback in parallel threads leading to issue. Added global mutex lock in such methods to avoid runtime concurrency around such data. Change-Id: Ie542c64a4f1e50fd9547ebb9f65df2b7b0c21a0e Signed-off-by: Ajit Pandey Signed-off-by: Jhansi Konathala --- sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c | 84 +++++++++++++++++----- 1 file changed, 66 insertions(+), 18 deletions(-) diff --git a/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c b/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c index 3746a201b23c..10976bf8f94b 100644 --- a/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c +++ b/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2012-2019, The Linux Foundation. All rights reserved. +/* Copyright (c) 2012-2020, The Linux Foundation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 and @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -107,6 +108,7 @@ struct msm_compr_pdata { int32_t ion_fd[MSM_FRONTEND_DAI_MAX]; bool is_in_use[MSM_FRONTEND_DAI_MAX]; bool avs_ver; + struct mutex lock; }; struct msm_compr_audio { @@ -1851,6 +1853,7 @@ static int msm_compr_playback_free(struct snd_compr_stream *cstream) } spin_unlock_irqrestore(&prtd->lock, flags); + mutex_lock(&pdata->lock); pdata->cstream[soc_prtd->dai_link->be_id] = NULL; if (cstream->direction == SND_COMPRESS_PLAYBACK) { msm_pcm_routing_dereg_phy_stream(soc_prtd->dai_link->be_id, @@ -1880,6 +1883,7 @@ static int msm_compr_playback_free(struct snd_compr_stream *cstream) pdata->is_in_use[soc_prtd->dai_link->be_id] = false; kfree(prtd); runtime->private_data = NULL; + mutex_unlock(&pdata->lock); return 0; } @@ -1930,6 +1934,7 @@ static int msm_compr_capture_free(struct snd_compr_stream *cstream) } spin_unlock_irqrestore(&prtd->lock, flags); + mutex_lock(&pdata->lock); pdata->cstream[soc_prtd->dai_link->be_id] = NULL; msm_pcm_routing_dereg_phy_stream(soc_prtd->dai_link->be_id, SNDRV_PCM_STREAM_CAPTURE); @@ -1940,6 +1945,7 @@ static int msm_compr_capture_free(struct snd_compr_stream *cstream) kfree(prtd); runtime->private_data = NULL; + mutex_unlock(&pdata->lock); return 0; } @@ -3228,6 +3234,7 @@ static int msm_compr_audio_effects_config_put(struct snd_kcontrol *kcontrol, struct msm_compr_audio *prtd = NULL; long *values = &(ucontrol->value.integer.value[0]); int effects_module; + int ret = 0; pr_debug("%s\n", __func__); if (fe_id >= MSM_FRONTEND_DAI_MAX) { @@ -3235,16 +3242,19 @@ static int msm_compr_audio_effects_config_put(struct snd_kcontrol *kcontrol, __func__, fe_id); return -EINVAL; } + mutex_lock(&pdata->lock); cstream = pdata->cstream[fe_id]; audio_effects = pdata->audio_effects[fe_id]; if (!cstream || !audio_effects) { pr_err("%s: stream or effects inactive\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } prtd = cstream->runtime->private_data; if (!prtd) { pr_err("%s: cannot set audio effects\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } if (prtd->compr_passthr != LEGACY_PCM) { pr_debug("%s: No effects for compr_type[%d]\n", @@ -3310,9 +3320,11 @@ static int msm_compr_audio_effects_config_put(struct snd_kcontrol *kcontrol, break; default: pr_err("%s Invalid effects config module\n", __func__); - return -EINVAL; + ret = -EINVAL; } - return 0; +done: + mutex_unlock(&pdata->lock); + return ret; } static int msm_compr_audio_effects_config_get(struct snd_kcontrol *kcontrol, @@ -3325,6 +3337,7 @@ static int msm_compr_audio_effects_config_get(struct snd_kcontrol *kcontrol, struct msm_compr_audio_effects *audio_effects = NULL; struct snd_compr_stream *cstream = NULL; struct msm_compr_audio *prtd = NULL; + int ret = 0; pr_debug("%s\n", __func__); if (fe_id >= MSM_FRONTEND_DAI_MAX) { @@ -3332,19 +3345,23 @@ static int msm_compr_audio_effects_config_get(struct snd_kcontrol *kcontrol, __func__, fe_id); return -EINVAL; } + mutex_lock(&pdata->lock); cstream = pdata->cstream[fe_id]; audio_effects = pdata->audio_effects[fe_id]; if (!cstream || !audio_effects) { pr_err("%s: stream or effects inactive\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } prtd = cstream->runtime->private_data; if (!prtd) { pr_err("%s: cannot set audio effects\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } - - return 0; +done: + mutex_unlock(&pdata->lock); + return ret; } static int msm_compr_query_audio_effect_put(struct snd_kcontrol *kcontrol, @@ -3357,6 +3374,7 @@ static int msm_compr_query_audio_effect_put(struct snd_kcontrol *kcontrol, struct msm_compr_audio_effects *audio_effects = NULL; struct snd_compr_stream *cstream = NULL; struct msm_compr_audio *prtd = NULL; + int ret = 0; long *values = &(ucontrol->value.integer.value[0]); if (fe_id >= MSM_FRONTEND_DAI_MAX) { @@ -3364,28 +3382,34 @@ static int msm_compr_query_audio_effect_put(struct snd_kcontrol *kcontrol, __func__, fe_id); return -EINVAL; } + mutex_lock(&pdata->lock); cstream = pdata->cstream[fe_id]; audio_effects = pdata->audio_effects[fe_id]; if (!cstream || !audio_effects) { pr_err("%s: stream or effects inactive\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } prtd = cstream->runtime->private_data; if (!prtd) { pr_err("%s: cannot set audio effects\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } if (prtd->compr_passthr != LEGACY_PCM) { pr_err("%s: No effects for compr_type[%d]\n", __func__, prtd->compr_passthr); - return -EPERM; + ret = -EPERM; + goto done; } audio_effects->query.mod_id = (u32)*values++; audio_effects->query.parm_id = (u32)*values++; audio_effects->query.size = (u32)*values++; audio_effects->query.offset = (u32)*values++; audio_effects->query.device = (u32)*values++; - return 0; +done: + mutex_unlock(&pdata->lock); + return ret; } static int msm_compr_query_audio_effect_get(struct snd_kcontrol *kcontrol, @@ -3398,6 +3422,7 @@ static int msm_compr_query_audio_effect_get(struct snd_kcontrol *kcontrol, struct msm_compr_audio_effects *audio_effects = NULL; struct snd_compr_stream *cstream = NULL; struct msm_compr_audio *prtd = NULL; + int ret = 0; long *values = &(ucontrol->value.integer.value[0]); if (fe_id >= MSM_FRONTEND_DAI_MAX) { @@ -3405,23 +3430,28 @@ static int msm_compr_query_audio_effect_get(struct snd_kcontrol *kcontrol, __func__, fe_id); return -EINVAL; } + mutex_lock(&pdata->lock); cstream = pdata->cstream[fe_id]; audio_effects = pdata->audio_effects[fe_id]; if (!cstream || !audio_effects) { pr_debug("%s: stream or effects inactive\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } prtd = cstream->runtime->private_data; if (!prtd) { pr_err("%s: cannot set audio effects\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto done; } values[0] = (long)audio_effects->query.mod_id; values[1] = (long)audio_effects->query.parm_id; values[2] = (long)audio_effects->query.size; values[3] = (long)audio_effects->query.offset; values[4] = (long)audio_effects->query.device; - return 0; +done: + mutex_unlock(&pdata->lock); + return ret; } static int msm_compr_send_dec_params(struct snd_compr_stream *cstream, @@ -3508,6 +3538,7 @@ static int msm_compr_dec_params_put(struct snd_kcontrol *kcontrol, goto end; } + mutex_lock(&pdata->lock); switch (prtd->codec) { case FORMAT_MP3: case FORMAT_MPEG4_AAC: @@ -3556,6 +3587,7 @@ static int msm_compr_dec_params_put(struct snd_kcontrol *kcontrol, } end: pr_debug("%s: ret %d\n", __func__, rc); + mutex_unlock(&pdata->lock); return rc; } @@ -4093,6 +4125,7 @@ static int msm_compr_probe(struct snd_soc_platform *platform) if (!pdata) return -ENOMEM; + mutex_init(&pdata->lock); snd_soc_platform_set_drvdata(platform, pdata); for (i = 0; i < MSM_FRONTEND_DAI_MAX; i++) { @@ -4138,6 +4171,20 @@ static int msm_compr_probe(struct snd_soc_platform *platform) return 0; } +static int msm_compr_remove(struct snd_soc_platform *platform) +{ + + struct msm_compr_pdata *pdata = (struct msm_compr_pdata *) + snd_soc_platform_get_drvdata(platform); + if (!pdata) { + pr_err("%s pdata is null\n", __func__); + return -ENOMEM; + } + + mutex_destroy(&pdata->lock); + kfree(pdata); + return 0; +} static int msm_compr_volume_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { @@ -4900,6 +4947,7 @@ static struct snd_soc_platform_driver msm_soc_platform = { .probe = msm_compr_probe, .compr_ops = &msm_compr_ops, .pcm_new = msm_compr_new, + .remove = msm_compr_remove, }; static int msm_compr_dev_probe(struct platform_device *pdev) @@ -4910,7 +4958,7 @@ static int msm_compr_dev_probe(struct platform_device *pdev) &msm_soc_platform); } -static int msm_compr_remove(struct platform_device *pdev) +static int msm_compr_dev_remove(struct platform_device *pdev) { snd_soc_unregister_platform(&pdev->dev); return 0; @@ -4929,7 +4977,7 @@ static struct platform_driver msm_compr_driver = { .of_match_table = msm_compr_dt_match, }, .probe = msm_compr_dev_probe, - .remove = msm_compr_remove, + .remove = msm_compr_dev_remove, }; static int __init msm_soc_platform_init(void) From 6d644704248525d51f3551372dc1d6a82535be78 Mon Sep 17 00:00:00 2001 From: Jhansi Konathala Date: Fri, 29 Nov 2019 18:33:49 +0530 Subject: [PATCH 3/4] asoc: msm-compress: Replace goto with return in case of invalid value In case of invalid values in _put() callback return directly to avoid deadlock issue with mutex unlocking in goto label. Change-Id: Ib0623e26dd83b96cd6ec315f515098b8ea0b2dd2 Signed-off-by: Ajit Pandey Signed-off-by: Jhansi Konathala --- sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c b/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c index 10976bf8f94b..7a2c50c28e77 100644 --- a/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c +++ b/sound/soc/msm/qdsp6v2/msm-compress-q6-v2.c @@ -3519,8 +3519,7 @@ static int msm_compr_dec_params_put(struct snd_kcontrol *kcontrol, if (fe_id >= MSM_FRONTEND_DAI_MAX) { pr_err("%s Received out of bounds fe_id %lu\n", __func__, fe_id); - rc = -EINVAL; - goto end; + return -EINVAL; } cstream = pdata->cstream[fe_id]; @@ -3528,14 +3527,12 @@ static int msm_compr_dec_params_put(struct snd_kcontrol *kcontrol, if (!cstream || !dec_params) { pr_err("%s: stream or dec_params inactive\n", __func__); - rc = -EINVAL; - goto end; + return -EINVAL; } prtd = cstream->runtime->private_data; if (!prtd) { pr_err("%s: cannot set dec_params\n", __func__); - rc = -EINVAL; - goto end; + return -EINVAL; } mutex_lock(&pdata->lock); From 106a302993135d7b342c0a52c7c79c10eb349549 Mon Sep 17 00:00:00 2001 From: Dikshita Agarwal Date: Wed, 8 Jan 2020 19:03:03 +0530 Subject: [PATCH 4/4] msm: vidc: avoid OOB write while accessing memory Exclude 4 bytes which holds the size of the buffer while calculating the actual buffer size to avoid OOB write. Change-Id: I5471fabc3652a942797019c5beb06d17a713b079 Signed-off-by: Dikshita Agarwal --- drivers/media/platform/msm/vidc/venus_hfi.c | 23 +++++++++------------ 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/drivers/media/platform/msm/vidc/venus_hfi.c b/drivers/media/platform/msm/vidc/venus_hfi.c index e5fad84020bf..b432196113e6 100644 --- a/drivers/media/platform/msm/vidc/venus_hfi.c +++ b/drivers/media/platform/msm/vidc/venus_hfi.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2012-2019, The Linux Foundation. All rights reserved. +/* Copyright (c) 2012-2020, The Linux Foundation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 and @@ -3395,9 +3395,11 @@ exit: return; } -static void __process_sys_error(struct venus_hfi_device *device) +static void print_sfr_message(struct venus_hfi_device *device) { struct hfi_sfr_struct *vsfr = NULL; + u32 vsfr_size = 0; + void *p = NULL; /* Once SYS_ERROR received from HW, it is safe to halt the AXI. * With SYS_ERROR, Venus FW may have crashed and HW might be @@ -3408,12 +3410,11 @@ static void __process_sys_error(struct venus_hfi_device *device) vsfr = (struct hfi_sfr_struct *)device->sfr.align_virtual_addr; if (vsfr) { - void *p = memchr(vsfr->rg_data, '\0', vsfr->bufSize); - /* SFR isn't guaranteed to be NULL terminated - since SYS_ERROR indicates that Venus is in the - process of crashing.*/ + vsfr_size = vsfr->bufSize - sizeof(u32); + p = memchr(vsfr->rg_data, '\0', vsfr_size); + /* SFR isn't guaranteed to be NULL terminated */ if (p == NULL) - vsfr->rg_data[vsfr->bufSize - 1] = '\0'; + vsfr->rg_data[vsfr_size - 1] = '\0'; dprintk(VIDC_ERR, "SFR Message from FW: %s\n", vsfr->rg_data); @@ -3537,8 +3538,6 @@ static int __response_handler(struct venus_hfi_device *device) } if (device->intr_status & VIDC_WRAPPER_INTR_CLEAR_A2HWD_BMSK) { - struct hfi_sfr_struct *vsfr = (struct hfi_sfr_struct *) - device->sfr.align_virtual_addr; struct msm_vidc_cb_info info = { .response_type = HAL_SYS_WATCHDOG_TIMEOUT, .response.cmd = { @@ -3546,9 +3545,7 @@ static int __response_handler(struct venus_hfi_device *device) } }; - if (vsfr) - dprintk(VIDC_ERR, "SFR Message from FW: %s\n", - vsfr->rg_data); + print_sfr_message(device); dprintk(VIDC_ERR, "Received watchdog timeout\n"); packets[packet_count++] = info; @@ -3574,7 +3571,7 @@ static int __response_handler(struct venus_hfi_device *device) /* Process the packet types that we're interested in */ switch (info->response_type) { case HAL_SYS_ERROR: - __process_sys_error(device); + print_sfr_message(device); break; case HAL_SYS_RELEASE_RESOURCE_DONE: dprintk(VIDC_DBG, "Received SYS_RELEASE_RESOURCE\n");