Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length

[Miroslav Rezanina <mrezanin@redhat.com>]

----- Original Message -----
> From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> To: mrezanin@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, mrezanin@redhat.com wrote:
> > From: Miroslav Rezanina <mrezanin@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 <mrezanin@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