[PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Halil Pasic]

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
-- 
2.17.1

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Cornelia Huck]

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.

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Peter Maydell]

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

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Daniel P. Berrangé]

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 :|

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Cornelia Huck]

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

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Halil Pasic]

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

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Halil Pasic]

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

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Cornelia Huck]

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.