----- Original Message -----
> From: "Philippe Mathieu-Daudé" <phi...@redhat.com>
> To: mreza...@redhat.com, qemu-devel@nongnu.org
> Sent: Monday, January 11, 2021 1:10:07 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>
> Hi Miroslav,
>
> On 1/11/21 12:30 PM, mreza...@redhat.com wrote:
> > From: Miroslav Rezanina <mreza...@redhat.com>
> >
> > There are two cases when vm name is copied but closing \0 can be lost
> > in case name is too long (>=256 characters).
> >
> > Updating length to copy so there is space for closing \0.
> >
> > Signed-off-by: Miroslav Rezanina <mreza...@redhat.com>
> > ---
> > target/s390x/kvm.c | 2 +-
> > target/s390x/misc_helper.c | 4 +++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index b8385e6b95..2313b5727e 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
> > addr, uint8_t ar)
> > */
> > if (qemu_name) {
> > strncpy((char *)sysib.ext_names[0], qemu_name,
> > - sizeof(sysib.ext_names[0]));
> > + sizeof(sysib.ext_names[0]) - 1);
> > } else {
> > strcpy((char *)sysib.ext_names[0], "KVMguest");
> > }
>
> What about using strpadcpy() instead?
>
> strpadcpy((char *)sysib.sysib_322.ext_names[0],
> sizeof(sysib.sysib_322.ext_names[0]),
> qemu_name ?: "KVMguest", '\0');
Hi Philippe,
I went with -1 here because code use memset to 0 few lines above this code, so
we now there are only zeroes in the array and we do not have to write them
again.
>
> > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> > index 58dbc023eb..7c478b9e58 100644
> > --- a/target/s390x/misc_helper.c
> > +++ b/target/s390x/misc_helper.c
> > @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
> > uint64_t r0, uint64_t r1)
> > ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name,
> > MIN(sizeof(sysib.sysib_322.vm[0].name),
> > strlen(qemu_name)));
> > + memset((char *)sysib.sysib_322.ext_names[0], 0,
> > + sizeof(sysib.sysib_322.ext_names[0]));
> > strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name,
> > - sizeof(sysib.sysib_322.ext_names[0]));
> > + sizeof(sysib.sysib_322.ext_names[0]) - 1);
>
> And here:
>
> strpadcpy((char *)sysib.sysib_322.ext_names[0],
> sizeof(sysib.sysib_322.ext_names[0]),
> qemu_name, '\0');
However, here we are adding memset so using strpadcpy is better choice to
ensure we have \0 after name string.
Mirek
>
> > } else {
> > ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8);
> > strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest");
> >
>
>
--
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer
