[PATCH] staging: media: ipu7: fix boot_config leak on queue_mem failure

[PATCH] staging: media: ipu7: fix boot_config leak on queue_mem failure

[Huihui Huang]

Our code analyzer reported a memory leak in
drivers/staging/media/ipu7/ipu7-boot.c.

In ipu7_boot_init_boot_config(), boot_config is allocated by
ipu7_dma_alloc(). If the second ipu7_dma_alloc() for queue_mem fails,
the function returns -ENOMEM without freeing the previously allocated
boot_config.

My patch adds the missing ipu7_dma_free() call and sets boot_config
to NULL before returning on the error path, matching the cleanup in
ipu7_boot_release_boot_config().

Signed-off-by: Huihui Huang <hhhuang@smu.edu.sg>
---
 drivers/staging/media/ipu7/ipu7-boot.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/ipu7/ipu7-boot.c b/drivers/staging/media/ipu7/ipu7-boot.c
index d7901ff78b38..063312d33603 100644
--- a/drivers/staging/media/ipu7/ipu7-boot.c
+++ b/drivers/staging/media/ipu7/ipu7-boot.c
@@ -263,6 +263,9 @@ int ipu7_boot_init_boot_config(struct ipu7_bus_device *adev,
 					   GFP_KERNEL, 0);
 	if (!syscom->queue_mem) {
 		dev_err(dev, "Failed to allocate queue memory.\n");
+		ipu7_dma_free(adev, adev->boot_config_size,
+			      adev->boot_config, adev->boot_config_dma_addr, 0);
+		adev->boot_config = NULL;
 		return -ENOMEM;
 	}
 	syscom->queue_mem_size = total_queue_size_aligned;
-- 
2.50.1

[PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure

[Huihui Huang]

There is a memory leak in drivers/staging/media/ipu7/ipu7-boot.c.

In ipu7_boot_init_boot_config(), boot_config is allocated by
ipu7_dma_alloc(). If the second ipu7_dma_alloc() for queue_mem fails,
the function returns -ENOMEM without freeing the previously allocated
boot_config.

Add the missing ipu7_dma_free() call before returning on the error
path.

Signed-off-by: Huihui Huang <hhhuang@smu.edu.sg>
---
v2: Reword commit message in imperative mood. Remove unnecessary
    NULL assignment on the error path.
---
 drivers/staging/media/ipu7/ipu7-boot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/ipu7/ipu7-boot.c b/drivers/staging/media/ipu7/ipu7-boot.c
index d7901ff78b38..495b3e05a9b1 100644
--- a/drivers/staging/media/ipu7/ipu7-boot.c
+++ b/drivers/staging/media/ipu7/ipu7-boot.c
@@ -263,6 +263,8 @@ int ipu7_boot_init_boot_config(struct ipu7_bus_device *adev,
 					   GFP_KERNEL, 0);
 	if (!syscom->queue_mem) {
 		dev_err(dev, "Failed to allocate queue memory.\n");
+		ipu7_dma_free(adev, adev->boot_config_size,
+			      adev->boot_config, adev->boot_config_dma_addr, 0);
 		return -ENOMEM;
 	}
 	syscom->queue_mem_size = total_queue_size_aligned;
-- 
2.50.1

Re: [PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure

[Dan Carpenter]

On Fri, Apr 17, 2026 at 03:39:39PM +0800, Huihui Huang wrote:
> There is a memory leak in drivers/staging/media/ipu7/ipu7-boot.c.
> 
> In ipu7_boot_init_boot_config(), boot_config is allocated by
> ipu7_dma_alloc(). If the second ipu7_dma_alloc() for queue_mem fails,
> the function returns -ENOMEM without freeing the previously allocated
> boot_config.
> 
> Add the missing ipu7_dma_free() call before returning on the error
> path.
> 
> Signed-off-by: Huihui Huang <hhhuang@smu.edu.sg>
> ---
> v2: Reword commit message in imperative mood. Remove unnecessary
>     NULL assignment on the error path.
> ---
>  drivers/staging/media/ipu7/ipu7-boot.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/media/ipu7/ipu7-boot.c b/drivers/staging/media/ipu7/ipu7-boot.c
> index d7901ff78b38..495b3e05a9b1 100644
> --- a/drivers/staging/media/ipu7/ipu7-boot.c
> +++ b/drivers/staging/media/ipu7/ipu7-boot.c
> @@ -263,6 +263,8 @@ int ipu7_boot_init_boot_config(struct ipu7_bus_device *adev,
>  					   GFP_KERNEL, 0);
>  	if (!syscom->queue_mem) {
>  		dev_err(dev, "Failed to allocate queue memory.\n");
> +		ipu7_dma_free(adev, adev->boot_config_size,
> +			      adev->boot_config, adev->boot_config_dma_addr, 0);
>  		return -ENOMEM;

Adding a free here leads to a double free.  It's the same issue.
One magical cleanup function in the caller.

I haven't looked at this but I bet there are bugs in the error handling
since magical cleanup functions are always buggy.

regards,
dan carpenter

Re: [PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure

[Dan Carpenter]

On Fri, Apr 17, 2026 at 11:01:29AM +0300, Dan Carpenter wrote:
> On Fri, Apr 17, 2026 at 03:39:39PM +0800, Huihui Huang wrote:
> > There is a memory leak in drivers/staging/media/ipu7/ipu7-boot.c.
> > 
> > In ipu7_boot_init_boot_config(), boot_config is allocated by
> > ipu7_dma_alloc(). If the second ipu7_dma_alloc() for queue_mem fails,
> > the function returns -ENOMEM without freeing the previously allocated
> > boot_config.
> > 
> > Add the missing ipu7_dma_free() call before returning on the error
> > path.
> > 
> > Signed-off-by: Huihui Huang <hhhuang@smu.edu.sg>
> > ---
> > v2: Reword commit message in imperative mood. Remove unnecessary
> >     NULL assignment on the error path.
> > ---
> >  drivers/staging/media/ipu7/ipu7-boot.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/staging/media/ipu7/ipu7-boot.c b/drivers/staging/media/ipu7/ipu7-boot.c
> > index d7901ff78b38..495b3e05a9b1 100644
> > --- a/drivers/staging/media/ipu7/ipu7-boot.c
> > +++ b/drivers/staging/media/ipu7/ipu7-boot.c
> > @@ -263,6 +263,8 @@ int ipu7_boot_init_boot_config(struct ipu7_bus_device *adev,
> >  					   GFP_KERNEL, 0);
> >  	if (!syscom->queue_mem) {
> >  		dev_err(dev, "Failed to allocate queue memory.\n");
> > +		ipu7_dma_free(adev, adev->boot_config_size,
> > +			      adev->boot_config, adev->boot_config_dma_addr, 0);
> >  		return -ENOMEM;
> 
> Adding a free here leads to a double free.  It's the same issue.
> One magical cleanup function in the caller.
> 
> I haven't looked at this but I bet there are bugs in the error handling
> since magical cleanup functions are always buggy.

Btw, if you had kept the "adev->boot_config = NULL;" assignment that
you had in v1 then that would have prevented the double free since
ipu7_boot_release_boot_config() tests for that...  This information is
not useful to you at this point but I'm sure you will find it
frustrating.  :P

regards,
dan carpenter

Re: [PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure

[Dan Carpenter]

On Fri, Apr 17, 2026 at 11:01:29AM +0300, Dan Carpenter wrote:
> I haven't looked at this but I bet there are bugs in the error handling
> since magical cleanup functions are always buggy.

The error handling in ipu7_fw_isys_init() is frustrating to review but
it works fine.

Imagine that someone were writing this error handling methodically, using
a normal unwind ladder.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

drivers/staging/media/ipu7/ipu7-fw-isys.c 
    81  int ipu7_fw_isys_init(struct ipu7_isys *isys)
    82  {
    83          struct syscom_queue_config *queue_configs;
    84          struct ipu7_bus_device *adev = isys->adev;
    85          struct device *dev = &adev->auxdev.dev;
    86          struct ipu7_insys_config *isys_config;
    87          struct ipu7_syscom_context *syscom;
    88          dma_addr_t isys_config_dma_addr;
    89          unsigned int i, num_queues;
    90          u32 freq;
    91          u8 major;
    92          int ret;
    93  
    94          /* Allocate and init syscom context. */
    95          syscom = devm_kzalloc(dev, sizeof(struct ipu7_syscom_context),
    96                                GFP_KERNEL);
    97          if (!syscom)
    98                  return -ENOMEM;

This is our first resource.  It's allocated with devm_ so we don't need
to free it.  Nothing to free.

    99  
   100          adev->syscom = syscom;
   101          syscom->num_input_queues = IPU_INSYS_MAX_INPUT_QUEUES;
   102          syscom->num_output_queues = IPU_INSYS_MAX_OUTPUT_QUEUES;
   103          num_queues = syscom->num_input_queues + syscom->num_output_queues;
   104          queue_configs = devm_kzalloc(dev, FW_QUEUE_CONFIG_SIZE(num_queues),
   105                                       GFP_KERNEL);

This is our second resource.  Still devm_.  Still nothing to free.

   106          if (!queue_configs) {
   107                  ipu7_fw_isys_release(isys);

And yet we call ipu7_fw_isys_release().  Fortunately it's a no-op.

   108                  return -ENOMEM;
   109          }
   110          syscom->queue_configs = queue_configs;
   111          queue_configs[IPU_INSYS_OUTPUT_MSG_QUEUE].max_capacity =
   112                  IPU_ISYS_SIZE_RECV_QUEUE;
   113          queue_configs[IPU_INSYS_OUTPUT_MSG_QUEUE].token_size_in_bytes =
   114                  sizeof(struct ipu7_insys_resp);
   115          queue_configs[IPU_INSYS_OUTPUT_LOG_QUEUE].max_capacity =
   116                  IPU_ISYS_SIZE_LOG_QUEUE;
   117          queue_configs[IPU_INSYS_OUTPUT_LOG_QUEUE].token_size_in_bytes =
   118                  sizeof(struct ipu7_insys_resp);
   119          queue_configs[IPU_INSYS_OUTPUT_RESERVED_QUEUE].max_capacity = 0;
   120          queue_configs[IPU_INSYS_OUTPUT_RESERVED_QUEUE].token_size_in_bytes = 0;
   121  
   122          queue_configs[IPU_INSYS_INPUT_DEV_QUEUE].max_capacity =
   123                  IPU_ISYS_MAX_STREAMS;
   124          queue_configs[IPU_INSYS_INPUT_DEV_QUEUE].token_size_in_bytes =
   125                  sizeof(struct ipu7_insys_send_queue_token);
   126  
   127          for (i = IPU_INSYS_INPUT_MSG_QUEUE; i < num_queues; i++) {
   128                  queue_configs[i].max_capacity = IPU_ISYS_SIZE_SEND_QUEUE;
   129                  queue_configs[i].token_size_in_bytes =
   130                          sizeof(struct ipu7_insys_send_queue_token);
   131          }
   132  
   133          /* Allocate ISYS subsys config. */
   134          isys_config = ipu7_dma_alloc(adev, sizeof(struct ipu7_insys_config),
   135                                       &isys_config_dma_addr, GFP_KERNEL, 0);
   136          if (!isys_config) {
   137                  dev_err(dev, "Failed to allocate isys subsys config.\n");
   138                  ipu7_fw_isys_release(isys);

Still nothing to free.  This is still a no-op.

   139                  return -ENOMEM;
   140          }
   141          isys->subsys_config = isys_config;

isys->subsys_config is our first resource that we have to free.

   142          isys->subsys_config_dma_addr = isys_config_dma_addr;
   143          memset(isys_config, 0, sizeof(struct ipu7_insys_config));
   144          isys_config->logger_config.use_source_severity = 0;
   145          isys_config->logger_config.use_channels_enable_bitmask = 1;
   146          isys_config->logger_config.channels_enable_bitmask =
   147                  LOGGER_CONFIG_CHANNEL_ENABLE_SYSCOM_BITMASK;
   148          isys_config->logger_config.hw_printf_buffer_base_addr = 0U;
   149          isys_config->logger_config.hw_printf_buffer_size_bytes = 0U;
   150          isys_config->wdt_config.wdt_timer1_us = 0;
   151          isys_config->wdt_config.wdt_timer2_us = 0;
   152          ret = ipu_buttress_get_isys_freq(adev->isp, &freq);

This doesn't allocate anything, but we would need to free
isys->subsys_config.  Ideally, we would have a goto free_subsys_config;
here.

   153          if (ret) {
   154                  dev_err(dev, "Failed to get ISYS frequency.\n");
   155                  ipu7_fw_isys_release(isys);

But this does free isys->subsys_config.  Good.

   156                  return ret;
   157          }
   158  
   159          ipu7_dma_sync_single(adev, isys_config_dma_addr,
   160                               sizeof(struct ipu7_insys_config));
   161  
   162          major = is_ipu8(adev->isp->hw_ver) ? 2U : 1U;
   163          ret = ipu7_boot_init_boot_config(adev, queue_configs, num_queues,
   164                                           freq, isys_config_dma_addr, major);

This allocates one or both of syscom->queue_mem and adev->boot_config.
Ideally it would clean up partial allocations and we would do the same
goto free_subsys_config; here.

   165          if (ret)
   166                  ipu7_fw_isys_release(isys);

But this does free them along with isys->subsys_config.  Everything
works.

   167  
   168          return ret;
   169  }

regards,
dan carpenter