| Message ID | 20230527155136.61957-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: Fix buffer overrun in gmin_get_var_int() | expand |
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
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;
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(-)