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

[soufianeda@tutanota.com]

Hi Sakari,

I agree that removing the private IOCTL handler is the better
approach. While fuzzing the driver I found the same class of
unchecked user-controlled size fields in several other handlers
(ATOMISP_IOC_S_DIS_VECTOR, morph table, shading table), so
removing atomisp_vidioc_default() eliminates all of them at once.

I'm cool with sending a patch removing atomisp_vidioc_default() as
Hans suggested, if that would be helpful.

Regards,
Soufiane Dani


11 Feb 2026 at 09:11 by sakari.ailus@linux.intel.com:

> Hi Dan, Soufiane,
>
> On Tue, Feb 10, 2026 at 09:53:50PM +0300, Dan Carpenter wrote:
>
>> 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...
>>
>
> Indeed. Beyond that, even I have to admit I have little idea what this
> IOCTL is supposed to be doing. Possibly feed in a raw frame for processing?
> But that's not supposed to be implemented like this... The TODO file
> contains an entry that says "Remove/disable private IOCTLs" -- we should
> move to use parameter buffers instead.
>
> I'm not sure anyone depends on these IOCTLs at the moment, but definitely
> some obviously are associated with some risk.
>
> The world looked different when this code was written.
>
> I'd disable all private IOCTLs in the driver, with the possible exception
> of ATOMISP_IOC_S_ISP_PARM, which is close to the parameter buffer approach
> already.
>
> Also cc LMML, Greg and Andy.
>
> -- 
> Kind regards,
>
> Sakari Ailus
>