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

[Miroslav Rezanina <mrezanin@redhat.com>]

----- Original Message -----
> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
> To: "Thomas Huth" <thuth@redhat.com>, "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: "qemu-s390x" <qemu-s390x@nongnu.org>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
> Sent: Monday, January 11, 2021 2:02:32 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> 
> 
> 
> On 11.01.21 13:54, Thomas Huth wrote:
> > 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.
> The current code is actually correct. We are perfectly fine without the final
> \n if the string is really 256 bytes.
> 
> Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it
> necessary? No.

Yes, it is necessary because otherwise compiler (GCC 11) produce warning and so
build fail when --enable-werror is used.

Mirek


> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer