[PATCH v2] hppa: Fix bind-now audit (BZ #28857)

[PATCH v2] hppa: Fix bind-now audit (BZ #28857)

[Adhemerval Zanella]

On hppa, a function pointer returned by la_symbind is actually a function
descriptor has the plabel bit set (bit 30).  This must be cleared to get
the actual address of the descriptor.  If the descriptor has been bound,
the first word of the descriptor is the physical address of theA function,
otherwise, the first word of the descriptor points to a trampoline in the
PLT.

This patch also adds a workaround on tests because on hppa (and it seems
to be the only ABI I have see it), some shared library adds a dynamic PLT
relocation to am empty symbol name:

$ readelf -r elf/tst-audit25mod1.so
[...]
Relocation section '.rela.plt' at offset 0x464 contains 6 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00002008  00000081 R_PARISC_IPLT                508
[...]

It breaks some assumptions on the test, where a symbol with an empty
name ("") is passed on la_symbind.

Checked on x86_64-linux-gnu and hppa-linux-gnu.

---
v2: Remove _dl_lookup_address usage.

---
 elf/Makefile                | 2 +-
 elf/dl-audit.c              | 3 ++-
 elf/tst-auditmod24a.c       | 4 +++-
 elf/tst-auditmod24d.c       | 4 +++-
 elf/tst-auditmod25.c        | 2 +-
 sysdeps/hppa/dl-fptr.c      | 1 +
 sysdeps/hppa/dl-lookupcfg.h | 8 ++++++--
 7 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/elf/Makefile b/elf/Makefile
index 5bdf0a383d..7372cb191c 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -2210,7 +2210,7 @@ $(objpfx)tst-audit24c.out: $(objpfx)tst-auditmod24c.so
 $(objpfx)tst-audit24c: $(objpfx)tst-audit24amod1.so \
 		       $(objpfx)tst-audit24amod2.so
 tst-audit24c-ENV = LD_BIND_NOW=1 LD_AUDIT=$(objpfx)tst-auditmod24c.so
-LDFLAGS-tst-audit24b = -Wl,-z,lazy
+LDFLAGS-tst-audit24c = -Wl,-z,lazy
 
 $(objpfx)tst-audit24d.out: $(objpfx)tst-auditmod24d.so
 $(objpfx)tst-audit24d: $(objpfx)tst-audit24dmod1.so \
diff --git a/elf/dl-audit.c b/elf/dl-audit.c
index 794bfd45cd..efc0492474 100644
--- a/elf/dl-audit.c
+++ b/elf/dl-audit.c
@@ -257,7 +257,8 @@ _dl_audit_symbind (struct link_map *l, struct reloc_result *reloc_result,
       reloc_result->flags = flags;
     }
 
-  DL_FIXUP_BINDNOW_RELOC (value, new_value, sym.st_value);
+  if (flags & LA_SYMB_ALTVALUE)
+    DL_FIXUP_BINDNOW_RELOC (value, new_value, sym.st_value);
 }
 
 void
diff --git a/elf/tst-auditmod24a.c b/elf/tst-auditmod24a.c
index d8e88f3984..3075dfae2f 100644
--- a/elf/tst-auditmod24a.c
+++ b/elf/tst-auditmod24a.c
@@ -110,5 +110,7 @@ la_symbind32 (Elf32_Sym *sym, unsigned int ndx,
       return sym->st_value;
     }
 
-  abort ();
+  if (symname[0] != '\0')
+    abort ();
+  return sym->st_value;
 }
diff --git a/elf/tst-auditmod24d.c b/elf/tst-auditmod24d.c
index 8c803ecc0a..badc6be451 100644
--- a/elf/tst-auditmod24d.c
+++ b/elf/tst-auditmod24d.c
@@ -116,5 +116,7 @@ la_symbind32 (Elf32_Sym *sym, unsigned int ndx,
 	}
     }
 
-  abort ();
+  if (symname[0] != '\0')
+    abort ();
+  return sym->st_value;
 }
diff --git a/elf/tst-auditmod25.c b/elf/tst-auditmod25.c
index 526f5c54bc..20640a8daf 100644
--- a/elf/tst-auditmod25.c
+++ b/elf/tst-auditmod25.c
@@ -72,7 +72,7 @@ la_symbind32 (Elf32_Sym *sym, unsigned int ndx,
 	      unsigned int *flags, const char *symname)
 #endif
 {
-  if (*refcook != -1 && *defcook != -1)
+  if (*refcook != -1 && *defcook != -1 && symname[0] != '\0')
     fprintf (stderr, "la_symbind: %s %u\n", symname,
 	     *flags & (LA_SYMB_NOPLTENTER | LA_SYMB_NOPLTEXIT) ? 1 : 0);
   return sym->st_value;
diff --git a/sysdeps/hppa/dl-fptr.c b/sysdeps/hppa/dl-fptr.c
index 2584557c4f..4cc2cb21b1 100644
--- a/sysdeps/hppa/dl-fptr.c
+++ b/sysdeps/hppa/dl-fptr.c
@@ -407,3 +407,4 @@ _dl_lookup_address (const void *address)
 
   return (ElfW(Addr)) desc[0];
 }
+rtld_hidden_def (_dl_lookup_address)
diff --git a/sysdeps/hppa/dl-lookupcfg.h b/sysdeps/hppa/dl-lookupcfg.h
index 8da2412fea..3929fc84ae 100644
--- a/sysdeps/hppa/dl-lookupcfg.h
+++ b/sysdeps/hppa/dl-lookupcfg.h
@@ -30,6 +30,7 @@ rtld_hidden_proto (_dl_symbol_address)
 #define DL_SYMBOL_ADDRESS(map, ref) _dl_symbol_address(map, ref)
 
 Elf32_Addr _dl_lookup_address (const void *address);
+rtld_hidden_proto (_dl_lookup_address)
 
 #define DL_LOOKUP_ADDRESS(addr) _dl_lookup_address ((const void *) addr)
 
@@ -81,5 +82,8 @@ void attribute_hidden _dl_unmap (struct link_map *map);
 #define DL_FIXUP_VALUE_ADDR(value) ((uintptr_t) &(value))
 #define DL_FIXUP_ADDR_VALUE(addr) (*(struct fdesc *) (addr))
 #define DL_FIXUP_BINDNOW_ADDR_VALUE(addr) (addr)
-#define DL_FIXUP_BINDNOW_RELOC(value, new_value, st_value) \
-  (*value) = *(struct fdesc *) (st_value)
+#define DL_FIXUP_BINDNOW_RELOC(value, new_value, st_value)	\
+  ({								\
+     value->ip = _dl_lookup_address ((void *) new_value);	\
+     value->gp = ((struct fdesc *) (new_value))->gp;		\
+  })
-- 
2.32.0

Re: [PATCH v2] hppa: Fix bind-now audit (BZ #28857)

[John David Anglin]

On 2022-02-07 7:49 a.m., Adhemerval Zanella wrote:
> diff --git a/sysdeps/hppa/dl-lookupcfg.h b/sysdeps/hppa/dl-lookupcfg.h
> index 8da2412fea..3929fc84ae 100644
> --- a/sysdeps/hppa/dl-lookupcfg.h
> +++ b/sysdeps/hppa/dl-lookupcfg.h
> @@ -30,6 +30,7 @@ rtld_hidden_proto (_dl_symbol_address)
>   #define DL_SYMBOL_ADDRESS(map, ref) _dl_symbol_address(map, ref)
>   
>   Elf32_Addr _dl_lookup_address (const void *address);
> +rtld_hidden_proto (_dl_lookup_address)
>   
>   #define DL_LOOKUP_ADDRESS(addr) _dl_lookup_address ((const void *) addr)
>   
> @@ -81,5 +82,8 @@ void attribute_hidden _dl_unmap (struct link_map *map);
>   #define DL_FIXUP_VALUE_ADDR(value) ((uintptr_t) &(value))
>   #define DL_FIXUP_ADDR_VALUE(addr) (*(struct fdesc *) (addr))
>   #define DL_FIXUP_BINDNOW_ADDR_VALUE(addr) (addr)
> -#define DL_FIXUP_BINDNOW_RELOC(value, new_value, st_value) \
> -  (*value) = *(struct fdesc *) (st_value)
> +#define DL_FIXUP_BINDNOW_RELOC(value, new_value, st_value)	\
> +  ({								\
> +     value->ip = _dl_lookup_address ((void *) new_value);	\
> +     value->gp = ((struct fdesc *) (new_value))->gp;		\
> +  })
This still isn't correct.  The PLABEL bit is set in new_value in the cases I looked at.  It needs
to be cleared to access the ip and gp values in the function descriptor.  Otherwise, a misaligned
access will occur for the gp value and the wrong value will be loaded.

Based on my testing, it is unnecessary to call _dl_lookup_address().  It does not seem
necessary to bind the descriptor pointed to by new_value.  All the audit24 and audit25
tests pass without calling _dl_lookup_address().  It seems we just need to clear the
PLABEL bit and copy new_value to value.  Note that _dl_lookup_address() only returns
the ip address in the descriptor when it finds a valid descriptor. Otherwise, it returns
the original pointer value.

Can the PLABEL bit be set in the addr value in DL_FIXUP_ADDR_VALUE? In any case,
it doesn't hurt to clear it as function descriptors are always at least 4-byte aligned.  Currently,
they are 8-byte aligned so the ip and gp values can be updated atomically with a floating
point store.

If the PLABEL bit is not set in a function pointer, a descriptor is not used and the pointer
points directly at function or stub.  gp does not change.  This is only supported within objects.
I don't think this case occurs in the audit code.

-- 
John David Anglin  dave.anglin@bell.net