* [PULL 1/2] s390x/protvirt: allow to IPL secure guests with -no-reboot
2020-07-27 14:05 [PULL 0/2] some more s390x fixes Cornelia Huck
@ 2020-07-27 14:05 ` Cornelia Huck
2020-07-27 14:05 ` [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter Cornelia Huck
2020-07-27 19:59 ` [PULL 0/2] some more s390x fixes Peter Maydell
2 siblings, 0 replies; 7+ messages in thread
From: Cornelia Huck @ 2020-07-27 14:05 UTC (permalink / raw)
To: Peter Maydell
Cc: Janosch Frank, David Hildenbrand, Cornelia Huck, qemu-devel,
Christian Borntraeger, qemu-s390x, Viktor Mihajlovski
From: Christian Borntraeger <borntraeger@de.ibm.com>
Right now, -no-reboot prevents secure guests from running. This is
correct from an implementation point of view, as we have modeled the
transition from non-secure to secure as a program directed IPL. From
a user perspective, this is not the behavior of least surprise.
We should implement the IPL into protected mode similar to the
functions that we use for kdump/kexec. In other words, we do not stop
here when -no-reboot is specified on the command line. Like function 0
or function 1, function 10 is not a classic reboot. For example, it
can only be called once. Before calling it a second time, a real
reboot/reset must happen in-between. So function code 10 is more or
less a state transition reset, but not a "standard" reset or reboot.
Fixes: 4d226deafc44 ("s390x: protvirt: Support unpack facility")
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
Message-Id: <20200721103202.30610-1-borntraeger@de.ibm.com>
[CH: tweaked description]
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
hw/s390x/ipl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index d46b1f094f75..3d2652d75abd 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -630,7 +630,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
}
}
if (reset_type == S390_RESET_MODIFIED_CLEAR ||
- reset_type == S390_RESET_LOAD_NORMAL) {
+ reset_type == S390_RESET_LOAD_NORMAL ||
+ reset_type == S390_RESET_PV) {
/* ignore -no-reboot, send no event */
qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
} else {
--
2.25.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter
2020-07-27 14:05 [PULL 0/2] some more s390x fixes Cornelia Huck
2020-07-27 14:05 ` [PULL 1/2] s390x/protvirt: allow to IPL secure guests with -no-reboot Cornelia Huck
@ 2020-07-27 14:05 ` Cornelia Huck
2020-07-28 13:52 ` Peter Maydell
2020-07-27 19:59 ` [PULL 0/2] some more s390x fixes Peter Maydell
2 siblings, 1 reply; 7+ messages in thread
From: Cornelia Huck @ 2020-07-27 14:05 UTC (permalink / raw)
To: Peter Maydell
Cc: Halil Pasic, qemu-s390x, Cornelia Huck, qemu-devel, Thomas Huth
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter
2020-07-27 14:05 ` [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter Cornelia Huck
@ 2020-07-28 13:52 ` Peter Maydell
2020-07-28 15:14 ` Cornelia Huck
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-07-28 13:52 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Halil Pasic, qemu-s390x, QEMU Developers, Thomas Huth
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter
2020-07-28 13:52 ` Peter Maydell
@ 2020-07-28 15:14 ` Cornelia Huck
2020-07-28 20:22 ` Halil Pasic
0 siblings, 1 reply; 7+ messages in thread
From: Cornelia Huck @ 2020-07-28 15:14 UTC (permalink / raw)
To: Peter Maydell, Halil Pasic; +Cc: Thomas Huth, qemu-s390x, QEMU Developers
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter
2020-07-28 15:14 ` Cornelia Huck
@ 2020-07-28 20:22 ` Halil Pasic
0 siblings, 0 replies; 7+ messages in thread
From: Halil Pasic @ 2020-07-28 20:22 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Peter Maydell, Thomas Huth, qemu-s390x, QEMU Developers
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PULL 0/2] some more s390x fixes
2020-07-27 14:05 [PULL 0/2] some more s390x fixes Cornelia Huck
2020-07-27 14:05 ` [PULL 1/2] s390x/protvirt: allow to IPL secure guests with -no-reboot Cornelia Huck
2020-07-27 14:05 ` [PULL 2/2] s390x/s390-virtio-ccw: fix loadparm property getter Cornelia Huck
@ 2020-07-27 19:59 ` Peter Maydell
2 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-07-27 19:59 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-s390x, QEMU Developers
On Mon, 27 Jul 2020 at 15:05, Cornelia Huck <cohuck@redhat.com> wrote:
>
> The following changes since commit 8ffa52c20d5693d454f65f2024a1494edfea65d4:
>
> Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2020-07-23 13:38:21 +0100)
>
> are available in the Git repository at:
>
> https://github.com/cohuck/qemu tags/s390x-20200727
>
> for you to fetch changes up to d6645483285feaa0aa26fe2b0c3bac6989250d2f:
>
> s390x/s390-virtio-ccw: fix loadparm property getter (2020-07-24 08:49:53 +0200)
>
> ----------------------------------------------------------------
> fixes for protected virtualization and loadparm handling
>
> ----------------------------------------------------------------
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread