Re: [PATCH] media: atomisp: fix CAS scaler descriptor leaks

[Dan Carpenter]

On Sat, Jun 27, 2026 at 02:01:51PM +0800, Dawei Feng wrote:
> load_video_binaries() and load_primary_binaries() create a CAS scaler
> descriptor before allocating and looking up the YUV scaler binaries.
> Several failure paths after descriptor creation return without destroying
> the descriptor, leaking the frame-info arrays owned by it.
> 
> Route those exits through a descriptor cleanup label while keeping the
> existing pipe_settings ownership model. Also clear num_yuv_scaler when
> capture scaler binary allocation fails, so the existing failure unwind does
> not iterate a NULL scaler array.
> 
> The bug was first flagged by an experimental analysis tool we are
> developing for kernel memory-management bugs while analyzing
> v6.13-rc1. The tool is still under development and is not yet publicly
> available. Manual inspection confirms that the bug is still
> present in v7.1.1.
> 
> An x86_64 allyesconfig build showed no new warnings. As we do not have
> an Intel Atom ISP camera platform with matching sensor firmware and ACPI
> camera graph to test with, no runtime testing was able to be performed.
> 
> Fixes: ad85094b293e ("Revert "media: staging: atomisp: Remove driver"")
> Signed-off-by: Dawei Feng <dawei.feng@xxxxxxxxxx>
> ---
>  drivers/staging/media/atomisp/pci/sh_css.c | 35 ++++++++++++----------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> index 00082276f1db..d0ff16ba890f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -4528,20 +4528,20 @@ static int load_video_binaries(struct ia_css_pipe *pipe)
>  			  NULL,
>  			  &cas_scaler_descr);
>  		if (err)
> -			return err;
> +			goto destroy_cas_scaler_desc;
>  		mycs->num_yuv_scaler = cas_scaler_descr.num_stage;
>  		mycs->yuv_scaler_binary = kzalloc_objs(struct ia_css_binary,
>  						       cas_scaler_descr.num_stage);
>  		if (!mycs->yuv_scaler_binary) {
>  			mycs->num_yuv_scaler = 0;
>  			err = -ENOMEM;
> -			return err;
> +			goto destroy_cas_scaler_desc;
>  		}
>  		mycs->is_output_stage = kzalloc_objs(bool,
>  						     cas_scaler_descr.num_stage);
>  		if (!mycs->is_output_stage) {
>  			err = -ENOMEM;
> -			return err;
> +			goto destroy_cas_scaler_desc;
>  		}
>  		for (i = 0; i < cas_scaler_descr.num_stage; i++) {
>  			struct ia_css_binary_descr yuv_scaler_descr;
> @@ -4557,10 +4557,13 @@ static int load_video_binaries(struct ia_css_pipe *pipe)
>  			if (err) {
>  				kfree(mycs->is_output_stage);
>  				mycs->is_output_stage = NULL;
> -				return err;
> +				goto destroy_cas_scaler_desc;

What about freeing mycs->yuv_scaler_binary?  There are a bunch of
other leaks...  I would prefer a more complete fix.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

I would probably just do the free before the goto since this is not
part of the cleanup function.

		if (err) {
			ia_css_pipe_destroy_cas_scaler_desc(&cas_scaler_descr);
			goto free_output_stage;
		}

...

	return 0;

free_output_stage:
	if (need_scalar) {
		kfree(mycs->is_output_stage);
		mycs->is_output_stage = NULL;
	}
free_scalar_binary:
	if (need_scalar) {
		kfree(mycs->yuv_scaler_binary);
		mycs->yuv_scaler_binary = NULL;
	}

etc.

regards,
dan carpenter