: S390: Fix handling of needles crossing a page in strstr z15 ifunc-variant. [BZ #25226]
Commit Message
Hi,
if the specified needle crosses a page-boundary, the s390-z15 ifunc
variant of strstr truncates the needle which results in invalid results.
This is fixed by loading the needle beyond the page boundary to v18
instead of v16. The bug is sometimes observable in test-strstr.c in
check1() and check2() as the haystack and needle is stored on stack.
Thus the needle can be on a page boundary.
check2 is now extended to test haystack / needles located on stack, at
end of page and on two pages.
As soon as committed I'll backport it to glibc 2.30 release branch.
Bye
Stefan
Comments
* Stefan Liebler:
> if the specified needle crosses a page-boundary, the s390-z15 ifunc
> variant of strstr truncates the needle which results in invalid
> results.
>
> This is fixed by loading the needle beyond the page boundary to v18
> instead of v16. The bug is sometimes observable in test-strstr.c in
> check1() and check2() as the haystack and needle is stored on
> stack. Thus the needle can be on a page boundary.
(GNU style is “check1 and check2”, no parentheses.)
> check2 is now extended to test haystack / needles located on stack, at
> end of page and on two pages.
The test change looks okay to me. I haven't reviewed the assembler
change. I think you can commit that as the architecture maintainer.
We will need help from your team with testing the downstream fix. Do
you think we should fix this proactively in Red Hat Enterprise Linux 8.1
as well? It received the backport of the new strstr implementation,
too.
Thanks,
Florian
* Florian Weimer:
> * Stefan Liebler:
>
>> if the specified needle crosses a page-boundary, the s390-z15 ifunc
>> variant of strstr truncates the needle which results in invalid
>> results.
>>
>> This is fixed by loading the needle beyond the page boundary to v18
>> instead of v16. The bug is sometimes observable in test-strstr.c in
>> check1() and check2() as the haystack and needle is stored on
>> stack. Thus the needle can be on a page boundary.
>
> (GNU style is “check1 and check2”, no parentheses.)
>
>> check2 is now extended to test haystack / needles located on stack, at
>> end of page and on two pages.
>
> The test change looks okay to me. I haven't reviewed the assembler
> change. I think you can commit that as the architecture maintainer.
One more comment if it is not too late: It would be nice to reference
commit 6f47401bd5fc71209219779a0426170a9a7395b0 ("S390: Add arch13
strstr ifunc variant.") as the commit that introduced this bug.
Although it is very easy to track down in this case.
Thanks,
Florian
On 11/27/19 10:33 AM, Florian Weimer wrote:
> * Stefan Liebler:
>
>> if the specified needle crosses a page-boundary, the s390-z15 ifunc
>> variant of strstr truncates the needle which results in invalid
>> results.
>>
>> This is fixed by loading the needle beyond the page boundary to v18
>> instead of v16. The bug is sometimes observable in test-strstr.c in
>> check1() and check2() as the haystack and needle is stored on
>> stack. Thus the needle can be on a page boundary.
>
> (GNU style is “check1 and check2”, no parentheses.)
>
>> check2 is now extended to test haystack / needles located on stack, at
>> end of page and on two pages.
>
> The test change looks okay to me. I haven't reviewed the assembler
> change. I think you can commit that as the architecture maintainer.
>
> We will need help from your team with testing the downstream fix. Do
> you think we should fix this proactively in Red Hat Enterprise Linux 8.1
> as well? It received the backport of the new strstr implementation,
> too.
>
> Thanks,
> Florian
>
Committed it with “check1 and check2” and a reference to the commit
6f47401bd5fc71209219779a0426170a9a7395b0.
Regarding RHEL, I've recognized that you've opened a RHEL Bugzilla and
answered there.
Thanks,
Stefan
On 11/27/19 12:53 PM, Stefan Liebler wrote:
> On 11/27/19 10:33 AM, Florian Weimer wrote:
>> * Stefan Liebler:
>>
>>> if the specified needle crosses a page-boundary, the s390-z15 ifunc
>>> variant of strstr truncates the needle which results in invalid
>>> results.
>>>
>>> This is fixed by loading the needle beyond the page boundary to v18
>>> instead of v16. The bug is sometimes observable in test-strstr.c in
>>> check1() and check2() as the haystack and needle is stored on
>>> stack. Thus the needle can be on a page boundary.
>>
>> (GNU style is “check1 and check2”, no parentheses.)
>>
>>> check2 is now extended to test haystack / needles located on stack, at
>>> end of page and on two pages.
>>
>> The test change looks okay to me. I haven't reviewed the assembler
>> change. I think you can commit that as the architecture maintainer.
>>
>> We will need help from your team with testing the downstream fix. Do
>> you think we should fix this proactively in Red Hat Enterprise Linux 8.1
>> as well? It received the backport of the new strstr implementation,
>> too.
>>
>> Thanks,
>> Florian
>>
>
> Committed it with “check1 and check2” and a reference to the commit
> 6f47401bd5fc71209219779a0426170a9a7395b0.
>
> Regarding RHEL, I've recognized that you've opened a RHEL Bugzilla and
> answered there.
>
> Thanks,
> Stefan
>
I've just committed
https://sourceware.org/git/?p=glibc.git;a=commit;h=f4419b0d0181da28f18cba40068f2e75300f5020
commit 01bf86402c1bb4686c99b8752e5d2183ff365df6
Author: Stefan Liebler <stli@linux.ibm.com>
Date: Wed Nov 27 09:00:01 2019 +0100
S390: Fix handling of needles crossing a page in strstr z15 ifunc-variant. [BZ #25226]
If the specified needle crosses a page-boundary, the s390-z15 ifunc variant of
strstr truncates the needle which results in invalid results.
This is fixed by loading the needle beyond the page boundary to v18 instead of v16.
The bug is sometimes observable in test-strstr.c in check1() and check2() as the
haystack and needle is stored on stack. Thus the needle can be on a page boundary.
check2 is now extended to test haystack / needles located on stack, at end of page
and on two pages.
@@ -139,16 +139,45 @@ check1 (void)
static void
check2 (void)
{
- const char s1[] = ", enable_static, \0, enable_shared, ";
+ const char s1_stack[] = ", enable_static, \0, enable_shared, ";
+ const size_t s1_byte_count = 18;
+ const char *s2_stack = &(s1_stack[s1_byte_count]);
+ const size_t s2_byte_count = 18;
char *exp_result;
- char *s2 = (void *) buf1 + page_size - 18;
+ const size_t page_size_real = getpagesize ();
- strcpy (s2, s1);
- exp_result = stupid_strstr (s1, s1 + 18);
+ /* Haystack at end of page. The following page is protected. */
+ char *s1_page_end = (void *) buf1 + page_size - s1_byte_count;
+ strcpy (s1_page_end, s1_stack);
+
+ /* Haystack which crosses a page boundary.
+ Note: page_size is at least 2 * getpagesize. See test_init. */
+ char *s1_page_cross = (void *) buf1 + page_size_real - 8;
+ strcpy (s1_page_cross, s1_stack);
+
+ /* Needle at end of page. The following page is protected. */
+ char *s2_page_end = (void *) buf2 + page_size - s2_byte_count;
+ strcpy (s2_page_end, s2_stack);
+
+ /* Needle which crosses a page boundary.
+ Note: page_size is at least 2 * getpagesize. See test_init. */
+ char *s2_page_cross = (void *) buf2 + page_size_real - 8;
+ strcpy (s2_page_cross, s2_stack);
+
+ exp_result = stupid_strstr (s1_stack, s2_stack);
FOR_EACH_IMPL (impl, 0)
{
- check_result (impl, s1, s1 + 18, exp_result);
- check_result (impl, s2, s1 + 18, exp_result);
+ check_result (impl, s1_stack, s2_stack, exp_result);
+ check_result (impl, s1_stack, s2_page_end, exp_result);
+ check_result (impl, s1_stack, s2_page_cross, exp_result);
+
+ check_result (impl, s1_page_end, s2_stack, exp_result);
+ check_result (impl, s1_page_end, s2_page_end, exp_result);
+ check_result (impl, s1_page_end, s2_page_cross, exp_result);
+
+ check_result (impl, s1_page_cross, s2_stack, exp_result);
+ check_result (impl, s1_page_cross, s2_page_end, exp_result);
+ check_result (impl, s1_page_cross, s2_page_cross, exp_result);
}
}
@@ -164,7 +164,7 @@ ENTRY(STRSTR_ARCH13)
vfenezb %v19,%v18,%v18 /* Search zero in loaded needle bytes. */
veclb %v19,%v21 /* Zero index <= max loaded byte index? */
jle .Lneedle_loaded /* -> v18 contains full needle. */
- vl %v16,0(%r3) /* Load needle beyond page boundary. */
+ vl %v18,0(%r3) /* Load needle beyond page boundary. */
vfenezb %v19,%v18,%v18
j .Lneedle_loaded
END(STRSTR_ARCH13)