| Message ID | 20200729130222.29026-1-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter | expand |
On Wed, 29 Jul 2020 15:02:22 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) > reads one past of the end of ms->loadparm, so g_memdup() can not be used > here. > > Let's use malloc and memcpy instead! > > Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter") > Fixes: Coverity CID 1431058 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 403d30e13b..8b7bac0392 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error **errp) > char *loadparm_str; > > /* make a NUL-terminated string */ > - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); > - loadparm_str[sizeof(ms->loadparm)] = 0; > + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); > + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); > return loadparm_str; > } > > > base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a Thanks, queued to s390-fixes.
On Wed, 29 Jul 2020 at 14:05, Halil Pasic <pasic@linux.ibm.com> wrote: > > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) > reads one past of the end of ms->loadparm, so g_memdup() can not be used > here. > > Let's use malloc and memcpy instead! > > Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter") > Fixes: Coverity CID 1431058 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 403d30e13b..8b7bac0392 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error **errp) > char *loadparm_str; > > /* make a NUL-terminated string */ > - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); > - loadparm_str[sizeof(ms->loadparm)] = 0; > + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); > + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); > return loadparm_str; > } (relies on the zeroing of g_malloc0 to put in the terminator but I guess the existing comment makes that clear enough.) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Wed, Jul 29, 2020 at 03:02:22PM +0200, Halil Pasic wrote: > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) > reads one past of the end of ms->loadparm, so g_memdup() can not be used > here. > > Let's use malloc and memcpy instead! > > Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter") > Fixes: Coverity CID 1431058 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 403d30e13b..8b7bac0392 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error **errp) > char *loadparm_str; > > /* make a NUL-terminated string */ > - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); > - loadparm_str[sizeof(ms->loadparm)] = 0; > + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); > + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); I feel like g_strndup(ms->loadparm, sizeof(ms->loadparm)) is what should have been used here. It copies N characters, but allocates N+1 adding a trailing NUL which are the semantic we wanted here. Regards, Daniel
On Wed, 29 Jul 2020 15:02:22 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) > reads one past of the end of ms->loadparm, so g_memdup() can not be used > here. > > Let's use malloc and memcpy instead! Hm, an alternative would be to use g_strndup(). What do you think? > > Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter") > Fixes: Coverity CID 1431058 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 403d30e13b..8b7bac0392 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error **errp) > char *loadparm_str; > > /* make a NUL-terminated string */ > - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); > - loadparm_str[sizeof(ms->loadparm)] = 0; > + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); > + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); > return loadparm_str; > } > > > base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a
On Thu, 30 Jul 2020 12:26:56 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, 29 Jul 2020 15:02:22 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > > > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) > > reads one past of the end of ms->loadparm, so g_memdup() can not be used > > here. > > > > Let's use malloc and memcpy instead! > > Hm, an alternative would be to use g_strndup(). What do you think? Sure. It is more concise and does exactly what we want. I'm not too familiar with the string utility funcitons of glib, so it didn't jup at me. Shall I spin a v2? Halil > > > > > Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter") > > Fixes: Coverity CID 1431058 > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > --- > > hw/s390x/s390-virtio-ccw.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > > index 403d30e13b..8b7bac0392 100644 > > --- a/hw/s390x/s390-virtio-ccw.c > > +++ b/hw/s390x/s390-virtio-ccw.c > > @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error **errp) > > char *loadparm_str; > > > > /* make a NUL-terminated string */ > > - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); > > - loadparm_str[sizeof(ms->loadparm)] = 0; > > + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); > > + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); > > return loadparm_str; > > } > > > > > > base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a > >
On Thu, 30 Jul 2020 11:25:06 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > > /* make a NUL-terminated string */ > > - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); > > - loadparm_str[sizeof(ms->loadparm)] = 0; > > + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); > > + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); > > I feel like g_strndup(ms->loadparm, sizeof(ms->loadparm)) > is what should have been used here. > > It copies N characters, but allocates N+1 adding a trailing NUL > which are the semantic we wanted here. I agree. Thanks for pointing this out. I'm not very familiar with the string utilities of glib. Regards, Halil
On Thu, 30 Jul 2020 13:25:21 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Thu, 30 Jul 2020 12:26:56 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Wed, 29 Jul 2020 15:02:22 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) > > > reads one past of the end of ms->loadparm, so g_memdup() can not be used > > > here. > > > > > > Let's use malloc and memcpy instead! > > > > Hm, an alternative would be to use g_strndup(). What do you think? > > Sure. It is more concise and does exactly what we want. I'm not too > familiar with the string utility funcitons of glib, so it didn't jup > at me. > > Shall I spin a v2? Sounds good.
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 403d30e13b..8b7bac0392 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error **errp) char *loadparm_str; /* make a NUL-terminated string */ - loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1); - loadparm_str[sizeof(ms->loadparm)] = 0; + loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1); + memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm)); return loadparm_str; }
As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1) reads one past of the end of ms->loadparm, so g_memdup() can not be used here. Let's use malloc and memcpy instead! Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter") Fixes: Coverity CID 1431058 Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a