Re: [PATCH] staging: atomisp: fix heap buffer overflow in framebuffer conversion

[Dan Carpenter <dan.carpenter@linaro.org>]

On Tue, Feb 10, 2026 at 04:26:31PM +0100, Soufiane via B4 Relay wrote:
> From: Soufiane <soufianeda@tutanota.com>
> 
> Validate sizeimage against the allocated frame buffer size before
> hmm_store() to prevent out-of-bounds write.
> 
> Signed-off-by: Soufiane <soufianeda@tutanota.com>

We need a Fixes tag if the bug is real.

> ---
>  drivers/staging/media/atomisp/pci/atomisp_cmd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 3a4eb4f6d3be..ca7ffc7855ac 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -3326,6 +3326,11 @@ atomisp_v4l2_framebuffer_to_css_frame(const struct v4l2_framebuffer *arg,
>  		goto err;
>  	}
>  

There is some sketchy stuff happening in this code but I'm not sure I
understand the issue.  The code looks like this:

  3317          /* Note: the padded width on an ia_css_frame is in elements, not in
  3318             bytes. The RAW frame we use here should always be a 16bit RAW
  3319             frame. This is why we bytesperline/2 is equal to the padded with */
  3320          if (ia_css_frame_allocate(&res, arg->fmt.width, arg->fmt.height,
  3321                                         sh_format, padded_width, 0)) {

This allocates res.  Why would it allocate something smaller than
arg->fmt.sizeimage?  How did you find this bug?  By testing or reading
the code?  Do you have a reproducer?

  3322                  ret = -ENOMEM;
  3323                  goto err;
  3324          }

> +	if (arg->fmt.sizeimage > res->data_bytes) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +

  3325  
  3326          tmp_buf = vmalloc(arg->fmt.sizeimage);
  3327          if (!tmp_buf) {
  3328                  ret = -ENOMEM;
  3329                  goto err;
  3330          }
  3331          if (copy_from_user(tmp_buf, (void __user __force *)arg->base,
  3332                             arg->fmt.sizeimage)) {
  3333                  ret = -EFAULT;
  3334                  goto err;
  3335          }
  3336  
  3337          if (hmm_store(res->data, tmp_buf, arg->fmt.sizeimage)) {
                              ^^^^^^^^^
The worry is that the buffer this references is too small.  I would
prefer instead if there were some bounds checking before the memcpy()
calls in hmm_store().  They would use a different, smaller limit if
only part of the buffer could be used.  I don't know if that bounds
checking is really required though...

  3338                  ret = -EINVAL;
  3339                  goto err;
  3340          }
  3341  

regards,
dan carpenter