| Message ID | e1ad733af7b23929456d05aacae693ce6462d4b3.1610364304.git.mrezanin@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RHEL7,qemu-kvm,1/3] Fix net.c warning on GCC 11 | expand |
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? strpadcpy((char *)sysib.sysib_322.ext_names[0], sizeof(sysib.sysib_322.ext_names[0]), qemu_name ?: "KVMguest", '\0'); > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > index 58dbc023eb..7c478b9e58 100644 > --- a/target/s390x/misc_helper.c > +++ b/target/s390x/misc_helper.c > @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, uint64_t r0, uint64_t r1) > ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name, > MIN(sizeof(sysib.sysib_322.vm[0].name), > strlen(qemu_name))); > + memset((char *)sysib.sysib_322.ext_names[0], 0, > + sizeof(sysib.sysib_322.ext_names[0])); > strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name, > - sizeof(sysib.sysib_322.ext_names[0])); > + sizeof(sysib.sysib_322.ext_names[0]) - 1); And here: strpadcpy((char *)sysib.sysib_322.ext_names[0], sizeof(sysib.sysib_322.ext_names[0]), qemu_name, '\0'); > } else { > ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8); > strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest"); >
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. Thomas
----- Original Message ----- > From: "Philippe Mathieu-Daudé" <philmd@redhat.com> > To: mrezanin@redhat.com, qemu-devel@nongnu.org > Sent: Monday, January 11, 2021 1:10:07 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > > 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? > > strpadcpy((char *)sysib.sysib_322.ext_names[0], > sizeof(sysib.sysib_322.ext_names[0]), > qemu_name ?: "KVMguest", '\0'); Hi Philippe, I went with -1 here because code use memset to 0 few lines above this code, so we now there are only zeroes in the array and we do not have to write them again. > > > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > > index 58dbc023eb..7c478b9e58 100644 > > --- a/target/s390x/misc_helper.c > > +++ b/target/s390x/misc_helper.c > > @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, > > uint64_t r0, uint64_t r1) > > ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name, > > MIN(sizeof(sysib.sysib_322.vm[0].name), > > strlen(qemu_name))); > > + memset((char *)sysib.sysib_322.ext_names[0], 0, > > + sizeof(sysib.sysib_322.ext_names[0])); > > strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name, > > - sizeof(sysib.sysib_322.ext_names[0])); > > + sizeof(sysib.sysib_322.ext_names[0]) - 1); > > And here: > > strpadcpy((char *)sysib.sysib_322.ext_names[0], > sizeof(sysib.sysib_322.ext_names[0]), > qemu_name, '\0'); However, here we are adding memset so using strpadcpy is better choice to ensure we have \0 after name string. Mirek > > > } else { > > ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8); > > strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest"); > > > >
----- 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). Mirek > > Thomas >
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
----- Original Message ----- > From: "Thomas Huth" <thuth@redhat.com> > To: "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 1:54:06 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > > 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. Oh, they do not have to end with \0??? I did not know it. In that case we probably have to use strpadcpy to get rid of the compiler warning. Mirek > > Thomas >
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.
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.
----- 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 > >
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.
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"); } diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index 58dbc023eb..7c478b9e58 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, uint64_t r0, uint64_t r1) ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name, MIN(sizeof(sysib.sysib_322.vm[0].name), strlen(qemu_name))); + memset((char *)sysib.sysib_322.ext_names[0], 0, + sizeof(sysib.sysib_322.ext_names[0])); strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name, - sizeof(sysib.sysib_322.ext_names[0])); + sizeof(sysib.sysib_322.ext_names[0]) - 1); } else { ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8); strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest");