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

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

[Dawei Feng]

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@seu.edu.cn>
---
 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;
 			}
 		}
+destroy_cas_scaler_desc:
 		ia_css_pipe_destroy_cas_scaler_desc(&cas_scaler_descr);
+		if (err)
+			return err;
 	}
 
 	{
@@ -5103,24 +5106,21 @@ static int load_primary_binaries(
 			  pipe_out_info,
 			  NULL,
 			  &cas_scaler_descr);
-		if (err) {
-			IA_CSS_LEAVE_ERR_PRIVATE(err);
-			return err;
-		}
+		if (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;
-			IA_CSS_LEAVE_ERR_PRIVATE(err);
-			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;
-			IA_CSS_LEAVE_ERR_PRIVATE(err);
-			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;
@@ -5133,12 +5133,15 @@ static int load_primary_binaries(
 							     &cas_scaler_descr.vf_info[i]);
 			err = ia_css_binary_find(&yuv_scaler_descr,
 						 &mycs->yuv_scaler_binary[i]);
-			if (err) {
-				IA_CSS_LEAVE_ERR_PRIVATE(err);
-				return err;
-			}
+			if (err)
+				goto destroy_cas_scaler_desc;
 		}
+destroy_cas_scaler_desc:
 		ia_css_pipe_destroy_cas_scaler_desc(&cas_scaler_descr);
+		if (err) {
+			IA_CSS_LEAVE_ERR_PRIVATE(err);
+			return err;
+		}
 
 	} else {
 		capt_pp_out_info = pipe->output_info[0];
-- 
2.34.1

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@seu.edu.cn>
> ---
>  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

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

[Andy Shevchenko]

On Mon, Jun 29, 2026 at 11:30:40AM +0300, Dan Carpenter wrote:
> On Sat, Jun 27, 2026 at 02:01:51PM +0800, Dawei Feng wrote:

...

> 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;
> 	}

If we go this way, double check that the checks are needed as we have kfree()
to be NULL-aware.

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Carpenter]

On Mon, Jun 29, 2026 at 02:16:21PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 29, 2026 at 11:30:40AM +0300, Dan Carpenter wrote:
> > On Sat, Jun 27, 2026 at 02:01:51PM +0800, Dawei Feng wrote:
> 
> ...
> 
> > 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;
> > 	}
> 
> If we go this way, double check that the checks are needed as we have kfree()
> to be NULL-aware.

Dawei, Andy is the one reviewing atomisp so his opinion matters more
than mine here.  So do what he says.

But I don't really agree...

In this case, sure, hopefully the caller zeroes the ->yuv_scaler_binary
pointer, but if we just follow the simple rule of only undoing things
which we have done then we don't need to check.  The function is
self contained and self explanatory.

And more generally, I've always hated patches which delete NULL
checks before a ionmap() or whatever.  Hiding the NULL check inside the
free function makes the code less self contained.  The real fix is to
stop mixing allocated and unallocated pointers.  Then you don't need a
NULL check because you already know.  (Also I think those iounmap()
patches were wrong because some arches have a warning when you unmap
a NULL).

regards,
dan carpenter