some more s390x fixes

[PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter

Posted by Cornelia Huck 5 years, 6 months ago

From: Halil Pasic <pasic@linux.ibm.com>

The function machine_get_loadparm() is supposed to produce a C-string,
that is a NUL-terminated one, but it does not. ElectricFence can detect
this problem if the loadparm machine property is used.

Let us make the returned string a NUL-terminated one.

Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine")
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20200723162717.88485-1-pasic@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8cc2f25d8a6a..403d30e13bca 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -701,8 +701,12 @@ bool hpage_1m_allowed(void)
 static char *machine_get_loadparm(Object *obj, Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
+    char *loadparm_str;
 
-    return g_memdup(ms->loadparm, sizeof(ms->loadparm));
+    /* make a NUL-terminated string */
+    loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
+    loadparm_str[sizeof(ms->loadparm)] = 0;
+    return loadparm_str;
 }
 
 static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
-- 
2.25.4

Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter

Posted by Peter Maydell 5 years, 6 months ago

On Mon, 27 Jul 2020 at 15:05, Cornelia Huck <cohuck@redhat.com> wrote:
>
> From: Halil Pasic <pasic@linux.ibm.com>
>
> The function machine_get_loadparm() is supposed to produce a C-string,
> that is a NUL-terminated one, but it does not. ElectricFence can detect
> this problem if the loadparm machine property is used.
>
> Let us make the returned string a NUL-terminated one.
>
> Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine")
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20200723162717.88485-1-pasic@linux.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8cc2f25d8a6a..403d30e13bca 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -701,8 +701,12 @@ bool hpage_1m_allowed(void)
>  static char *machine_get_loadparm(Object *obj, Error **errp)
>  {
>      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> +    char *loadparm_str;
>
> -    return g_memdup(ms->loadparm, sizeof(ms->loadparm));
> +    /* make a NUL-terminated string */
> +    loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> +    loadparm_str[sizeof(ms->loadparm)] = 0;
> +    return loadparm_str;

Hi. Coverity points out (CID 1431058) that this code now
reads off the end of the ms->loadparm buffer, because
g_memdup() is going to read and copy 9 bytes (size + 1)
and the array itself is only 8 bytes.

I don't think you can use g_memdup() here -- you need to
allocate the memory with g_malloc() and then fill it with
memcpy(), something like:

    loadparm_str = g_malloc(sizeof(ms->loadparm) + 1);
    memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));
    loadparm_str[sizeof(ms->loadparm)] = 0;

thanks
-- PMM

Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter

Posted by Cornelia Huck 5 years, 6 months ago

On Tue, 28 Jul 2020 14:52:36 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 27 Jul 2020 at 15:05, Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > From: Halil Pasic <pasic@linux.ibm.com>
> >
> > The function machine_get_loadparm() is supposed to produce a C-string,
> > that is a NUL-terminated one, but it does not. ElectricFence can detect
> > this problem if the loadparm machine property is used.
> >
> > Let us make the returned string a NUL-terminated one.
> >
> > Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine")
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Message-Id: <20200723162717.88485-1-pasic@linux.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 8cc2f25d8a6a..403d30e13bca 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -701,8 +701,12 @@ bool hpage_1m_allowed(void)
> >  static char *machine_get_loadparm(Object *obj, Error **errp)
> >  {
> >      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> > +    char *loadparm_str;
> >
> > -    return g_memdup(ms->loadparm, sizeof(ms->loadparm));
> > +    /* make a NUL-terminated string */
> > +    loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> > +    loadparm_str[sizeof(ms->loadparm)] = 0;
> > +    return loadparm_str;  
> 
> Hi. Coverity points out (CID 1431058) that this code now
> reads off the end of the ms->loadparm buffer, because
> g_memdup() is going to read and copy 9 bytes (size + 1)
> and the array itself is only 8 bytes.
> 
> I don't think you can use g_memdup() here -- you need to
> allocate the memory with g_malloc() and then fill it with
> memcpy(), something like:
> 
>     loadparm_str = g_malloc(sizeof(ms->loadparm) + 1);
>     memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));
>     loadparm_str[sizeof(ms->loadparm)] = 0;

Sigh.

Halil, do you have time to cook up a patch?

Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter

Posted by Halil Pasic 5 years, 6 months ago

On Tue, 28 Jul 2020 17:14:38 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 28 Jul 2020 14:52:36 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On Mon, 27 Jul 2020 at 15:05, Cornelia Huck <cohuck@redhat.com> wrote:
> > >
> > > From: Halil Pasic <pasic@linux.ibm.com>
> > >
> > > The function machine_get_loadparm() is supposed to produce a C-string,
> > > that is a NUL-terminated one, but it does not. ElectricFence can detect
> > > this problem if the loadparm machine property is used.
> > >
> > > Let us make the returned string a NUL-terminated one.
> > >
> > > Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine")
> > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > > Message-Id: <20200723162717.88485-1-pasic@linux.ibm.com>
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > ---
> > >  hw/s390x/s390-virtio-ccw.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > index 8cc2f25d8a6a..403d30e13bca 100644
> > > --- a/hw/s390x/s390-virtio-ccw.c
> > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > @@ -701,8 +701,12 @@ bool hpage_1m_allowed(void)
> > >  static char *machine_get_loadparm(Object *obj, Error **errp)
> > >  {
> > >      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> > > +    char *loadparm_str;
> > >
> > > -    return g_memdup(ms->loadparm, sizeof(ms->loadparm));
> > > +    /* make a NUL-terminated string */
> > > +    loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> > > +    loadparm_str[sizeof(ms->loadparm)] = 0;
> > > +    return loadparm_str;  
> > 
> > Hi. Coverity points out (CID 1431058) that this code now
> > reads off the end of the ms->loadparm buffer, because
> > g_memdup() is going to read and copy 9 bytes (size + 1)
> > and the array itself is only 8 bytes.
> > 
> > I don't think you can use g_memdup() here -- you need to
> > allocate the memory with g_malloc() and then fill it with
> > memcpy(), something like:
> > 
> >     loadparm_str = g_malloc(sizeof(ms->loadparm) + 1);
> >     memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));
> >     loadparm_str[sizeof(ms->loadparm)] = 0;
> 
> Sigh.
> 
> Halil, do you have time to cook up a patch?
> 
> 

Will do!

Halil