media: ipu6: ipu7 and ipu6 fw-isys fixes
Subsystems (3)
v2 cover media: ipu6: ipu7 and ipu6 fw-isys fixes
Fix the firmware config leaks on every stream start in both the ipu7 and ipu6 fw-isys init paths, and a NULL dereference on a PIN_DATA_READY response for an unconfigured ipu7 output pin.
Changes in v2:
- 1/3: use goto labels for error handling (Sakari)
- 2/3: new, same leak in ipu6_isys_fwcom_cfg_init() (Sakari), also check its return value
- 3/3: unchanged
v1: https://lore.kernel.org/linux-media/[email protected]/
David Carlier (3):
media: ipu6: Fix ipu7 firmware context leak on stream start
media: ipu6: Fix firmware config leak on stream start
media: ipu6: Check the ipu7 output pin queue before completing a
buffer
drivers/media/pci/intel/ipu6/ipu6-fw-isys.c | 34 +++++++++++++++------ drivers/media/pci/intel/ipu6/ipu7-fw-isys.c | 26 ++++++++++------ 2 files changed, 41 insertions(+), 19 deletions(-)
base-commit: 2dcdfb625c3b8fe87454e19dfbc54b3e3f0ad70e
Signature · 1 line
2.55.0
PATCH v2 1/3 media: ipu6: Fix ipu7 firmware context leak on stream start
ipu7_fw_isys_init() runs on every first stream start, but allocates the firmware context and queue configs with devm and never frees them, so each STREAMON/STREAMOFF cycle leaks them. Use kzalloc and free them in ipu7_fw_isys_cleanup().
--- drivers/media/pci/intel/ipu6/ipu7-fw-isys.c | 23 ++++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-)
drivers/media/pci/intel/ipu6/ipu7-fw-isys.c +15 -8
@@ -34,6 +34,8 @@ static void ipu7_fw_isys_cleanup(struct ipu6_isys *isys) } isys->fwctx =NULL ; +kfree (fwctx->queue_configs); +kfree (fwctx); }static int ipu7_fw_isys_open (struct ipu6_isys *isys)
@@ -67,8 +69,7 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams)int ret;/* Allocate and init firmware context. */ - fwctx =devm_kzalloc (dev,sizeof (struct ipu7_fw_com_context ), -GFP_KERNEL ); + fwctx =kzalloc_obj (*fwctx);if (!fwctx)return -ENOMEM ;
@@ -76,11 +77,10 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams) fwctx->num_output_queues =IPU7_INSYS_MAX_OUTPUT_QUEUES ; num_queues = fwctx->num_input_queues + fwctx->num_output_queues; - queue_configs =devm_kcalloc (dev, num_queues,sizeof (*queue_configs), -GFP_KERNEL ); + queue_configs =kzalloc_objs (*queue_configs, num_queues);if (!queue_configs) { -ipu7_fw_isys_cleanup (isys); -return -ENOMEM ; + ret = -ENOMEM ; + goto err_free_fwctx; } fwctx->fw_entry = adev->fw_entry; fwctx->queue_configs = queue_configs;
@@ -111,8 +111,8 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams) &fw_config_dma_addr, GFP_KERNEL, 0);if (!fw_config) {dev_err (dev,"Failed to allocate isys subsys config.\n" ); -ipu7_fw_isys_cleanup (isys); -return -ENOMEM ; + ret = -ENOMEM ; + goto err_free_queue_configs; } fwctx->fw_config = fw_config; fwctx->fw_config_dma_addr = fw_config_dma_addr;
@@ -144,6 +144,13 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams)ipu7_fw_isys_cleanup (isys);return ret; + +err_free_queue_configs: +kfree (queue_configs); +err_free_fwctx: +kfree (fwctx); + +return ret; }static struct ipu7_insys_resp *ipu7_fw_isys_get_resp (struct ipu6_isys *isys)
Signature · 1 line
2.55.0
PATCH v2 2/3 media: ipu6: Fix firmware config leak on stream start
In reply to David Carlieripu6_isys_fwcom_cfg_init() runs on every first stream start and allocates the firmware config and queue configs with devm, so each STREAMON/STREAMOFF cycle leaks them. They are only needed until ipu6_fw_com_prepare() has copied them, so free them right after. Also check the return value, an allocation failure would otherwise make ipu6_fw_com_prepare() dereference a NULL queue config.
--- drivers/media/pci/intel/ipu6/ipu6-fw-isys.c | 34 +++++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-)
drivers/media/pci/intel/ipu6/ipu6-fw-isys.c +24 -10
@@ -212,7 +212,6 @@ static int ipu6_isys_fwcom_cfg_init(struct ipu6_isys *isys,unsigned int max_send_queues, max_sram_blocks, max_devq_size;struct ipu6_fw_syscom_queue_config *input_queue_cfg;struct ipu6_fw_syscom_queue_config *output_queue_cfg; -struct device *dev = &isys->adev->auxdev.dev;int type_proxy =IPU6_FW_ISYS_QUEUE_TYPE_PROXY ;int type_dev =IPU6_FW_ISYS_QUEUE_TYPE_DEV ;int type_msg =IPU6_FW_ISYS_QUEUE_TYPE_MSG ;
@@ -222,7 +221,6 @@ static int ipu6_isys_fwcom_cfg_init(struct ipu6_isys *isys,struct ipu6_fw_isys_fw_config *isys_fw_cfg;u32 num_in_message_queues;unsigned int max_streams; -unsigned int size;unsigned int i; max_streams = isys->pdata->ipdata->max_streams;
@@ -230,7 +228,7 @@ static int ipu6_isys_fwcom_cfg_init(struct ipu6_isys *isys, max_sram_blocks = isys->pdata->ipdata->max_sram_blocks; max_devq_size = isys->pdata->ipdata->max_devq_size; num_in_message_queues =clamp (num_streams,1U , max_streams); - isys_fw_cfg =devm_kzalloc (dev,sizeof (*isys_fw_cfg),GFP_KERNEL ); + isys_fw_cfg =kzalloc_obj (*isys_fw_cfg);if (!isys_fw_cfg)return -ENOMEM ;
@@ -242,15 +240,14 @@ static int ipu6_isys_fwcom_cfg_init(struct ipu6_isys *isys, isys_fw_cfg->num_recv_queues[type_dev] =0 ; isys_fw_cfg->num_recv_queues[type_msg] =1 ; - size =sizeof (*input_queue_cfg) * max_send_queues; - input_queue_cfg =devm_kzalloc (dev, size,GFP_KERNEL ); + input_queue_cfg =kzalloc_objs (*input_queue_cfg, max_send_queues);if (!input_queue_cfg) -return -ENOMEM ; + goto err_free_fw_cfg; - size =sizeof (*output_queue_cfg) *IPU6_N_MAX_RECV_QUEUES ; - output_queue_cfg =devm_kzalloc (dev, size,GFP_KERNEL ); + output_queue_cfg =kzalloc_objs (*output_queue_cfg, +IPU6_N_MAX_RECV_QUEUES );if (!output_queue_cfg) -return -ENOMEM ; + goto err_free_input_queue_cfg; fwcom_cfg->input = input_queue_cfg; fwcom_cfg->output = output_queue_cfg;
@@ -315,6 +312,20 @@ static int ipu6_isys_fwcom_cfg_init(struct ipu6_isys *isys, fwcom_cfg->specific_size =sizeof (*isys_fw_cfg);return 0 ; + +err_free_input_queue_cfg: +kfree (input_queue_cfg); +err_free_fw_cfg: +kfree (isys_fw_cfg); + +return -ENOMEM ; +} + +static void ipu6_isys_fwcom_cfg_free (struct ipu6_fw_com_cfg *fwcom_cfg) +{ +kfree (fwcom_cfg->specific_addr); +kfree (fwcom_cfg->output); +kfree (fwcom_cfg->input); }static int ipu6_fw_isys_init (struct ipu6_isys *isys,unsigned int num_streams)
@@ -328,10 +339,13 @@ static int ipu6_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams) };int ret; -ipu6_isys_fwcom_cfg_init (isys, &fwcom_cfg, num_streams); + ret =ipu6_isys_fwcom_cfg_init (isys, &fwcom_cfg, num_streams); +if (ret) +return ret; isys->fwctx =ipu6_fw_com_prepare (&fwcom_cfg, isys->adev, isys->pdata->base); +ipu6_isys_fwcom_cfg_free (&fwcom_cfg);if (!isys->fwctx) {dev_err (dev,"isys fw com prepare failed\n" );return -EIO ;
Signature · 1 line
2.55.0
PATCH v2 3/3 media: ipu6: Check the ipu7 output pin queue before completing a buffer
In reply to David CarlierA PIN_DATA_READY response for an unconfigured pin dereferences a NULL output_pins_queue entry in interrupt context. Check the queue like the ipu6 path does.
--- drivers/media/pci/intel/ipu6/ipu7-fw-isys.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/media/pci/intel/ipu6/ipu7-fw-isys.c +2 -1
@@ -644,7 +644,8 @@ static int ipu7_isys_isr_one(struct ipu6_bus_device *adev) struct isys_fw_msgs, dummy);ipu6_put_fw_msg_buf (ipu6_bus_get_drvdata (adev), isys_fw_msg); -if (resp->pin_id <IPU6_ISYS_OUTPUT_PINS ) +if (resp->pin_id <IPU6_ISYS_OUTPUT_PINS && + stream->output_pins_queue[resp->pin_id])ipu6_stream_buf_ready (stream, resp->pin_id, resp->pin.addr, ts,0 );else
Signature · 1 line
2.55.0