diff mbox series

elf/tst-big-note: Improve accuracy of test [BZ #20419]

Message ID 878sxyy8id.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series elf/tst-big-note: Improve accuracy of test [BZ #20419] | expand

Commit Message

Florian Weimer March 1, 2019, 2:27 p.m. UTC 
It is possible that the link editor injects an allocated ABI tag note
before the artificial, allocated large note in the test.  Note parsing
in open_verify stops when the first ABI tag note is encountered, so if
the ABI tag note comes first, the problematic code is not actually
exercised.

Also tweak the artificial note so that it is a syntactically valid
4-byte aligned note, in case the link editor tries to parse notes and
process them.

Improves the testing part of commit 0065aaaaae51cd60210ec3a7e13.

Tested on ppc64le, including that the test now crashes as expected on a
glibc-2.17-derived glibc without the backport of the original fix.

2019-03-01  Florian Weimer  <fweimer@redhat.com>

	[BZ #20419]
	* elf/tst-big-note-lib.S: Create a syntactically valid note.
	* elf/Makefile (tst-big-note-lib.so): Do not link with startup
	code, to avoid creating an ABI tag note.

Comments

Paul Pluzhnikov March 1, 2019, 4:18 p.m. UTC | #1 
On Fri, Mar 1, 2019 at 6:28 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> It is possible that the link editor injects an allocated ABI tag note
> before the artificial, allocated large note in the test.

Looks good to me. Thanks!

> +$(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
> +       $(LINK.o) -shared -o $@ $(LDFLAGS.so) $<

I got lost tracing through Makefile machinery trying to find how
-nostartfiles gets into the link line here, but I'll take your word
for it :-)
Florian Weimer March 1, 2019, 4:41 p.m. UTC | #2 
* Paul Pluzhnikov:

> On Fri, Mar 1, 2019 at 6:28 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> It is possible that the link editor injects an allocated ABI tag note
>> before the artificial, allocated large note in the test.
>
> Looks good to me. Thanks!
>
>> +$(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
>> +       $(LINK.o) -shared -o $@ $(LDFLAGS.so) $<
>
> I got lost tracing through Makefile machinery trying to find how
> -nostartfiles gets into the link line here, but I'll take your word
> for it :-)

I get this command (slightly 

gcc -m32 -nostdlib -nostartfiles -o /BUILD/elf/tst-big-note
  -Wl,-z,combreloc -Wl,-z,relro -Wl,--hash-style=both /BUILD/csu/crt1.o
  /BUILD/csu/crti.o `gcc -m32 --print-file-name=crtbegin.o`
  /BUILD/elf/tst-big-note.o /BUILD/support/libsupport_nonshared.a
  /BUILD/elf/tst-big-note-lib.so -Wl,-dynamic-linker=/lib/ld-linux.so.2
  -Wl,-rpath-link=/BUILD:/BUILD/math:/BUILD/elf:/BUILD/dlfcn:/BUILD/nss:/BUILD/nis:/BUILD/rt:/BUILD/resolv:/BUILD/mathvec:/BUILD/support:/BUILD/crypt:/BUILD/nptl
  /BUILD/libc.so.6 /BUILD/libc_nonshared.a -Wl,--as-needed
  /BUILD/elf/ld.so -Wl,--no-as-needed -lgcc -Wl,--as-needed -lgcc_s
  -Wl,--no-as-needed `gcc -m32 --print-file-name=crtend.o`
  /BUILD/csu/crtn.o

But looking at the build output, I saw new warnings:

Makefile:1522: warning: overriding recipe for target '/home/fweimer/src/gnu/glib
c/build/elf/tst-big-note-lib.so'
../Makerules:769: warning: ignoring old recipe for target
'/home/fweimer/src/gnu/glibc/build/elf/tst-big-note-lib.so'

I missed one further adjustment in the filtmod1 case I needed to mirror,
but with the new patch, the warning is gone.

Thanks,
Florian

elf/tst-big-note: Improve accuracy of test [BZ #20419]

It is possible that the link editor injects an allocated ABI tag note
before the artificial, allocated large note in the test.  Note parsing
in open_verify stops when the first ABI tag note is encountered, so if
the ABI tag note comes first, the problematic code is not actually
exercised.

Also tweak the artificial note so that it is a syntactically valid
4-byte aligned note, in case the link editor tries to parse notes and
process them.

Improves the testing part of commit 0065aaaaae51cd60210ec3a7e13.

2019-03-01  Florian Weimer  <fweimer@redhat.com>

	[BZ #20419]
	* elf/tst-big-note-lib.S: Create a syntactically valid note.
	* elf/Makefile (tst-big-note-lib.so): Do not link with startup
	code, to avoid creating an ABI tag note.
	(modules-names-nobuild): Add tst-big-note-lib.

diff --git a/elf/Makefile b/elf/Makefile
index 55204073a3..310a37cc13 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -316,8 +316,8 @@ endif
 modules-execstack-yes = tst-execstack-mod
 extra-test-objs += $(addsuffix .os,$(strip $(modules-names)))
 
-# filtmod1.so has a special rule
-modules-names-nobuild := filtmod1
+# filtmod1.so, tst-big-note-lib.so have special rules.
+modules-names-nobuild := filtmod1 tst-big-note-lib
 
 tests += $(tests-static)
 
@@ -1515,6 +1515,11 @@ tst-libc_dlvsym-static-ENV = \
 $(objpfx)tst-libc_dlvsym-static.out: $(objpfx)tst-libc_dlvsym-dso.so
 
 $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
+# Avoid creating an ABI tag note, which may come before the
+# artificial, large note in tst-big-note-lib.o and invalidate the
+# test.
+$(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
+	$(LINK.o) -shared -o $@ $(LDFLAGS.so) $<
 
 $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
 
diff --git a/elf/tst-big-note-lib.S b/elf/tst-big-note-lib.S
index e2008cf4ae..721686fa0e 100644
--- a/elf/tst-big-note-lib.S
+++ b/elf/tst-big-note-lib.S
@@ -20,7 +20,13 @@
    On a typical Linux system with 8MiB "ulimit -s", that was enough
    to trigger stack overflow in open_verify.  */
 
+#define NOTE_SIZE 8*1024*1024
+
 .pushsection .note.big,"a"
-.balign 4
-.fill 8*1024*1024, 1, 0
+	.balign 4
+	.long 5 		/* n_namesz.  Length of "GLIBC".  */
+	.long NOTE_SIZE		/* n_descsz.  */
+	.long 0			/* n_type.  */
+	.ascii "GLIBC\0\0\0"	/* Name and alignment to four bytes.  */
+	.fill NOTE_SIZE, 1, 0
 .popsection
Carlos O'Donell March 1, 2019, 5:51 p.m. UTC | #3 
On 3/1/19 11:41 AM, Florian Weimer wrote:
> elf/tst-big-note: Improve accuracy of test [BZ #20419]
> 
> It is possible that the link editor injects an allocated ABI tag note
> before the artificial, allocated large note in the test.  Note parsing
> in open_verify stops when the first ABI tag note is encountered, so if
> the ABI tag note comes first, the problematic code is not actually
> exercised.

OK. Good catch, and that's done on purpose. The second note is detected
in dl-prop.h, but it immediately causes the function to return without
setting ibt/sht bits. So we don't process it.

> Also tweak the artificial note so that it is a syntactically valid
> 4-byte aligned note, in case the link editor tries to parse notes and
> process them.

OK. Good catch also. Thanks for cleaning up the test.

> Improves the testing part of commit 0065aaaaae51cd60210ec3a7e13.
> 
> 2019-03-01  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #20419]
> 	* elf/tst-big-note-lib.S: Create a syntactically valid note.
> 	* elf/Makefile (tst-big-note-lib.so): Do not link with startup
> 	code, to avoid creating an ABI tag note.
> 	(modules-names-nobuild): Add tst-big-note-lib.
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 55204073a3..310a37cc13 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -316,8 +316,8 @@ endif
>  modules-execstack-yes = tst-execstack-mod
>  extra-test-objs += $(addsuffix .os,$(strip $(modules-names)))
>  
> -# filtmod1.so has a special rule
> -modules-names-nobuild := filtmod1
> +# filtmod1.so, tst-big-note-lib.so have special rules.
> +modules-names-nobuild := filtmod1 tst-big-note-lib

OK. Right so tst-big-note-lib is in $(modules-names) and so the build
is handled by extra-modules-build, unless you exclude it via
$(modules-names-nobuild), which you do here.

>  
>  tests += $(tests-static)
>  
> @@ -1515,6 +1515,11 @@ tst-libc_dlvsym-static-ENV = \
>  $(objpfx)tst-libc_dlvsym-static.out: $(objpfx)tst-libc_dlvsym-dso.so
>  
>  $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
> +# Avoid creating an ABI tag note, which may come before the
> +# artificial, large note in tst-big-note-lib.o and invalidate the
> +# test.
> +$(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
> +	$(LINK.o) -shared -o $@ $(LDFLAGS.so) $<

OK. tst-big-note already depends on the DSO.

>  
>  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>  
> diff --git a/elf/tst-big-note-lib.S b/elf/tst-big-note-lib.S
> index e2008cf4ae..721686fa0e 100644
> --- a/elf/tst-big-note-lib.S
> +++ b/elf/tst-big-note-lib.S
> @@ -20,7 +20,13 @@
>     On a typical Linux system with 8MiB "ulimit -s", that was enough
>     to trigger stack overflow in open_verify.  */
>  
> +#define NOTE_SIZE 8*1024*1024
> +

OK.

>  .pushsection .note.big,"a"
> -.balign 4
> -.fill 8*1024*1024, 1, 0
> +	.balign 4
> +	.long 5 		/* n_namesz.  Length of "GLIBC".  */
> +	.long NOTE_SIZE		/* n_descsz.  */
> +	.long 0			/* n_type.  */
> +	.ascii "GLIBC\0\0\0"	/* Name and alignment to four bytes.  */

OK. Good, alignment filled up here.

> +	.fill NOTE_SIZE, 1, 0
>  .popsection

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
diff mbox series

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 55204073a3..cc48b5d273 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1515,6 +1515,11 @@  tst-libc_dlvsym-static-ENV = \
 $(objpfx)tst-libc_dlvsym-static.out: $(objpfx)tst-libc_dlvsym-dso.so
 
 $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
+# Avoid creating an ABI tag note, which may come before the
+# artificial, large note in tst-big-note-lib.o and invalidate the
+# test.
+$(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
+	$(LINK.o) -shared -o $@ $(LDFLAGS.so) $<
 
 $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
 
diff --git a/elf/tst-big-note-lib.S b/elf/tst-big-note-lib.S
index e2008cf4ae..721686fa0e 100644
--- a/elf/tst-big-note-lib.S
+++ b/elf/tst-big-note-lib.S
@@ -20,7 +20,13 @@ 
    On a typical Linux system with 8MiB "ulimit -s", that was enough
    to trigger stack overflow in open_verify.  */
 
+#define NOTE_SIZE 8*1024*1024
+
 .pushsection .note.big,"a"
-.balign 4
-.fill 8*1024*1024, 1, 0
+	.balign 4
+	.long 5 		/* n_namesz.  Length of "GLIBC".  */
+	.long NOTE_SIZE		/* n_descsz.  */
+	.long 0			/* n_type.  */
+	.ascii "GLIBC\0\0\0"	/* Name and alignment to four bytes.  */
+	.fill NOTE_SIZE, 1, 0
 .popsection