* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-29 13:02 [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter Halil Pasic
@ 2020-07-29 17:09 ` Cornelia Huck
2020-07-29 17:12 ` Peter Maydell
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2020-07-29 17:09 UTC (permalink / raw)
To: Halil Pasic
Cc: Peter Maydell, Thomas Huth, David Hildenbrand, qemu-devel,
Christian Borntraeger, qemu-s390x, Richard Henderson
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-29 13:02 [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter Halil Pasic
2020-07-29 17:09 ` Cornelia Huck
@ 2020-07-29 17:12 ` Peter Maydell
2020-07-30 10:25 ` Daniel P. Berrangé
2020-07-30 10:26 ` Cornelia Huck
3 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2020-07-29 17:12 UTC (permalink / raw)
To: Halil Pasic
Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers,
Christian Borntraeger, qemu-s390x, Richard Henderson
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-29 13:02 [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter Halil Pasic
2020-07-29 17:09 ` Cornelia Huck
2020-07-29 17:12 ` Peter Maydell
@ 2020-07-30 10:25 ` Daniel P. Berrangé
2020-07-30 11:28 ` Halil Pasic
2020-07-30 10:26 ` Cornelia Huck
3 siblings, 1 reply; 8+ messages in thread
From: Daniel P. Berrangé @ 2020-07-30 10:25 UTC (permalink / raw)
To: Halil Pasic
Cc: Peter Maydell, Thomas Huth, David Hildenbrand, Cornelia Huck,
qemu-devel, Christian Borntraeger, qemu-s390x, Richard Henderson
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-30 10:25 ` Daniel P. Berrangé
@ 2020-07-30 11:28 ` Halil Pasic
0 siblings, 0 replies; 8+ messages in thread
From: Halil Pasic @ 2020-07-30 11:28 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Maydell, Thomas Huth, David Hildenbrand, Cornelia Huck,
qemu-devel, Christian Borntraeger, qemu-s390x, Richard Henderson
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-29 13:02 [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter Halil Pasic
` (2 preceding siblings ...)
2020-07-30 10:25 ` Daniel P. Berrangé
@ 2020-07-30 10:26 ` Cornelia Huck
2020-07-30 11:25 ` Halil Pasic
3 siblings, 1 reply; 8+ messages in thread
From: Cornelia Huck @ 2020-07-30 10:26 UTC (permalink / raw)
To: Halil Pasic
Cc: Peter Maydell, Thomas Huth, David Hildenbrand, qemu-devel,
Christian Borntraeger, qemu-s390x, Richard Henderson
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-30 10:26 ` Cornelia Huck
@ 2020-07-30 11:25 ` Halil Pasic
2020-07-30 11:29 ` Cornelia Huck
0 siblings, 1 reply; 8+ messages in thread
From: Halil Pasic @ 2020-07-30 11:25 UTC (permalink / raw)
To: Cornelia Huck
Cc: Peter Maydell, Thomas Huth, David Hildenbrand, qemu-devel,
Christian Borntraeger, qemu-s390x, Richard Henderson
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter
2020-07-30 11:25 ` Halil Pasic
@ 2020-07-30 11:29 ` Cornelia Huck
0 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2020-07-30 11:29 UTC (permalink / raw)
To: Halil Pasic
Cc: Peter Maydell, Thomas Huth, David Hildenbrand, qemu-devel,
Christian Borntraeger, qemu-s390x, Richard Henderson
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.
^ permalink raw reply [flat|nested] 8+ messages in thread