| Message ID | 20200727140522.251815-3-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/2] s390x/protvirt: allow to IPL secure guests with -no-reboot | expand |
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
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?
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
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)