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

[Christian Borntraeger]

On 11.01.21 14:02, Christian Borntraeger wrote:
> 
> 
> 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.

And yes, Thomas is right. The -1 is wrong.