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

[Christian Borntraeger]

On 11.01.21 14:17, Miroslav Rezanina wrote:
> 
> 
> ----- 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.

Fair enough. But that actually means that the compiler warning is wrong, because
we use strncpy exactly in the way as described (allowing for the final \n to be
missing). But let us use strpadcpy then.