[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter
From: |
Peter Maydell |
Subject: |
Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter |
Date: |
Tue, 28 Jul 2020 14:52:36 +0100 |
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