| Message ID | 20230527155136.61957-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | fadac6afccf7d8a4efa1e0ca89958f6716685333 |
| 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;
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;
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(-)