[PATCH] media: atomisp: Fix buffer overrun in gmin_get_var_int()

[PATCH] media: atomisp: Fix buffer overrun in gmin_get_var_int()

[Hans de Goede]

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(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 93bfb3fadcf7..139ad7ad1dcf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -1429,8 +1429,8 @@ static int gmin_get_config_var(struct device *maindev,
 
 int gmin_get_var_int(struct device *dev, bool is_gmin, const char *var, int def)
 {
-	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;
 	long result;
 	int ret;
 
-- 
2.40.1

Re: [PATCH] media: atomisp: Fix buffer overrun in gmin_get_var_int()

[Andy Shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] media: atomisp: Fix buffer overrun in gmin_get_var_int()

[Hans de Goede]

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

Re: [PATCH] media: atomisp: Fix buffer overrun in gmin_get_var_int()

[Andy Shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] media: atomisp: Fix buffer overrun in gmin_get_var_int()

[Hans de Goede]

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