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

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

[Sakari Ailus]

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

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

[Andy Shevchenko]

On Wed, Feb 11, 2026 at 10:11:23AM +0200, Sakari Ailus wrote:
> On Tue, Feb 10, 2026 at 09:53:50PM +0300, Dan Carpenter wrote:

...

> 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.

I agree with a caveat (see below).

> Also cc LMML, Greg and Andy.

Perhaps also ask Hans or other people from libcamera? What does that use when
it comes to AtomISP?

-- 
With Best Regards,
Andy Shevchenko

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

[johannes.goede]

Hi,

On 11-Feb-26 09:11, Sakari Ailus wrote:
> 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.

Ack, my suggestion would be to completely remove the entire
atomisp_vidioc_default() function from:

drivers/staging/media/atomisp/pci/atomisp_ioctl.c

(and stop setting .vidioc_default)

as mentioned this is something on the TODO list anyways.

This will cause a bunch of code to turn into dead code, but I would
like to keep that code around since when we add support for
a parameter buffer queue that code can serve as an example how to send
parameters to the ISP.

Regards,

Hans

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

[Andy Shevchenko]

On Wed, Feb 11, 2026 at 12:28:35PM +0100, johannes.goede@oss.qualcomm.com wrote:
> On 11-Feb-26 09:11, Sakari Ailus wrote:

...

> This will cause a bunch of code to turn into dead code, but I would
> like to keep that code around since when we add support for
> a parameter buffer queue that code can serve as an example how to send
> parameters to the ISP.

But it's forever in the Git index, we can remove it, so it's just matter
of convenience to keep it in a working copy (tree). That being said,
I would rather drop the dead code to avoid a stream of not-so-useful
white space, style, and similar cleanups.

-- 
With Best Regards,
Andy Shevchenko

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

[johannes.goede]

Hi,

On 11-Feb-26 12:39, Andy Shevchenko wrote:
> On Wed, Feb 11, 2026 at 12:28:35PM +0100, johannes.goede@oss.qualcomm.com wrote:
>> On 11-Feb-26 09:11, Sakari Ailus wrote:
> 
> ...
> 
>> This will cause a bunch of code to turn into dead code, but I would
>> like to keep that code around since when we add support for
>> a parameter buffer queue that code can serve as an example how to send
>> parameters to the ISP.
> 
> But it's forever in the Git index, we can remove it, so it's just matter
> of convenience to keep it in a working copy (tree). That being said,
> I would rather drop the dead code to avoid a stream of not-so-useful
> white space, style, and similar cleanups.

That is a good point, dropping some of the dead-code stemming
from this is fine with me.

We should probably stop pruning dead code when we get
deep into the helpers to pack things into fw specific
formats.

Regards,

Hans

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

[Sakari Ailus]

Hi Hans, Andy,

On Wed, Feb 11, 2026 at 12:50:18PM +0100, johannes.goede@oss.qualcomm.com wrote:
> Hi,
> 
> On 11-Feb-26 12:39, Andy Shevchenko wrote:
> > On Wed, Feb 11, 2026 at 12:28:35PM +0100, johannes.goede@oss.qualcomm.com wrote:
> >> On 11-Feb-26 09:11, Sakari Ailus wrote:
> > 
> > ...
> > 
> >> This will cause a bunch of code to turn into dead code, but I would
> >> like to keep that code around since when we add support for
> >> a parameter buffer queue that code can serve as an example how to send
> >> parameters to the ISP.
> > 
> > But it's forever in the Git index, we can remove it, so it's just matter
> > of convenience to keep it in a working copy (tree). That being said,
> > I would rather drop the dead code to avoid a stream of not-so-useful
> > white space, style, and similar cleanups.
> 
> That is a good point, dropping some of the dead-code stemming
> from this is fine with me.
> 
> We should probably stop pruning dead code when we get
> deep into the helpers to pack things into fw specific
> formats.

Either works for me, however the actual IOCTL handling related code
contains less redundancy than the rest of the driver. When it comes to this
patch, I'd keep the changes small allow easy backporting.

-- 
Sakari Ailus

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

[johannes.goede]

Hi,

On 11-Feb-26 12:54, Sakari Ailus wrote:
> Hi Hans, Andy,
> 
> On Wed, Feb 11, 2026 at 12:50:18PM +0100, johannes.goede@oss.qualcomm.com wrote:
>> Hi,
>>
>> On 11-Feb-26 12:39, Andy Shevchenko wrote:
>>> On Wed, Feb 11, 2026 at 12:28:35PM +0100, johannes.goede@oss.qualcomm.com wrote:
>>>> On 11-Feb-26 09:11, Sakari Ailus wrote:
>>>
>>> ...
>>>
>>>> This will cause a bunch of code to turn into dead code, but I would
>>>> like to keep that code around since when we add support for
>>>> a parameter buffer queue that code can serve as an example how to send
>>>> parameters to the ISP.
>>>
>>> But it's forever in the Git index, we can remove it, so it's just matter
>>> of convenience to keep it in a working copy (tree). That being said,
>>> I would rather drop the dead code to avoid a stream of not-so-useful
>>> white space, style, and similar cleanups.
>>
>> That is a good point, dropping some of the dead-code stemming
>> from this is fine with me.
>>
>> We should probably stop pruning dead code when we get
>> deep into the helpers to pack things into fw specific
>> formats.
> 
> Either works for me, however the actual IOCTL handling related code
> contains less redundancy than the rest of the driver. When it comes to this
> patch, I'd keep the changes small allow easy backporting.

Ack, as said we can start with a patch just dropping the
default ioctl handler. That should be easy to backport.

Removing some of the then unused functions can be done as
followup patches.

Regards,

Hans

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

[Andy Shevchenko]

On Wed, Feb 11, 2026 at 01:31:19PM +0100, johannes.goede@oss.qualcomm.com wrote:
> On 11-Feb-26 12:54, Sakari Ailus wrote:
> > On Wed, Feb 11, 2026 at 12:50:18PM +0100, johannes.goede@oss.qualcomm.com wrote:
> >> On 11-Feb-26 12:39, Andy Shevchenko wrote:
> >>> On Wed, Feb 11, 2026 at 12:28:35PM +0100, johannes.goede@oss.qualcomm.com wrote:
> >>>> On 11-Feb-26 09:11, Sakari Ailus wrote:

...

> >>>> This will cause a bunch of code to turn into dead code, but I would
> >>>> like to keep that code around since when we add support for
> >>>> a parameter buffer queue that code can serve as an example how to send
> >>>> parameters to the ISP.
> >>>
> >>> But it's forever in the Git index, we can remove it, so it's just matter
> >>> of convenience to keep it in a working copy (tree). That being said,
> >>> I would rather drop the dead code to avoid a stream of not-so-useful
> >>> white space, style, and similar cleanups.
> >>
> >> That is a good point, dropping some of the dead-code stemming
> >> from this is fine with me.
> >>
> >> We should probably stop pruning dead code when we get
> >> deep into the helpers to pack things into fw specific
> >> formats.
> > 
> > Either works for me, however the actual IOCTL handling related code
> > contains less redundancy than the rest of the driver. When it comes to this
> > patch, I'd keep the changes small allow easy backporting.
> 
> Ack, as said we can start with a patch just dropping the
> default ioctl handler. That should be easy to backport.
> 
> Removing some of the then unused functions can be done as
> followup patches.

I am on the same page.

-- 
With Best Regards,
Andy Shevchenko

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

[soufianeda]

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
>

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

[Sakari Ailus]

Hi Soufiane,

On Wed, Feb 11, 2026 at 02:43:17PM +0100, soufianeda@tutanota.com wrote:
> 
> 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.

Oops. I read your message after posting the patch...

Indeed it sounds like we should disable all private IOCTLs, also the only
one that should have been there to begin with. (I can update my patch as
well.)

-- 
Kind regards,

Sakari Ailus

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

[soufianeda]

Hi Dan,

The issue is that res->data_bytes and arg->fmt.sizeimage are computed
from independent sources with no validation linking them.

ia_css_frame_allocate() computes data_bytes internally based on
width, height, format, and padded_width through frame_init_planes():

  frame_allocate_with_data()
    -> frame_create(width, height, ...)
    -> ia_css_frame_init_planes()
       -> frame_init_single_plane() / frame_init_nv_planes() / ...
          -> frame->data_bytes = stride * height  (varies by format)
    -> frame_allocate_buffer_data()
       -> hmm_alloc(frame->data_bytes)

But arg->fmt.sizeimage is a separate user-controlled field in
struct v4l2_framebuffer. Nothing enforces that sizeimage matches
the data_bytes computed from width/height/format. A user can pass:

  width=100, height=100  -> small data_bytes allocation
  sizeimage=1048576      -> 1MB copy via hmm_store()

The hmm_store() then does memcpy() with the sizeimage length into
the data_bytes-sized buffer.

I found this by code review, then confirmed with a userspace harness
compiled with AFL++/ASAN that simulates the allocation and copy. The
ASAN output shows heap-buffer-overflow immediately with mismatched
values.

The ioctl path is:

  ioctl(fd, ATOMISP_IOC_S_ISP_FPN_TABLE, &fb)
    -> atomisp_fixed_pattern_table()
    -> atomisp_v4l2_framebuffer_to_css_frame()

Regarding your suggestion about bounds checking in hmm_store() -
that would also work, but hmm_store() is a generic function used
elsewhere. Validating at the call site before we even vmalloc the
oversized tmp_buf seems cleaner and catches it earlier.

Regards,
Soufiane Dani

11 Feb 2026 at 06:35 by dan.carpenter@linaro.org:

> Please send this email to the list.
>
> This information is not secret and should be included in the
> commit message.
>
> regards,
> dan carpenter
>
>