media: atomisp: Fix buffer overrun in gmin_get_var_int()

Not all functions used in gmin_get_var_int() update len to the actual
length of the returned string. So len may still have its initial value
of the length of val[] when "val[len] = 0;" is run to ensure 0 termination.

If this happens we end up writing one beyond the bounds of val[], fix this.

Note this is a quick fix for this since the entirety of
atomisp_gmin_platform.c will be removed once all atomisp sensor
drivers have been moved over to runtime-pm + v4l2-async device
registration.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-media/26f37e19-c240-4d77-831d-ef3f1a4dd51d@kili.mountain/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On Sat, May 27, 2023 at 6:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Not all functions used in gmin_get_var_int() update len to the actual
> length of the returned string. So len may still have its initial value
> of the length of val[] when "val[len] = 0;" is run to ensure 0 termination.
>
> If this happens we end up writing one beyond the bounds of val[], fix this.
>
> Note this is a quick fix for this since the entirety of
> atomisp_gmin_platform.c will be removed once all atomisp sensor
> drivers have been moved over to runtime-pm + v4l2-async device
> registration.

...

> Closes: https://lore.kernel.org/linux-media/26f37e19-c240-4d77-831d-ef3f1a4dd51d@kili.mountain/

Is this a new official tag? (Just to my surprise)

...

> -       char val[CFG_VAR_NAME_MAX];
> -       size_t len = sizeof(val);
> +       char val[CFG_VAR_NAME_MAX + 1];
> +       size_t len = CFG_VAR_NAME_MAX;

Why not sizeof() - 1? At least it will be a single point when change
can be made and not breaking again in a similar way the code.

>         long result;
>         int ret;
  

Hi,

On 5/27/23 18:55, Andy Shevchenko wrote:
> On Sat, May 27, 2023 at 6:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Not all functions used in gmin_get_var_int() update len to the actual
>> length of the returned string. So len may still have its initial value
>> of the length of val[] when "val[len] = 0;" is run to ensure 0 termination.
>>
>> If this happens we end up writing one beyond the bounds of val[], fix this.
>>
>> Note this is a quick fix for this since the entirety of
>> atomisp_gmin_platform.c will be removed once all atomisp sensor
>> drivers have been moved over to runtime-pm + v4l2-async device
>> registration.
> 
> ...
> 
>> Closes: https://lore.kernel.org/linux-media/26f37e19-c240-4d77-831d-ef3f1a4dd51d@kili.mountain/
> 
> Is this a new official tag? (Just to my surprise)

Yes, I was surprised too, checkpatch.pl now wants a Closes: tag
after a Reported-by: one, rather then a Link: tag.

> 
> ...
> 
>> -       char val[CFG_VAR_NAME_MAX];
>> -       size_t len = sizeof(val);
>> +       char val[CFG_VAR_NAME_MAX + 1];
>> +       size_t len = CFG_VAR_NAME_MAX;
> 
> Why not sizeof() - 1? At least it will be a single point when change
> can be made and not breaking again in a similar way the code.

I wanted to make the buffer one byte bigger to make room for
the terminating 0, not 1 bute smaller.

Regards,

Hams
  

On Sat, May 27, 2023 at 8:54 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/27/23 18:55, Andy Shevchenko wrote:
> > On Sat, May 27, 2023 at 6:51 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> Closes: https://lore.kernel.org/linux-media/26f37e19-c240-4d77-831d-ef3f1a4dd51d@kili.mountain/
> >
> > Is this a new official tag? (Just to my surprise)
>
> Yes, I was surprised too, checkpatch.pl now wants a Closes: tag
> after a Reported-by: one, rather then a Link: tag.

Interesting...

...

> >> -       char val[CFG_VAR_NAME_MAX];
> >> -       size_t len = sizeof(val);
> >> +       char val[CFG_VAR_NAME_MAX + 1];
> >> +       size_t len = CFG_VAR_NAME_MAX;
> >
> > Why not sizeof() - 1? At least it will be a single point when change
> > can be made and not breaking again in a similar way the code.
>
> I wanted to make the buffer one byte bigger to make room for
> the terminating 0, not 1 bute smaller.

I understand, and I'm commenting only on the len assignment. Sorry for
not being clear.

Hence you will have

  buf[SIZE + 1];
  sizeof(buf) - 1;
  

Hi,

On 5/28/23 09:55, Andy Shevchenko wrote:
> On Sat, May 27, 2023 at 8:54 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/27/23 18:55, Andy Shevchenko wrote:
>>> On Sat, May 27, 2023 at 6:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> Closes: https://lore.kernel.org/linux-media/26f37e19-c240-4d77-831d-ef3f1a4dd51d@kili.mountain/
>>>
>>> Is this a new official tag? (Just to my surprise)
>>
>> Yes, I was surprised too, checkpatch.pl now wants a Closes: tag
>> after a Reported-by: one, rather then a Link: tag.
> 
> Interesting...
> 
> ...
> 
>>>> -       char val[CFG_VAR_NAME_MAX];
>>>> -       size_t len = sizeof(val);
>>>> +       char val[CFG_VAR_NAME_MAX + 1];
>>>> +       size_t len = CFG_VAR_NAME_MAX;
>>>
>>> Why not sizeof() - 1? At least it will be a single point when change
>>> can be made and not breaking again in a similar way the code.
>>
>> I wanted to make the buffer one byte bigger to make room for
>> the terminating 0, not 1 bute smaller.
> 
> I understand, and I'm commenting only on the len assignment. Sorry for
> not being clear.
> 
> Hence you will have
> 
>   buf[SIZE + 1];
>   sizeof(buf) - 1;

That is just ugly IMHO, why take the sizeof something which
we know is SIZE + 1 and then substract 1 instead of just
writing SIZE ?

Note that for any future SIZE define changes both methods
are equally future proof in that they both automatically
adjust to the define changing.

Regards,

Hans