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

[Christian Borntraeger]

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.