diff mbox series

powerpc: Fix VSX register number on __strncpy_power9 [BZ #29197]

Message ID 20220607143019.135154-1-msc@linux.ibm.com
State New
Headers show
Series powerpc: Fix VSX register number on __strncpy_power9 [BZ #29197] | expand

Commit Message

Matheus Castanho June 7, 2022, 2:30 p.m. UTC 
__strncpy_power9 initializes VR 18 with zeroes to be used throughout the
code, including when zero-padding the destination string. However, the
v18 reference was mistakenly being used for stxv and stxvl, which take a
VSX vector as operand. The code ended up using the uninitialized VSR 18
register by mistake.

Both occurrences have been changed to use the proper VSX number for VR 18
(i.e. VSR 50).

Tested on powerpc, powerpc64 and powerpc64le.

Suggested-by: Kewen Lin <linkw@gcc.gnu.org>
---
 sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Paul E Murphy June 7, 2022, 2:37 p.m. UTC | #1 
On 6/7/22 9:30 AM, Matheus Castanho via Libc-alpha wrote:
> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the
> code, including when zero-padding the destination string. However, the
> v18 reference was mistakenly being used for stxv and stxvl, which take a
> VSX vector as operand. The code ended up using the uninitialized VSR 18
> register by mistake.
> 
> Both occurrences have been changed to use the proper VSX number for VR 18
> (i.e. VSR 50).
> 
> Tested on powerpc, powerpc64 and powerpc64le.
> 
> Suggested-by: Kewen Lin <linkw@gcc.gnu.org>
> ---
>   sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> index ae23161316..deb94671cc 100644
> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> @@ -352,7 +352,7 @@ L(zero_padding_loop):
>   	cmpldi	cr6,r5,16	/* Check if length was reached.  */
>   	ble	cr6,L(zero_padding_end)
> 
> -	stxv	v18,0(r11)
> +	stxv	32+v18,0(r11)
>   	addi	r11,r11,16
>   	addi	r5,r5,-16
> 
> @@ -360,7 +360,7 @@ L(zero_padding_loop):
> 
>   L(zero_padding_end):
>   	sldi	r10,r5,56	/* stxvl wants size in top 8 bits  */
> -	stxvl	v18,r11,r10	/* Partial store  */
> +	stxvl	32+v18,r11,r10	/* Partial store  */
>   	blr
> 
>   	.align	4

LGTM, will you also backport this as needed too?

If this wasn't caught by a test-case, I think adding one would be 
desirable too.
Matheus Castanho June 7, 2022, 2:50 p.m. UTC | #2 
Paul E Murphy <murphyp@linux.ibm.com> writes:

> On 6/7/22 9:30 AM, Matheus Castanho via Libc-alpha wrote:
>> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the
>> code, including when zero-padding the destination string. However, the
>> v18 reference was mistakenly being used for stxv and stxvl, which take a
>> VSX vector as operand. The code ended up using the uninitialized VSR 18
>> register by mistake.
>> Both occurrences have been changed to use the proper VSX number for VR 18
>> (i.e. VSR 50).
>> Tested on powerpc, powerpc64 and powerpc64le.
>> Suggested-by: Kewen Lin <linkw@gcc.gnu.org>
>> ---
>>   sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>> b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>> index ae23161316..deb94671cc 100644
>> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>> @@ -352,7 +352,7 @@ L(zero_padding_loop):
>>   	cmpldi	cr6,r5,16	/* Check if length was reached.  */
>>   	ble	cr6,L(zero_padding_end)
>> -	stxv	v18,0(r11)
>> +	stxv	32+v18,0(r11)
>>   	addi	r11,r11,16
>>   	addi	r5,r5,-16
>> @@ -360,7 +360,7 @@ L(zero_padding_loop):
>>   L(zero_padding_end):
>>   	sldi	r10,r5,56	/* stxvl wants size in top 8 bits  */
>> -	stxvl	v18,r11,r10	/* Partial store  */
>> +	stxvl	32+v18,r11,r10	/* Partial store  */
>>   	blr
>>   	.align	4
>
> LGTM, will you also backport this as needed too?

Yes, that's the idea. Ideally to all affected releases.

> If this wasn't caught by a test-case, I think adding one would be desirable too.

string/test-strncpy.c already checks if the bytes beyond the terminating
NUL have been properly set [1]. But the issue is that vs18 is also zero
when the test starts, so even with the bug the string ends up in the
expected state.

To actually trigger the bug we would have to fill vs18 with something !=
NUL before calling __strncpy_power9 (as is done in the reproducer). But
since this is very Power-specific and the test is generic, I'm not sure
what's the best way forward.

--
Matheus Castanho
Carlos O'Donell June 7, 2022, 2:58 p.m. UTC | #3 
On 6/7/22 10:37, Paul E Murphy via Libc-alpha wrote:
> LGTM, will you also backport this as needed too?
> 
> If this wasn't caught by a test-case, I think adding one would be desirable too.
 
Please commit the fix first and we can add a test case later.

This particular class of bug is often tricky to test.

Given that you have ACK'd the bug please feel free to backport to all affected release branches.

I'm interested in backporting this fix downstream :-)
Siddhesh Poyarekar June 7, 2022, 3:04 p.m. UTC | #4 
On 07/06/2022 20:00, Matheus Castanho via Libc-alpha wrote:
> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the
> code, including when zero-padding the destination string. However, the
> v18 reference was mistakenly being used for stxv and stxvl, which take a
> VSX vector as operand. The code ended up using the uninitialized VSR 18
> register by mistake.
> 
> Both occurrences have been changed to use the proper VSX number for VR 18
> (i.e. VSR 50).
> 
> Tested on powerpc, powerpc64 and powerpc64le.
> 
> Suggested-by: Kewen Lin <linkw@gcc.gnu.org>

Signed-off-by is the right tag here given that it is pretty much the 
same patch that's in the bugzilla.

> ---
>   sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> index ae23161316..deb94671cc 100644
> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> @@ -352,7 +352,7 @@ L(zero_padding_loop):
>   	cmpldi	cr6,r5,16	/* Check if length was reached.  */
>   	ble	cr6,L(zero_padding_end)
>   
> -	stxv	v18,0(r11)
> +	stxv	32+v18,0(r11)
>   	addi	r11,r11,16
>   	addi	r5,r5,-16
>   
> @@ -360,7 +360,7 @@ L(zero_padding_loop):
>   
>   L(zero_padding_end):
>   	sldi	r10,r5,56	/* stxvl wants size in top 8 bits  */
> -	stxvl	v18,r11,r10	/* Partial store  */
> +	stxvl	32+v18,r11,r10	/* Partial store  */
>   	blr
>   
>   	.align	4
Paul E Murphy June 7, 2022, 3:06 p.m. UTC | #5 
On 6/7/22 9:50 AM, Matheus Castanho wrote:
> 
> Paul E Murphy <murphyp@linux.ibm.com> writes:
> 
>> On 6/7/22 9:30 AM, Matheus Castanho via Libc-alpha wrote:
>>> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the
>>> code, including when zero-padding the destination string. However, the
>>> v18 reference was mistakenly being used for stxv and stxvl, which take a
>>> VSX vector as operand. The code ended up using the uninitialized VSR 18
>>> register by mistake.
>>> Both occurrences have been changed to use the proper VSX number for VR 18
>>> (i.e. VSR 50).
>>> Tested on powerpc, powerpc64 and powerpc64le.
>>> Suggested-by: Kewen Lin <linkw@gcc.gnu.org>
>>> ---
>>>    sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>>> b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>>> index ae23161316..deb94671cc 100644
>>> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>>> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
>>> @@ -352,7 +352,7 @@ L(zero_padding_loop):
>>>    	cmpldi	cr6,r5,16	/* Check if length was reached.  */
>>>    	ble	cr6,L(zero_padding_end)
>>> -	stxv	v18,0(r11)
>>> +	stxv	32+v18,0(r11)
>>>    	addi	r11,r11,16
>>>    	addi	r5,r5,-16
>>> @@ -360,7 +360,7 @@ L(zero_padding_loop):
>>>    L(zero_padding_end):
>>>    	sldi	r10,r5,56	/* stxvl wants size in top 8 bits  */
>>> -	stxvl	v18,r11,r10	/* Partial store  */
>>> +	stxvl	32+v18,r11,r10	/* Partial store  */
>>>    	blr
>>>    	.align	4
>>
>> LGTM, will you also backport this as needed too?
> 
> Yes, that's the idea. Ideally to all affected releases.
> 
>> If this wasn't caught by a test-case, I think adding one would be desirable too.
> 
> string/test-strncpy.c already checks if the bytes beyond the terminating
> NUL have been properly set [1]. But the issue is that vs18 is also zero
> when the test starts, so even with the bug the string ends up in the
> expected state.
> 
> To actually trigger the bug we would have to fill vs18 with something !=
> NUL before calling __strncpy_power9 (as is done in the reproducer). But
> since this is very Power-specific and the test is generic, I'm not sure
> what's the best way forward.

Yes, agreed. This is a hard thing to catch.  As Carlos noted, please 
commit this; it is a fairly obvious fix (to me).  I don't have any 
practical ideas at the moment for randomizing unused caller-saved 
registers in this case.
Matheus Castanho June 7, 2022, 5:16 p.m. UTC | #6 
Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> On 07/06/2022 20:00, Matheus Castanho via Libc-alpha wrote:
>> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the
>> code, including when zero-padding the destination string. However, the
>> v18 reference was mistakenly being used for stxv and stxvl, which take a
>> VSX vector as operand. The code ended up using the uninitialized VSR 18
>> register by mistake.
>> Both occurrences have been changed to use the proper VSX number for VR 18
>> (i.e. VSR 50).
>> Tested on powerpc, powerpc64 and powerpc64le.
>> Suggested-by: Kewen Lin <linkw@gcc.gnu.org>
>
> Signed-off-by is the right tag here given that it is pretty much the same patch
> that's in the bugzilla.
>

Ack. I'll change it before pushing.

Thanks,
Matheus Castanho
Matheus Castanho June 7, 2022, 6:45 p.m. UTC | #7 
"Carlos O'Donell" <carlos@redhat.com> writes:

> On 6/7/22 10:37, Paul E Murphy via Libc-alpha wrote:
>> LGTM, will you also backport this as needed too?
>>
>> If this wasn't caught by a test-case, I think adding one would be desirable too.
>
> Please commit the fix first and we can add a test case later.
>
> This particular class of bug is often tricky to test.
>
> Given that you have ACK'd the bug please feel free to backport to all affected release branches.
>
> I'm interested in backporting this fix downstream :-)

Pushed and backported to all affected versions: 2.33, 2.34 and 2.35

--
Matheus Castanho
diff mbox series

Patch

diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
index ae23161316..deb94671cc 100644
--- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
+++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
@@ -352,7 +352,7 @@  L(zero_padding_loop):
 	cmpldi	cr6,r5,16	/* Check if length was reached.  */
 	ble	cr6,L(zero_padding_end)
 
-	stxv	v18,0(r11)
+	stxv	32+v18,0(r11)
 	addi	r11,r11,16
 	addi	r5,r5,-16
 
@@ -360,7 +360,7 @@  L(zero_padding_loop):
 
 L(zero_padding_end):
 	sldi	r10,r5,56	/* stxvl wants size in top 8 bits  */
-	stxvl	v18,r11,r10	/* Partial store  */
+	stxvl	32+v18,r11,r10	/* Partial store  */
 	blr
 
 	.align	4