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

[Thomas Huth]

On 11/01/2021 13.42, Miroslav Rezanina wrote:
> ----- Original Message -----
> > From: "Thomas Huth" <thuth@redhat.com>
> > To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com, 
> > qemu-devel@nongnu.org, "qemu-s390x"
> > <qemu-s390x@nongnu.org>
> > Sent: Monday, January 11, 2021 1:24:57 PM
> > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> >
> > On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
> > > 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?
> >
> > Yes, strpadcpy is the better way here - this field has to be padded with
> > zeroes, so doing "- 1" is wrong here.
>
> Hi Thomas,
>
> as I wrote in reply to Phillipe - the array is memset to zeroes before the if 
> so we
> are sure it's padded with zeroes (in this occurrence, not true for second one).

Ok, but dropping the last character is still wrong here. The ext_names do 
not need to be terminated with a \0 if they have the full length.

 Thomas