public inbox for libffi-discuss@sourceware.org
 help / color / mirror / Atom feed
* [RFC PATCH v3 0/5] Libffi Static Trampolines
       [not found] <1ef5c7e1c9a6ebb140a476ba555ec955681f4fba>
@ 2021-01-15 18:46 ` madvenka
  2021-01-15 18:46   ` [RFC PATCH v3 1/5] " madvenka
                     ` (5 more replies)
  0 siblings, 6 replies; 30+ messages in thread
From: madvenka @ 2021-01-15 18:46 UTC (permalink / raw)
  To: libffi-discuss; +Cc: green, fweimer, dj, madvenka

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

Libffi Static Trampolines

Closures
========

Libffi Closures enable a program to call a function whose arguments, argument
types and return value are known only at runtime. Also, the calling
conventions of the called function can be different from native calling
conventions.

Closures support a variety of architectures and Application Binary Interfaces
or ABIs.

Closure Trampoline
==================

When a program invokes a closure, a libffi supplied trampoline is executed.
The trampoline loads the closure pointer in a designated register or on the
stack (depending on the ABI of the target function) and jumps to an ABI
handler. The ABI handler extracts arguments from the closure, calls the
target function and returns the function's return value in the native ABI.
To the program, it appears as if the target function was called natively.

Security Issue
==============

Currently, the trampoline code used in libffi is not statically defined in
a source file (except for MACH). The trampoline is either pre-defined
machine code in a data buffer. Or, it is generated at runtime. In order to
execute a trampoline, it needs to be placed in a page with executable
permissions.

Executable data pages are attack surfaces for attackers who may try to
inject their own code into the page and contrive to have it executed. The
security settings in a system may prevent various tricks used in user land
to write code into a page and to have it executed somehow. On such systems,
libffi trampolines would not be able to run.

Static Trampoline
=================

To solve this problem, the trampoline code needs to be defined statically
in a source file, compiled and placed in the text segment so it can be
mapped and executed naturally without any tricks. However, the trampoline
needs to be able to access the closure pointer at runtime.

PC-relative data referencing
============================

The solution implemented in this patch set uses PC-relative data references.
The trampoline is mapped in a code page. Adjacent to the code page, a data
page is mapped that contains the parameters of the trampoline:

	- the closure pointer
	- pointer to the ABI handler to jump to

The trampoline code uses an offset relative to its current PC to access its
data.

Some architectures support PC-relative data references in the ISA itself.
E.g., X64 supports RIP-relative references. For others, the PC has to
somehow be loaded into a general purpose register to do PC-relative data
referencing. To do this, we need to define a get_pc() kind of function and
call it to load the PC in a desired register.

There are two cases:

1. The call instruction pushes the return address on the stack.

   In this case, get_pc() will extract the return address from the stack
   and load it in the desired register and return.

2. The call instruction stores the return address in a designated register.

   In this case, get_pc() will copy the return address to the desired
   register and return.

Either way, the PC next to the call instruction is obtained.

Scratch register
================

In order to do its job, the trampoline code would need to use a scratch
register. Depending on the ABI, there may not be a register available for
scratch. This problem needs to be solved so that all ABIs will work.

The trampoline will save two values on the stack:

	- the closure pointer
	- the original value of the scratch register

This is what the stack will look like:

	sp before trampoline ------>	--------------------
					| closure pointer  |
					--------------------
					| scratch register |
	sp after trampoline ------->	--------------------

The ABI handler can do the following as needed:

	- the closure pointer can be loaded in a desired register

	- the scratch register can be restored to its original value

	- the stack pointer can be restored to its original value
	  (the value when the trampoline was invoked)

To do this, I have defined prolog code for each ABI handler. The legacy
trampoline jumps to the ABI handler directly. But the static trampoline
defined in this patch jumps tp the prolog code which performs the above
actions before jumping to the ABI handler.

NOTE:
	The documentation for this feature will contain information on:

	- the name of the scratch register for each architecture

	- the stack offsets at which the closure and the scratch register
	  will be copied

Trampoline Table
================

In order to reduce the trampoline memory footprint, the trampoline code
would be defined as a code array in the text segment. This array would be
mapped into the address space of the caller. The mapping would, therefore,
contain a trampoline table.

Adjacent to the trampoline table mapping, there will be a data mapping that
contains a parameter table, one parameter block for each trampoline. The
parameter block will contain:

	- a pointer to the closure
	- a pointer to the ABI handler

The static trampoline code would finally look like this:

	- Make space on the stack for the closure and the scratch register
	  by moving the stack pointer down
	- Store the original value of the scratch register on the stack
	- Using PC-relative reference, get the closure pointer
	- Store the closure pointer on the stack
	- Using PC-relative reference, get the ABI handler pointer
	- Jump to the ABI handler

Trampoline API
==============

There is a lot of dynamic code out there. They all have the same security
issue. Dynamic code can be re-written into static code provided the data
required by the static code can be passed to it just like we pass the closure
pointer to an ABI handler.

So, the same trampoline functions used by libffi internally need to be
made available to the rest of the world in the form of an API. The
following API has been defined in this solution:

int ffi_tramp_is_supported(void);

	To support static trampolines, code needs to be added to each
	architecture. This function tells us whether or not the feature is
	supported in the current libffi for the current architecture.

void *ffi_tramp_alloc (int flags);

	Allocate a trampoline. Currently, flags are zero. An opaque
	trampoline structure pointer is returned.
	
	Internally, libffi manages trampoline tables and individual
	trampolines in each table.

int ffi_tramp_set_parms (void *tramp, void *target, void *data);

	Initialize the parameters of a trampoline. That is, the target code
	that the trampoline should jump to and the data that needs to be
	passed to the target code.

void *ffi_tramp_get_addr (void *tramp);

	Return the address of the trampoline to invoke the trampoline with.
	The trampoline can be invoked in one of two ways:

		- Simply branch to the trampoline address
		- Treat the trampoline address as a function pointer and call
		  it.

	Which method is used depends on the target code.

void ffi_tramp_free (void *tramp);

	Free a trampoline.

Testing
=======

The libffi selftests have been run successfully on X86 and ARM, 32-bit and
64-bit. I also have my own API test that does stress testing of the API.

TBD
===

Changelog:

v1
	Introduced the Static Trampoline feature.

v2
	- I have removed the configuration option --enable-static-tramp from
	  configure.ac. Now, this feature is enabled unconditionally for
	  Linux and BSD variants mentioned below.

	- In v1, I had defined a method to obtain the path to libffi and the
	  offset within the libffi binary of the trampoline code table. This
	  is used to mmap() the trampoline code table into an address space.

	  This is obtained by parsing /proc/<pid>/maps. However, the maps file
	  is not available on other OSes (except NetBSD). So, I have defined
	  an alternative way. Wherever mkstemp() is available, a temporary file
	  can be created and the trampoline code table can be written into that
	  file and the temporary file can be used to map the trampoline code
	  table.

	  The code tries the maps method first. If that fails, it falls back
	  to using the temporary file method.

	- In v1, only Linux was supported. In v2, I have added support for
	  static trampolines for FreeBSD, NetBSD and OpenBSD. With the NetBSD
	  support, we can remove the following chunk of code from
	  src/closures.c:

	  #if __NetBSD_Version__ - 0 >= 799007200
	  /* NetBSD with PROT_MPROTECT */
	  ...
	  #else /* !NetBSD with PROT_MPROTECT */

	  The new code supports more versions of NetBSD than before.

	- In configure.ac, I set FFI_MMAP_EXEC_WRIT to 1 for NetBSD as well
	  so that NetBSD versions can use static trampolines instead of using
	  malloc() for obtaining executable data space.

	- The code in src/tramp.c has been organized into OS-specific and
	  generic parts to make it easier to add support for more OSes. For
	  instance, if we add support for MACH (which is just a few lines of
	  changes), we can get rid of the FFI_EXEC_TRAMPOLINE_TABLE code.

	- There was a bug in version 1. If, for any reason, the static
	  trampoline initialization fails, then libffi should fall back to
	  using the legacy trampolines. But this was not implemented correctly.
	  I have now corrected the problem.

	- Fixed a build problem encountered when CI testing was done on my pull
	  request.

v3:
	- Added some comments.

	- Fixed a compilation warning on 32-bit systems from the sscanf()
	  statement in ffi_tramp_get_libffi() in src/tramp.c.

	- Addressed build failures that happen when old compilers that have not
	  been updated for Intel Control Flow Enforcement Technology (CET) are
	  used.

	- Removed the support for BSD variants. I plan to submit these as a
	  separate patchset once the initial static trampoline patch supporting
	  Linux is accepted. All of the code works properly on the BSD variants
	  and will be worked on next. This is mainly to keep the current
	  review smaller and more focused.

	- Added code to handle the case where the OS page size can differ from
	  the hardware page size.
I need to study how to include my trampoline API test in the libffi selftests.
---

Madhavan T. Venkataraman (5):
  Libffi Static Trampolines
  x86: Support for Static Trampolines
  i386: Support for Static Trampolines
  arm64: Support for Static Trampolines
  arm: Support for Static Trampolines

 Makefile.am                                 |   3 +-
 configure.ac                                |   9 +-
 include/ffi.h.in                            |  13 +-
 include/ffi_common.h                        |   4 +
 libffi.map.in                               |   9 +
 src/aarch64/ffi.c                           |  36 +-
 src/aarch64/internal.h                      |  10 +
 src/aarch64/sysv.S                          |  68 ++
 src/arm/ffi.c                               |  29 +-
 src/arm/internal.h                          |  10 +
 src/arm/sysv.S                              |  45 ++
 src/closures.c                              |  47 +-
 src/tramp.c                                 | 718 ++++++++++++++++++++
 src/x86/ffi.c                               |  35 +
 src/x86/ffi64.c                             |  40 +-
 src/x86/ffiw64.c                            |  10 +
 src/x86/internal.h                          |  11 +
 src/x86/internal64.h                        |  11 +
 src/x86/sysv.S                              | 116 ++++
 src/x86/unix64.S                            | 104 +++
 src/x86/win64.S                             |  12 +
 testsuite/libffi.closures/closure_loc_fn0.c |   3 +
 22 files changed, 1332 insertions(+), 11 deletions(-)
 create mode 100644 src/tramp.c


base-commit: e70bf987daa7b7b5df2de7579d5c51a888e8bf7d
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v3 1/5] Libffi Static Trampolines
  2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
@ 2021-01-15 18:46   ` madvenka
  2021-01-27  3:31     ` DJ Delorie
  2021-01-15 18:46   ` [RFC PATCH v3 2/5] x86: Support for " madvenka
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: madvenka @ 2021-01-15 18:46 UTC (permalink / raw)
  To: libffi-discuss; +Cc: green, fweimer, dj, madvenka

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

Closure Trampoline Security Issue
=================================

Currently, the trampoline code used in libffi is not statically defined in
a source file (except for MACH). The trampoline is either pre-defined
machine code in a data buffer. Or, it is generated at runtime. In order to
execute a trampoline, it needs to be placed in a page with executable
permissions.

Executable data pages are attack surfaces for attackers who may try to
inject their own code into the page and contrive to have it executed. The
security settings in a system may prevent various tricks used in user land
to write code into a page and to have it executed somehow. On such systems,
libffi trampolines would not be able to run.

Static Trampoline
=================

To solve this problem, the trampoline code needs to be defined statically
in a source file, compiled and placed in the text segment so it can be
mapped and executed naturally without any tricks. However, the trampoline
needs to be able to access the closure pointer at runtime.

PC-relative data referencing
============================

The solution implemented in this patch set uses PC-relative data references.
The trampoline is mapped in a code page. Adjacent to the code page, a data
page is mapped that contains the parameters of the trampoline:

	- the closure pointer
	- pointer to the ABI handler to jump to

The trampoline code uses an offset relative to its current PC to access its
data.

Some architectures support PC-relative data references in the ISA itself.
E.g., X64 supports RIP-relative references. For others, the PC has to
somehow be loaded into a general purpose register to do PC-relative data
referencing. To do this, we need to define a get_pc() kind of function and
call it to load the PC in a desired register.

There are two cases:

1. The call instruction pushes the return address on the stack.

   In this case, get_pc() will extract the return address from the stack
   and load it in the desired register and return.

2. The call instruction stores the return address in a designated register.

   In this case, get_pc() will copy the return address to the desired
   register and return.

Either way, the PC next to the call instruction is obtained.

Scratch register
================

In order to do its job, the trampoline code would need to use a scratch
register. Depending on the ABI, there may not be a register available for
scratch. This problem needs to be solved so that all ABIs will work.

The trampoline will save two values on the stack:

	- the closure pointer
	- the original value of the scratch register

This is what the stack will look like:

	sp before trampoline ------>	--------------------
					| closure pointer  |
					--------------------
					| scratch register |
	sp after trampoline ------->	--------------------

The ABI handler can do the following as needed by the ABI:

	- the closure pointer can be loaded in a desired register

	- the scratch register can be restored to its original value

	- the stack pointer can be restored to its original value
	  (the value when the trampoline was invoked)

To do this, I have defined prolog code for each ABI handler. The legacy
trampoline jumps to the ABI handler directly. But the static trampoline
defined in this patch jumps tp the prolog code which performs the above
actions before jumping to the ABI handler.

NOTE:
	The documentation for this feature will contain information on:

	- the name of the scratch register for each architecture

	- the stack offsets at which the closure and the scratch register
	  will be copied

Trampoline Table
================

In order to reduce the trampoline memory footprint, the trampoline code
would be defined as a code array in the text segment. This array would be
mapped into the address space of the caller. The mapping would, therefore,
contain a trampoline table.

Adjacent to the trampoline table mapping, there will be a data mapping that
contains a parameter table, one parameter block for each trampoline. The
parameter block will contain:

	- a pointer to the closure
	- a pointer to the ABI handler

The static trampoline code would finally look like this:

	- Make space on the stack for the closure and the scratch register
	  by moving the stack pointer down
	- Store the original value of the scratch register on the stack
	- Using PC-relative reference, get the closure pointer
	- Store the closure pointer on the stack
	- Using PC-relative reference, get the ABI handler pointer
	- Jump to the ABI handler

Trampoline API
==============

There is a lot of dynamic code out there. They all have the same security
issue. Dynamic code can be re-written into static code provided the data
required by the static code can be passed to it just like we pass the
closure pointer to an ABI handler.

So, the same trampoline functions used by libffi internally need to be
made available to the rest of the world in the form of an API. The
following API has been defined in this solution:

int ffi_tramp_is_supported(void);

	To support static trampolines, code needs to be added to each
	architecture. This function tells us whether or not the feature is
	supported in the current libffi for the current architecture.

void *ffi_tramp_alloc (int flags);

	Allocate a trampoline. Currently, flags are zero. An opaque
	trampoline structure pointer is returned.

	Internally, libffi manages trampoline tables and individual
	trampolines in each table.

int ffi_tramp_set_parms (void *tramp, void *target, void *data);

	Initialize the parameters of a trampoline. That is, the target code
	that the trampoline should jump to and the data that needs to be
	passed to the target code.

void *ffi_tramp_get_addr (void *tramp);

	Return the address of the trampoline to invoke the trampoline with.
	The trampoline can be invoked in one of two ways:

		- Simply branch to the trampoline address
		- Treat the trampoline address as a function pointer and
		  call it.

	Which method is used depends on the target code.

void ffi_tramp_free (void *tramp);

	Free a trampoline.

Mapping size
============

The size of the code mapping that contains the trampoline table needs to be
determined on a per architecture basis. If a particular architecture
supports multiple base page sizes, then the largest supported base page size
needs to be chosen. E.g., we choose 16K for ARM64.

Trampoline allocation and free
==============================

Static trampolines are allocated in ffi_closure_alloc() and freed in
ffi_closure_free().

Normally, applications use these functions. But there are some cases out
there where the user of libffi allocates and manages its own closure
memory. In such cases, the static trampoline API cannot be used. These
will fall back to using legacy trampolines. The user has to make sure
that the memory is executable.

ffi_closure structure
=====================

I did not want to make any changes to the size of the closure structure for
this feature to guarantee compatibility. But the opaque static trampoline
handle needs to be stored in the closure. I have defined it as follows:

-  char tramp[FFI_TRAMPOLINE_SIZE];
+  union {
+    char tramp[FFI_TRAMPOLINE_SIZE];
+    void *ftramp;
+  };

If static trampolines are used, then tramp[] is not needed to store a
dynamic trampoline. That space can be reused to store the handle. Hence,
the union.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
 Makefile.am                                 |   3 +-
 configure.ac                                |   9 +-
 include/ffi.h.in                            |  13 +-
 include/ffi_common.h                        |   4 +
 libffi.map.in                               |   9 +
 src/closures.c                              |  47 +-
 src/tramp.c                                 | 718 ++++++++++++++++++++
 testsuite/libffi.closures/closure_loc_fn0.c |   3 +
 8 files changed, 801 insertions(+), 5 deletions(-)
 create mode 100644 src/tramp.c

diff --git a/Makefile.am b/Makefile.am
index 7654bf5..1b18198 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -38,7 +38,8 @@ toolexeclib_LTLIBRARIES = libffi.la
 noinst_LTLIBRARIES = libffi_convenience.la
 
 libffi_la_SOURCES = src/prep_cif.c src/types.c \
-		src/raw_api.c src/java_raw_api.c src/closures.c
+		src/raw_api.c src/java_raw_api.c src/closures.c \
+		src/tramp.c
 
 if FFI_DEBUG
 libffi_la_SOURCES += src/debug.c
diff --git a/configure.ac b/configure.ac
index 790274e..8bfbabf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -64,7 +64,7 @@ EOF
 AM_MAINTAINER_MODE
 
 AC_CHECK_HEADERS(sys/mman.h)
-AC_CHECK_FUNCS([mmap mkostemp])
+AC_CHECK_FUNCS([mmap mkostemp mkstemp])
 AC_FUNC_MMAP_BLACKLIST
 
 dnl The -no-testsuite modules omit the test subdir.
@@ -360,6 +360,13 @@ AC_ARG_ENABLE(raw-api,
     AC_DEFINE(FFI_NO_RAW_API, 1, [Define this if you do not want support for the raw API.])
   fi)
 
+case "$target" in
+     *-linux*)
+       AC_DEFINE(FFI_EXEC_STATIC_TRAMP, 1,
+                 [Define this if you want statically defined trampolines])
+     ;;
+esac
+
 AC_ARG_ENABLE(purify-safety,
 [  --enable-purify-safety  purify-safe mode],
   if test "$enable_purify_safety" = "yes"; then
diff --git a/include/ffi.h.in b/include/ffi.h.in
index 38885b0..c6a21d9 100644
--- a/include/ffi.h.in
+++ b/include/ffi.h.in
@@ -310,7 +310,10 @@ typedef struct {
   void *trampoline_table;
   void *trampoline_table_entry;
 #else
-  char tramp[FFI_TRAMPOLINE_SIZE];
+  union {
+    char tramp[FFI_TRAMPOLINE_SIZE];
+    void *ftramp;
+  };
 #endif
   ffi_cif   *cif;
   void     (*fun)(ffi_cif*,void*,void**,void*);
@@ -457,6 +460,14 @@ FFI_API void ffi_call_go (ffi_cif *cif, void (*fn)(void), void *rvalue,
 
 #endif /* FFI_GO_CLOSURES */
 
+/* ---- Static Trampoline Definitions -------------------------------------- */
+
+FFI_API int ffi_tramp_is_supported(void);
+FFI_API void *ffi_tramp_alloc (int flags);
+FFI_API int ffi_tramp_set_parms (void *tramp, void *data, void *code);
+FFI_API void *ffi_tramp_get_addr (void *tramp);
+FFI_API void ffi_tramp_free (void *tramp);
+
 /* ---- Public interface definition -------------------------------------- */
 
 FFI_API 
diff --git a/include/ffi_common.h b/include/ffi_common.h
index 76b9dd6..d3db33a 100644
--- a/include/ffi_common.h
+++ b/include/ffi_common.h
@@ -103,6 +103,10 @@ ffi_status ffi_prep_cif_core(ffi_cif *cif,
    some targets.  */
 void *ffi_data_to_code_pointer (void *data) FFI_HIDDEN;
 
+/* The arch code calls this to determine if a given closure has a
+   static trampoline. */
+int ffi_tramp_is_present (void *closure);
+
 /* Extended cif, used in callback from assembly routine */
 typedef struct
 {
diff --git a/libffi.map.in b/libffi.map.in
index de8778a..9d8a472 100644
--- a/libffi.map.in
+++ b/libffi.map.in
@@ -74,3 +74,12 @@ LIBFFI_GO_CLOSURE_8.0 {
 	ffi_prep_go_closure;
 } LIBFFI_CLOSURE_8.0;
 #endif
+
+LIBFFI_STATIC_TRAMP_8.0 {
+  global:
+	ffi_tramp_is_supported;
+	ffi_tramp_alloc;
+	ffi_tramp_set_parms;
+	ffi_tramp_get_addr;
+	ffi_tramp_free;
+} LIBFFI_BASE_8.0;
diff --git a/src/closures.c b/src/closures.c
index 4fe6158..a006548 100644
--- a/src/closures.c
+++ b/src/closures.c
@@ -109,6 +109,12 @@ ffi_closure_free (void *ptr)
   munmap(dataseg, rounded_size);
   munmap(codeseg, rounded_size);
 }
+
+int
+ffi_tramp_is_present (__attribute__((unused)) void *ptr)
+{
+  return 0;
+}
 #else /* !NetBSD with PROT_MPROTECT */
 
 #if !FFI_MMAP_EXEC_WRIT && !FFI_EXEC_TRAMPOLINE_TABLE
@@ -843,6 +849,12 @@ dlmmap (void *start, size_t length, int prot,
 	  && flags == (MAP_PRIVATE | MAP_ANONYMOUS)
 	  && fd == -1 && offset == 0);
 
+  if (execfd == -1 && ffi_tramp_is_supported ())
+    {
+      ptr = mmap (start, length, prot & ~PROT_EXEC, flags, fd, offset);
+      return ptr;
+    }
+
   if (execfd == -1 && is_emutramp_enabled ())
     {
       ptr = mmap (start, length, prot & ~PROT_EXEC, flags, fd, offset);
@@ -922,7 +934,7 @@ segment_holding_code (mstate m, char* addr)
 void *
 ffi_closure_alloc (size_t size, void **code)
 {
-  void *ptr;
+  void *ptr, *ftramp;
 
   if (!code)
     return NULL;
@@ -934,6 +946,17 @@ ffi_closure_alloc (size_t size, void **code)
       msegmentptr seg = segment_holding (gm, ptr);
 
       *code = add_segment_exec_offset (ptr, seg);
+      if (!ffi_tramp_is_supported ())
+        return ptr;
+
+      ftramp = ffi_tramp_alloc (0);
+      if (ftramp == NULL)
+      {
+        dlfree (FFI_RESTORE_PTR (ptr));
+        return NULL;
+      }
+      *code = ffi_tramp_get_addr (ftramp);
+      ((ffi_closure *) ptr)->ftramp = ftramp;
     }
 
   return ptr;
@@ -943,12 +966,17 @@ void *
 ffi_data_to_code_pointer (void *data)
 {
   msegmentptr seg = segment_holding (gm, data);
+
   /* We expect closures to be allocated with ffi_closure_alloc(), in
      which case seg will be non-NULL.  However, some users take on the
      burden of managing this memory themselves, in which case this
      we'll just return data. */
   if (seg)
-    return add_segment_exec_offset (data, seg);
+    {
+      if (!ffi_tramp_is_supported ())
+        return add_segment_exec_offset (data, seg);
+      return ffi_tramp_get_addr (((ffi_closure *) data)->ftramp);
+    }
   else
     return data;
 }
@@ -966,10 +994,19 @@ ffi_closure_free (void *ptr)
   if (seg)
     ptr = sub_segment_exec_offset (ptr, seg);
 #endif
+  if (ffi_tramp_is_supported ())
+    ffi_tramp_free (((ffi_closure *) ptr)->ftramp);
 
   dlfree (FFI_RESTORE_PTR (ptr));
 }
 
+int
+ffi_tramp_is_present (void *ptr)
+{
+  msegmentptr seg = segment_holding (gm, ptr);
+  return seg != NULL && ffi_tramp_is_supported();
+}
+
 # else /* ! FFI_MMAP_EXEC_WRIT */
 
 /* On many systems, memory returned by malloc is writable and
@@ -998,6 +1035,12 @@ ffi_data_to_code_pointer (void *data)
   return data;
 }
 
+int
+ffi_tramp_is_present (__attribute__((unused)) void *ptr)
+{
+  return 0;
+}
+
 # endif /* ! FFI_MMAP_EXEC_WRIT */
 #endif /* FFI_CLOSURES */
 
diff --git a/src/tramp.c b/src/tramp.c
new file mode 100644
index 0000000..4151cc9
--- /dev/null
+++ b/src/tramp.c
@@ -0,0 +1,718 @@
+/* -----------------------------------------------------------------------
+   tramp.c - Copyright (c) 2020 Madhavan T. Venkataraman
+
+   API and support functions for managing statically defined closure
+   trampolines.
+
+   Permission is hereby granted, free of charge, to any person obtaining
+   a copy of this software and associated documentation files (the
+   ``Software''), to deal in the Software without restriction, including
+   without limitation the rights to use, copy, modify, merge, publish,
+   distribute, sublicense, and/or sell copies of the Software, and to
+   permit persons to whom the Software is furnished to do so, subject to
+   the following conditions:
+
+   The above copyright notice and this permission notice shall be included
+   in all copies or substantial portions of the Software.
+
+   THE SOFTWARE IS PROVIDED ``AS IS'', WITHOUT WARRANTY OF ANY KIND,
+   EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+   MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+   NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+   HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+   WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+   OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+   DEALINGS IN THE SOFTWARE.
+   ----------------------------------------------------------------------- */
+
+#include <fficonfig.h>
+
+#ifdef FFI_EXEC_STATIC_TRAMP
+
+/* -------------------------- Headers and Definitions ---------------------*/
+/*
+ * Add support for other OSes later. For now, it is just Linux.
+ */
+
+#if defined __linux__
+#ifdef __linux__
+#define _GNU_SOURCE 1
+#endif
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <sys/mman.h>
+#ifdef __linux__
+#include <linux/limits.h>
+#include <linux/types.h>
+#endif
+#endif /* __linux__ */
+
+/*
+ * Each architecture defines static code for a trampoline code table. The
+ * trampoline code table is mapped into the address space of a process.
+ *
+ * The following architecture specific function returns:
+ *
+ *	- the address of the trampoline code table in the text segment
+ *	- the size of each trampoline in the trampoline code table
+ *	- the size of the mapping for the whole trampoline code table
+ */
+void __attribute__((weak)) *ffi_tramp_arch (size_t *tramp_size,
+  size_t *map_size);
+
+/* ------------------------- Trampoline Data Structures --------------------*/
+
+struct tramp;
+
+/*
+ * Trampoline table. Manages one trampoline code table and one trampoline
+ * parameter table.
+ *
+ * prev, next	Links in the global trampoline table list.
+ * code_table	Trampoline code table mapping.
+ * parm_table	Trampoline parameter table mapping.
+ * array	Array of trampolines malloced.
+ * free		List of free trampolines.
+ * nfree	Number of free trampolines.
+ */
+struct tramp_table
+{
+  struct tramp_table *prev;
+  struct tramp_table *next;
+  void *code_table;
+  void *parm_table;
+  struct tramp *array;
+  struct tramp *free;
+  int nfree;
+};
+
+/*
+ * Parameters for each trampoline.
+ *
+ * data
+ *	Data for the target code that the trampoline jumps to.
+ * target
+ *	Target code that the trampoline jumps to.
+ */
+struct tramp_parm
+{
+  void *data;
+  void *target;
+};
+
+/*
+ * Trampoline structure for each trampoline.
+ *
+ * prev, next	Links in the trampoline free list of a trampoline table.
+ * table	Trampoline table to which this trampoline belongs.
+ * code		Address of this trampoline in the code table mapping.
+ * parm		Address of this trampoline's parameters in the parameter
+ *		table mapping.
+ */
+struct tramp
+{
+  struct tramp *prev;
+  struct tramp *next;
+  struct tramp_table *table;
+  void *code;
+  struct tramp_parm *parm;
+};
+
+enum gtramp_status {
+	GTRAMP_UNINITIALIZED = 0,
+	GTRAMP_PASSED,
+	GTRAMP_FAILED,
+};
+
+/*
+ * Trampoline globals.
+ *
+ * fd
+ *	File descriptor of binary file that contains the trampoline code table.
+ * offset
+ *	Offset of the trampoline code table in that file.
+ * text
+ *	Address of the trampoline code table in the text segment.
+ * map_size
+ *	Size of the trampoline code table mapping.
+ * size
+ *	Size of one trampoline in the trampoline code table.
+ * ntramp
+ *	Total number of trampolines in the trampoline code table.
+ * tables
+ *	List of trampoline tables that contain free trampolines.
+ * ntables
+ *	Number of trampoline tables that contain free trampolines.
+ * status
+ *	Initialization status.
+ */
+struct tramp_global
+{
+  int fd;
+  off_t offset;
+  void *text;
+  size_t map_size;
+  size_t size;
+  int ntramp;
+  struct tramp_table *tables;
+  int ntables;
+  enum gtramp_status status;
+};
+
+static struct tramp_global gtramp;
+
+/* --------------------- Trampoline File Initialization --------------------*/
+
+/*
+ * The trampoline file is the file used to map the trampoline code table into
+ * the address space of a process. There are two ways to get this file:
+ *
+ * - From the OS. E.g., on Linux, /proc/<pid>/maps lists all the memory
+ *   mappings for <pid>. For file-backed mappings, maps supplies the file name
+ *   and the file offset. Using this, we can locate the mapping that maps
+ *   libffi and get the path to the libffi binary. And, we can compute the
+ *   offset of the trampoline code table within that binary.
+ *
+ * - Else, if we can create a temporary file, we can write the trampoline code
+ *   table from the text segment into the temporary file.
+ *
+ * The first method is the preferred one. If the OS security subsystem
+ * disallows mapping unsigned files with PROT_EXEC, then the second method
+ * will fail.
+ *
+ * If an OS allows the trampoline code table in the text segment to be
+ * directly remapped (e.g., MACH vm_remap ()), then we don't need the
+ * trampoline file.
+ */
+static int tramp_table_alloc (void);
+
+#if defined __linux__
+
+static int
+ffi_tramp_get_libffi (void)
+{
+  FILE *fp;
+  char file[PATH_MAX], line[PATH_MAX+100], perm[10], dev[10];
+  unsigned long start, end, offset, inode;
+  uintptr_t addr = (uintptr_t) gtramp.text;
+  int nfields, found;
+
+  snprintf (file, PATH_MAX, "/proc/%d/maps", getpid());
+  fp = fopen (file, "r");
+  if (fp == NULL)
+    return 0;
+
+  found = 0;
+  while (feof (fp) == 0) {
+    if (fgets (line, sizeof (line), fp) == 0)
+      break;
+
+    nfields = sscanf (line, "%lx-%lx %s %lx %s %ld %s",
+      &start, &end, perm, &offset, dev, &inode, file);
+    if (nfields != 7)
+      continue;
+
+    if (addr >= start && addr < end) {
+      gtramp.offset = offset + (addr - start);
+      found = 1;
+      break;
+    }
+  }
+  fclose (fp);
+
+  if (!found)
+    return 0;
+
+  gtramp.fd = open (file, O_RDONLY);
+  if (gtramp.fd == -1)
+    return 0;
+
+  /*
+   * Allocate a trampoline table just to make sure that the trampoline code
+   * table can be mapped.
+   */
+  if (!tramp_table_alloc ())
+    {
+      close (gtramp.fd);
+      gtramp.fd = -1;
+      return 0;
+    }
+  return 1;
+}
+
+#endif /* __linux__ */
+
+#if defined __linux__
+
+#if defined HAVE_MKSTEMP
+
+static int
+ffi_tramp_get_temp_file (void)
+{
+  char template[12] = "/tmp/XXXXXX";
+  ssize_t count;
+
+  gtramp.offset = 0;
+  gtramp.fd = mkstemp (template);
+  if (gtramp.fd == -1)
+    return 0;
+
+  unlink (template);
+  /*
+   * Write the trampoline code table into the temporary file and allocate a
+   * trampoline table to make sure that the temporary file can be mapped.
+   */
+  count = write(gtramp.fd, gtramp.text, gtramp.map_size);
+  if (count == gtramp.map_size && tramp_table_alloc ())
+    return 1;
+
+  close (gtramp.fd);
+  gtramp.fd = -1;
+  return 0;
+}
+
+#else /* !defined HAVE_MKSTEMP */
+
+/*
+ * TODO:
+ * Perhaps, libffi can supply its own version of mkstemp() if it is
+ * not natively available.
+ */
+static int
+ffi_tramp_get_temp_file (void)
+{
+  gtramp.offset = 0;
+  gtramp.fd = -1;
+  return 0;
+}
+
+#endif /* defined HAVE_MKSTEMP */
+
+#endif /* __linux__ */
+
+/* ------------------------ OS-specific Initialization ----------------------*/
+
+#if defined __linux__
+
+static int
+ffi_tramp_init_os (void)
+{
+  if (ffi_tramp_get_libffi ())
+    return 1;
+  return ffi_tramp_get_temp_file ();
+}
+
+#endif /* __linux__ */
+
+/* --------------------------- OS-specific Locking -------------------------*/
+
+#if defined __linux__
+
+static pthread_mutex_t gtramp_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static void
+ffi_tramp_lock(void)
+{
+  pthread_mutex_lock (&gtramp_mutex);
+}
+
+static void
+ffi_tramp_unlock()
+{
+  pthread_mutex_unlock (&gtramp_mutex);
+}
+
+#endif /* __linux__ */
+
+/* ------------------------ OS-specific Memory Mapping ----------------------*/
+
+/*
+ * Create a trampoline code table mapping and a trampoline parameter table
+ * mapping. The two mappings must be adjacent to each other for PC-relative
+ * access.
+ *
+ * For each trampoline in the code table, there is a corresponding parameter
+ * block in the parameter table. The size of the parameter block is the same
+ * as the size of the trampoline. This means that the parameter block is at
+ * a fixed offset from its trampoline making it easy for a trampoline to find
+ * its parameters using PC-relative access.
+ *
+ * The parameter block will contain a struct tramp_parm. This means that
+ * sizeof (struct tramp_parm) cannot exceed the size of a parameter block.
+ */
+
+#if defined __linux__
+
+static int
+tramp_table_map (struct tramp_table *table)
+{
+  char *addr;
+
+  /*
+   * Create an anonymous mapping twice the map size. The top half will be used
+   * for the code table. The bottom half will be used for the parameter table.
+   */
+  addr = mmap (NULL, gtramp.map_size * 2, PROT_READ | PROT_WRITE,
+    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (addr == MAP_FAILED)
+    return 0;
+
+  /*
+   * Replace the top half of the anonymous mapping with the code table mapping.
+   */
+  table->code_table = mmap (addr, gtramp.map_size, PROT_READ | PROT_EXEC,
+    MAP_PRIVATE | MAP_FIXED, gtramp.fd, gtramp.offset);
+  if (table->code_table == MAP_FAILED)
+    {
+      (void) munmap (addr, gtramp.map_size * 2);
+      return 0;
+    }
+  table->parm_table = table->code_table + gtramp.map_size;
+  return 1;
+}
+
+static void
+tramp_table_unmap (struct tramp_table *table)
+{
+  (void) munmap (table->code_table, gtramp.map_size);
+  (void) munmap (table->parm_table, gtramp.map_size);
+}
+
+#endif /* __linux__ */
+
+/* ------------------------ Trampoline Initialization ----------------------*/
+
+/*
+ * Initialize the static trampoline feature.
+ */
+static int
+ffi_tramp_init (void)
+{
+  if (gtramp.status == GTRAMP_PASSED)
+    return 1;
+
+  if (gtramp.status == GTRAMP_FAILED)
+    return 0;
+
+  if (ffi_tramp_arch == NULL)
+    {
+      gtramp.status = GTRAMP_FAILED;
+      return 0;
+    }
+
+  gtramp.tables = NULL;
+  gtramp.ntables = 0;
+
+  /*
+   * Get trampoline code table information from the architecture.
+   */
+  gtramp.text = ffi_tramp_arch (&gtramp.size, &gtramp.map_size);
+  gtramp.ntramp = gtramp.map_size / gtramp.size;
+
+  if (sysconf (_SC_PAGESIZE) > gtramp.map_size)
+    return 0;
+
+  if (ffi_tramp_init_os ())
+    {
+      gtramp.status = GTRAMP_PASSED;
+      return 1;
+    }
+
+  gtramp.status = GTRAMP_FAILED;
+  return 0;
+}
+
+/* ---------------------- Trampoline Table functions ---------------------- */
+
+/* This code assumes that malloc () is available on all OSes. */
+
+static void tramp_add (struct tramp *tramp);
+
+/*
+ * Allocate and initialize a trampoline table.
+ */
+static int
+tramp_table_alloc (void)
+{
+  struct tramp_table *table;
+  struct tramp *tramp_array, *tramp;
+  size_t size;
+  char *code, *parm;
+  int i;
+
+  /*
+   * If we already have tables with free trampolines, there is no need to
+   * allocate a new table.
+   */
+  if (gtramp.ntables > 0)
+    return 1;
+
+  /*
+   * Allocate a new trampoline table structure.
+   */
+  table = malloc (sizeof (*table));
+  if (table == NULL)
+    return 0;
+
+  /*
+   * Allocate new trampoline structures.
+   */
+  tramp_array = malloc (sizeof (*tramp) * gtramp.ntramp);
+  if (tramp_array == NULL)
+    goto free_table;
+
+  /*
+   * Map a code table and a parameter table into the caller's address space.
+   */
+  if (!tramp_table_map (table))
+    goto free_tramp_array;
+
+  /*
+   * Initialize the trampoline table.
+   */
+  table->array = tramp_array;
+  table->free = NULL;
+  table->nfree = 0;
+
+  /*
+   * Populate the trampoline table free list. This will also add the trampoline
+   * table to the global list of trampoline tables.
+   */
+  size = gtramp.size;
+  code = table->code_table;
+  parm = table->parm_table;
+  for (i = 0; i < gtramp.ntramp; i++)
+    {
+      tramp = &tramp_array[i];
+      tramp->table = table;
+      tramp->code = code;
+      tramp->parm = (struct tramp_parm *) parm;
+      tramp_add (tramp);
+
+      code += size;
+      parm += size;
+    }
+  return 1;
+
+free_tramp_array:
+  free (tramp_array);
+free_table:
+  free (table);
+  return 0;
+}
+
+/*
+ * Free a trampoline table.
+ */
+static void
+tramp_table_free (struct tramp_table *table)
+{
+  tramp_table_unmap (table);
+  free (table->array);
+  free (table);
+}
+
+/*
+ * Add a new trampoline table to the global table list.
+ */
+static void
+tramp_table_add (struct tramp_table *table)
+{
+  table->next = gtramp.tables;
+  table->prev = NULL;
+  if (gtramp.tables != NULL)
+    gtramp.tables->prev = table;
+  gtramp.tables = table;
+  gtramp.ntables++;
+}
+
+/*
+ * Delete a trampoline table from the global table list.
+ */
+static void
+tramp_table_del (struct tramp_table *table)
+{
+  gtramp.ntables--;
+  if (table->prev != NULL)
+    table->prev->next = table->next;
+  if (table->next != NULL)
+    table->next->prev = table->prev;
+  if (gtramp.tables == table)
+    gtramp.tables = table->next;
+}
+
+/* ------------------------- Trampoline functions ------------------------- */
+
+/*
+ * Add a trampoline to its trampoline table.
+ */
+static void
+tramp_add (struct tramp *tramp)
+{
+  struct tramp_table *table = tramp->table;
+
+  tramp->next = table->free;
+  tramp->prev = NULL;
+  if (table->free != NULL)
+    table->free->prev = tramp;
+  table->free = tramp;
+  table->nfree++;
+
+  if (table->nfree == 1)
+    tramp_table_add (table);
+
+  /*
+   * We don't want to keep too many free trampoline tables lying around.
+   */
+  if (table->nfree == gtramp.ntramp && gtramp.ntables > 1)
+    {
+      tramp_table_del (table);
+      tramp_table_free (table);
+    }
+}
+
+/*
+ * Remove a trampoline from its trampoline table.
+ */
+static void
+tramp_del (struct tramp *tramp)
+{
+  struct tramp_table *table = tramp->table;
+
+  table->nfree--;
+  if (tramp->prev != NULL)
+    tramp->prev->next = tramp->next;
+  if (tramp->next != NULL)
+    tramp->next->prev = tramp->prev;
+  if (table->free == tramp)
+    table->free = tramp->next;
+
+  if (table->nfree == 0)
+    tramp_table_del (table);
+}
+
+/* ------------------------ Trampoline API functions ------------------------ */
+
+int
+ffi_tramp_is_supported(void)
+{
+  int ret;
+
+  ffi_tramp_lock();
+  ret = ffi_tramp_init ();
+  ffi_tramp_unlock();
+  return ret;
+}
+
+/*
+ * Allocate a trampoline and return its opaque address.
+ */
+void *
+ffi_tramp_alloc (int flags)
+{
+  struct tramp *tramp;
+
+  ffi_tramp_lock();
+
+  if (!ffi_tramp_init () || flags != 0)
+    {
+      ffi_tramp_unlock();
+      return NULL;
+    }
+
+  if (!tramp_table_alloc ())
+    {
+      ffi_tramp_unlock();
+      return NULL;
+    }
+
+  tramp = gtramp.tables->free;
+  tramp_del (tramp);
+
+  ffi_tramp_unlock();
+
+  return tramp;
+}
+
+/*
+ * Set the parameters for a trampoline.
+ */
+void
+ffi_tramp_set_parms (void *arg, void *target, void *data)
+{
+  struct tramp *tramp = arg;
+
+  ffi_tramp_lock();
+  tramp->parm->target = target;
+  tramp->parm->data = data;
+  ffi_tramp_unlock();
+}
+
+/*
+ * Get the invocation address of a trampoline.
+ */
+void *
+ffi_tramp_get_addr (void *arg)
+{
+  struct tramp *tramp = arg;
+  void *addr;
+
+  ffi_tramp_lock();
+  addr = tramp->code;
+  ffi_tramp_unlock();
+
+  return addr;
+}
+
+/*
+ * Free a trampoline.
+ */
+void
+ffi_tramp_free (void *arg)
+{
+  struct tramp *tramp = arg;
+
+  ffi_tramp_lock();
+  tramp_add (tramp);
+  ffi_tramp_unlock();
+}
+
+/* ------------------------------------------------------------------------- */
+
+#else /* !FFI_EXEC_STATIC_TRAMP */
+
+#include <stddef.h>
+
+int
+ffi_tramp_is_supported(void)
+{
+  return 0;
+}
+
+void *
+ffi_tramp_alloc (int flags)
+{
+  return NULL;
+}
+
+void
+ffi_tramp_set_parms (void *arg, void *target, void *data)
+{
+}
+
+void *
+ffi_tramp_get_addr (void *arg)
+{
+  return NULL;
+}
+
+void
+ffi_tramp_free (void *arg)
+{
+}
+
+#endif /* FFI_EXEC_STATIC_TRAMP */
diff --git a/testsuite/libffi.closures/closure_loc_fn0.c b/testsuite/libffi.closures/closure_loc_fn0.c
index b3afa0b..ad488ac 100644
--- a/testsuite/libffi.closures/closure_loc_fn0.c
+++ b/testsuite/libffi.closures/closure_loc_fn0.c
@@ -83,7 +83,10 @@ int main (void)
   CHECK(ffi_prep_closure_loc(pcl, &cif, closure_loc_test_fn0,
 			 (void *) 3 /* userdata */, codeloc) == FFI_OK);
   
+#ifndef FFI_EXEC_STATIC_TRAMP
+  /* With static trampolines, the codeloc does not point to closure */
   CHECK(memcmp(pcl, codeloc, sizeof(*pcl)) == 0);
+#endif
 
   res = (*((closure_loc_test_type0)codeloc))
     (1LL, 2, 3LL, 4, 127, 429LL, 7, 8, 9.5, 10, 11, 12, 13,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
  2021-01-15 18:46   ` [RFC PATCH v3 1/5] " madvenka
@ 2021-01-15 18:46   ` madvenka
  2021-01-27  3:31     ` DJ Delorie
  2021-01-15 18:46   ` [RFC PATCH v3 3/5] i386: " madvenka
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: madvenka @ 2021-01-15 18:46 UTC (permalink / raw)
  To: libffi-discuss; +Cc: green, fweimer, dj, madvenka

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

	- Define the arch-specific initialization function ffi_tramp_arch ()
	  that returns trampoline size information to common code.

	- Define the trampoline code mapping and data mapping sizes.

	- Define the trampoline code table statically. Define two tables,
	  actually, one with CET and one without.

	- Introduce a tiny prolog for each ABI handling function. The ABI
	  handlers addressed are:

	  	- ffi_closure_unix64
		- ffi_closure_unix64_sse
		- ffi_closure_win64

	  The prolog functions are called:

		- ffi_closure_unix64_alt
		- ffi_closure_unix64_sse_alt
		- ffi_closure_win64_alt

	  The legacy trampoline jumps to the ABI handler. The static
	  trampoline jumps to the prolog function. The prolog function uses
	  the information provided by the static trampoline, sets things up
	  for the ABI handler and then jumps to the ABI handler.

	- Call ffi_closure_tramp_init () in ffi_prep_closure_loc () to
	  initialize static trampoline parameters.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
 src/x86/ffi64.c      |  40 ++++++++++++++++-
 src/x86/ffiw64.c     |  10 +++++
 src/x86/internal64.h |  11 +++++
 src/x86/unix64.S     | 104 +++++++++++++++++++++++++++++++++++++++++++
 src/x86/win64.S      |  12 +++++
 5 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/src/x86/ffi64.c b/src/x86/ffi64.c
index 39f9598..2a5cf5a 100644
--- a/src/x86/ffi64.c
+++ b/src/x86/ffi64.c
@@ -713,7 +713,9 @@ ffi_call_go (ffi_cif *cif, void (*fn)(void), void *rvalue,
 #endif /* FFI_GO_CLOSURES */
 
 extern void ffi_closure_unix64(void) FFI_HIDDEN;
+extern void ffi_closure_unix64_alt(void) FFI_HIDDEN;
 extern void ffi_closure_unix64_sse(void) FFI_HIDDEN;
+extern void ffi_closure_unix64_sse_alt(void) FFI_HIDDEN;
 
 #ifndef __ILP32__
 extern ffi_status
@@ -742,6 +744,7 @@ ffi_prep_closure_loc (ffi_closure* closure,
     0x0f, 0x1f, 0x80, 0x00, 0x00, 0x00, 0x00
   };
   void (*dest)(void);
+  void (*dest_alt)(void);
   char *tramp = closure->tramp;
 
 #ifndef __ILP32__
@@ -752,13 +755,28 @@ ffi_prep_closure_loc (ffi_closure* closure,
     return FFI_BAD_ABI;
 
   if (cif->flags & UNIX64_FLAG_XMM_ARGS)
-    dest = ffi_closure_unix64_sse;
+    {
+      dest = ffi_closure_unix64_sse;
+      dest_alt = ffi_closure_unix64_sse_alt;
+    }
   else
-    dest = ffi_closure_unix64;
+    {
+      dest = ffi_closure_unix64;
+      dest_alt = ffi_closure_unix64_alt;
+    }
 
+  if (ffi_tramp_is_present(closure))
+    {
+      /* Initialize the static trampoline's parameters. */
+      ffi_tramp_set_parms (closure->ftramp, dest_alt, closure);
+      goto out;
+    }
+
+  /* Initialize the dynamic trampoline. */
   memcpy (tramp, trampoline, sizeof(trampoline));
   *(UINT64 *)(tramp + sizeof (trampoline)) = (uintptr_t)dest;
 
+out:
   closure->cif = cif;
   closure->fun = fun;
   closure->user_data = user_data;
@@ -892,4 +910,22 @@ ffi_prep_go_closure (ffi_go_closure* closure, ffi_cif* cif,
 
 #endif /* FFI_GO_CLOSURES */
 
+#if defined(FFI_EXEC_STATIC_TRAMP)
+void *
+ffi_tramp_arch (size_t *tramp_size, size_t *map_size)
+{
+  extern void *trampoline_code_table_cet;
+  extern void *trampoline_code_table;
+  extern int ffi_cet_present;
+
+  *map_size = UNIX64_TRAMP_MAP_SIZE;
+  if (ffi_cet_present) {
+    *tramp_size = UNIX64_TRAMP_SIZE_CET;
+    return &trampoline_code_table_cet;
+  }
+  *tramp_size = UNIX64_TRAMP_SIZE;
+  return &trampoline_code_table;
+}
+#endif
+
 #endif /* __x86_64__ */
diff --git a/src/x86/ffiw64.c b/src/x86/ffiw64.c
index a43a9eb..df81d66 100644
--- a/src/x86/ffiw64.c
+++ b/src/x86/ffiw64.c
@@ -187,6 +187,7 @@ EFI64(ffi_call_go)(ffi_cif *cif, void (*fn)(void), void *rvalue,
 
 
 extern void ffi_closure_win64(void) FFI_HIDDEN;
+extern void ffi_closure_win64_alt(void) FFI_HIDDEN;
 
 #ifdef FFI_GO_CLOSURES
 extern void ffi_go_closure_win64(void) FFI_HIDDEN;
@@ -220,9 +221,18 @@ EFI64(ffi_prep_closure_loc)(ffi_closure* closure,
       return FFI_BAD_ABI;
     }
 
+  if (ffi_tramp_is_present(closure))
+    {
+      /* Initialize the static trampoline's parameters. */
+      ffi_tramp_set_parms (closure->ftramp, ffi_closure_win64_alt, closure);
+      goto out;
+    }
+
+  /* Initialize the dynamic trampoline. */
   memcpy (tramp, trampoline, sizeof(trampoline));
   *(UINT64 *)(tramp + sizeof (trampoline)) = (uintptr_t)ffi_closure_win64;
 
+out:
   closure->cif = cif;
   closure->fun = fun;
   closure->user_data = user_data;
diff --git a/src/x86/internal64.h b/src/x86/internal64.h
index 512e955..410bdf2 100644
--- a/src/x86/internal64.h
+++ b/src/x86/internal64.h
@@ -20,3 +20,14 @@
 #define UNIX64_FLAG_RET_IN_MEM	(1 << 10)
 #define UNIX64_FLAG_XMM_ARGS	(1 << 11)
 #define UNIX64_SIZE_SHIFT	12
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * For the trampoline code table mapping, a mapping size of 4K (base page size)
+ * is chosen.
+ */
+#define UNIX64_TRAMP_MAP_SHIFT	12
+#define UNIX64_TRAMP_MAP_SIZE	(1 << UNIX64_TRAMP_MAP_SHIFT)
+#define UNIX64_TRAMP_SIZE_CET	40
+#define UNIX64_TRAMP_SIZE	32
+#endif
diff --git a/src/x86/unix64.S b/src/x86/unix64.S
index 89d7db1..e26ea2c 100644
--- a/src/x86/unix64.S
+++ b/src/x86/unix64.S
@@ -63,6 +63,7 @@
 C(ffi_call_unix64):
 L(UW0):
 	_CET_ENDBR
+L(endbr):
 	movq	(%rsp), %r10		/* Load return address.  */
 	leaq	(%rdi, %rsi), %rax	/* Find local stack base.  */
 	movq	%rdx, (%rax)		/* Save flags.  */
@@ -270,6 +271,17 @@ L(UW6):
 L(UW7):
 ENDF(C(ffi_closure_unix64_sse))
 
+	.balign	2
+	.globl	C(ffi_closure_unix64_sse_alt)
+	FFI_HIDDEN(C(ffi_closure_unix64_sse_alt))
+
+C(ffi_closure_unix64_sse_alt):
+	_CET_ENDBR
+	movq	8(%rsp), %r10
+	addq	$16, %rsp
+	jmp	C(ffi_closure_unix64_sse)
+ENDF(C(ffi_closure_unix64_sse_alt))
+
 	.balign	2
 	.globl	C(ffi_closure_unix64)
 	FFI_HIDDEN(C(ffi_closure_unix64))
@@ -400,6 +412,17 @@ L(la):	call	PLT(C(abort))
 L(UW11):
 ENDF(C(ffi_closure_unix64))
 
+	.balign	8
+	.globl	C(ffi_closure_unix64_alt)
+	FFI_HIDDEN(C(ffi_closure_unix64_alt))
+
+C(ffi_closure_unix64_alt):
+	_CET_ENDBR
+	movq	8(%rsp), %r10
+	addq	$16, %rsp
+	jmp	C(ffi_closure_unix64)
+	ENDF(C(ffi_closure_unix64_alt))
+
 	.balign	2
 	.globl	C(ffi_go_closure_unix64_sse)
 	FFI_HIDDEN(C(ffi_go_closure_unix64_sse))
@@ -456,6 +479,80 @@ L(sse_entry2):
 L(UW17):
 ENDF(C(ffi_go_closure_unix64))
 
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * Below is the definition of the trampoline code table. Each element in
+ * the code table is a trampoline.
+ *
+ * Because we jump to the trampoline, we place a _CET_ENDBR at the
+ * beginning of the trampoline to mark it as a valid branch target. This is
+ * part of the the Intel CET (Control Flow Enforcement Technology).
+ *
+ * If CET is present, _CET_ENDBR is defined as the endbr64 instruction. Else,
+ * _CET_ENDBR is defined empty. Consequently, the size of the trampoline and
+ * the PC-relative offsets in the trampoline code also differ. So, two versions
+ * of the code table have been defined - one with the endbr64 instruction and
+ * one without. ffi_tramp_arch() figures out which version of the code table
+ * should be used by looking at ffi_cet_present (defined at the end of this
+ * file).
+ */
+/*
+ * The trampoline uses register r10. It saves the original value of r10 on
+ * the stack.
+ *
+ * The trampoline has two parameters - target code to jump to and data for
+ * the target code. The trampoline extracts the parameters from its parameter
+ * block (see tramp_table_map()). The trampoline saves the data address on
+ * the stack. Finally, it jumps to the target code.
+ *
+ * The target code can choose to:
+ *
+ * - restore the value of r10
+ * - load the data address in a register
+ * - restore the stack pointer to what it was when the trampoline was invoked.
+ */
+
+	.align	UNIX64_TRAMP_MAP_SIZE
+	.globl	trampoline_code_table_cet
+	FFI_HIDDEN(C(trampoline_code_table_cet))
+
+C(trampoline_code_table_cet):
+	.rept	UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE_CET
+	_CET_ENDBR
+	subq	$16, %rsp		/* Make space on the stack */
+	movq	%r10, (%rsp)		/* Save %r10 on stack */
+	movq	4077(%rip), %r10	/* Copy data into %r10 */
+	movq	%r10, 8(%rsp)		/* Save data on stack */
+	movq	4073(%rip), %r10	/* Copy code into %r10 */
+	jmp	*%r10			/* Jump to code */
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	.endr
+ENDF(C(trampoline_code_table_cet))
+
+	.align	UNIX64_TRAMP_MAP_SIZE
+	.globl	trampoline_code_table
+	FFI_HIDDEN(C(trampoline_code_table))
+
+C(trampoline_code_table):
+	.rept	UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE
+	subq	$16, %rsp		/* Make space on the stack */
+	movq	%r10, (%rsp)		/* Save %r10 on stack */
+	movq	4081(%rip), %r10	/* Copy data into %r10 */
+	movq	%r10, 8(%rsp)		/* Save data on stack */
+	movq	4077(%rip), %r10	/* Copy code into %r10 */
+	jmp	*%r10			/* Jump to code */
+	nop
+	nop
+	.endr
+ENDF(C(trampoline_code_table))
+	.align	UNIX64_TRAMP_MAP_SIZE
+#endif /* FFI_EXEC_STATIC_TRAMP */
+
 /* Sadly, OSX cctools-as doesn't understand .cfi directives at all.  */
 
 #ifdef __APPLE__
@@ -615,6 +712,13 @@ L(EFDE5):
 	.quad    0
 #endif
 
+	.section .rodata
+	.align 8
+	.globl ffi_cet_present
+ffi_cet_present:
+	.set	L6,L(endbr)-L(UW0)
+	.int	L6
+
 #endif /* __x86_64__ */
 #if defined __ELF__ && defined __linux__
 	.section	.note.GNU-stack,"",@progbits
diff --git a/src/x86/win64.S b/src/x86/win64.S
index 8315e8b..6ca3068 100644
--- a/src/x86/win64.S
+++ b/src/x86/win64.S
@@ -234,6 +234,18 @@ C(ffi_closure_win64):
 
 	cfi_endproc
 	SEH(.seh_endproc)
+
+	.align	8
+	.globl	C(ffi_closure_win64_alt)
+	FFI_HIDDEN(C(ffi_closure_win64_alt))
+
+	SEH(.seh_proc ffi_closure_win64_alt)
+C(ffi_closure_win64_alt):
+	_CET_ENDBR
+	movq	8(%rsp), %r10
+	addq	$16, %rsp
+	jmp	C(ffi_closure_win64)
+	SEH(.seh_endproc)
 #endif /* __x86_64__ */
 
 #if defined __ELF__ && defined __linux__
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v3 3/5] i386: Support for Static Trampolines
  2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
  2021-01-15 18:46   ` [RFC PATCH v3 1/5] " madvenka
  2021-01-15 18:46   ` [RFC PATCH v3 2/5] x86: Support for " madvenka
@ 2021-01-15 18:46   ` madvenka
  2021-01-15 18:46   ` [RFC PATCH v3 4/5] arm64: " madvenka
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: madvenka @ 2021-01-15 18:46 UTC (permalink / raw)
  To: libffi-discuss; +Cc: green, fweimer, dj, madvenka

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

	- Define the arch-specific initialization function ffi_tramp_arch ()
	  that returns trampoline size information to common code.

	- Define the trampoline code table statically. Define two tables,
	  actually, one with CET and one without.

	- Define the trampoline code table statically.

	- Introduce a tiny prolog for each ABI handling function. The ABI
	  handlers addressed are:

	  	- ffi_closure_i386
		- ffi_closure_STDCALL
		- ffi_closure_REGISTER

	  The prolog functions are called:

	  	- ffi_closure_i386_alt
		- ffi_closure_STDCALL_alt
		- ffi_closure_REGISTER_alt

	  The legacy trampoline jumps to the ABI handler. The static
	  trampoline jumps to the prolog function. The prolog function uses
	  the information provided by the static trampoline, sets things up
	  for the ABI handler and then jumps to the ABI handler.

	- Call ffi_closure_tramp_init () in ffi_prep_closure_loc () to
	  initialize static trampoline parameters.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
 src/x86/ffi.c      |  35 ++++++++++++++
 src/x86/internal.h |  11 +++++
 src/x86/sysv.S     | 116 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+)

diff --git a/src/x86/ffi.c b/src/x86/ffi.c
index 5f7fd81..dced0c8 100644
--- a/src/x86/ffi.c
+++ b/src/x86/ffi.c
@@ -409,8 +409,11 @@ ffi_call_go (ffi_cif *cif, void (*fn)(void), void *rvalue,
 /** private members **/
 
 void FFI_HIDDEN ffi_closure_i386(void);
+void FFI_HIDDEN ffi_closure_i386_alt(void);
 void FFI_HIDDEN ffi_closure_STDCALL(void);
+void FFI_HIDDEN ffi_closure_STDCALL_alt(void);
 void FFI_HIDDEN ffi_closure_REGISTER(void);
+void FFI_HIDDEN ffi_closure_REGISTER_alt(void);
 
 struct closure_frame
 {
@@ -537,6 +540,7 @@ ffi_prep_closure_loc (ffi_closure* closure,
 {
   char *tramp = closure->tramp;
   void (*dest)(void);
+  void (*dest_alt)(void);
   int op = 0xb8;  /* movl imm, %eax */
 
   switch (cif->abi)
@@ -546,19 +550,30 @@ ffi_prep_closure_loc (ffi_closure* closure,
     case FFI_FASTCALL:
     case FFI_MS_CDECL:
       dest = ffi_closure_i386;
+      dest_alt = ffi_closure_i386_alt;
       break;
     case FFI_STDCALL:
     case FFI_PASCAL:
       dest = ffi_closure_STDCALL;
+      dest_alt = ffi_closure_STDCALL_alt;
       break;
     case FFI_REGISTER:
       dest = ffi_closure_REGISTER;
+      dest_alt = ffi_closure_REGISTER_alt;
       op = 0x68;  /* pushl imm */
       break;
     default:
       return FFI_BAD_ABI;
     }
 
+  if (ffi_tramp_is_present(closure))
+    {
+      /* Initialize the static trampoline's parameters. */
+      ffi_tramp_set_parms (closure->ftramp, dest_alt, closure);
+      goto out;
+    }
+
+  /* Initialize the dynamic trampoline. */
   /* endbr32.  */
   *(UINT32 *) tramp = 0xfb1e0ff3;
 
@@ -570,6 +585,7 @@ ffi_prep_closure_loc (ffi_closure* closure,
   tramp[9] = 0xe9;
   *(unsigned *)(tramp + 10) = (unsigned)dest - ((unsigned)codeloc + 14);
 
+out:
   closure->cif = cif;
   closure->fun = fun;
   closure->user_data = user_data;
@@ -767,4 +783,23 @@ ffi_raw_call(ffi_cif *cif, void (*fn)(void), void *rvalue, ffi_raw *avalue)
   ffi_call_i386 (frame, stack);
 }
 #endif /* !FFI_NO_RAW_API */
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+void *
+ffi_tramp_arch (size_t *tramp_size, size_t *map_size)
+{
+  extern void *trampoline_code_table_cet;
+  extern void *trampoline_code_table;
+  extern int ffi_cet_present;
+
+  *map_size = X86_TRAMP_MAP_SIZE;
+  if (ffi_cet_present) {
+    *tramp_size = X86_TRAMP_SIZE_CET;
+    return &trampoline_code_table_cet;
+  }
+  *tramp_size = X86_TRAMP_SIZE;
+  return &trampoline_code_table;
+}
+#endif
+
 #endif /* __i386__ */
diff --git a/src/x86/internal.h b/src/x86/internal.h
index 09771ba..f648623 100644
--- a/src/x86/internal.h
+++ b/src/x86/internal.h
@@ -27,3 +27,14 @@
 #else
 # define HAVE_FASTCALL 1
 #endif
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * For the trampoline code table mapping, a mapping size of 4K (base page size)
+ * is chosen.
+ */
+#define X86_TRAMP_MAP_SHIFT	12
+#define X86_TRAMP_MAP_SIZE	(1 << X86_TRAMP_MAP_SHIFT)
+#define X86_TRAMP_SIZE_CET	44
+#define X86_TRAMP_SIZE		40
+#endif
diff --git a/src/x86/sysv.S b/src/x86/sysv.S
index d8ab4b0..09e9c4f 100644
--- a/src/x86/sysv.S
+++ b/src/x86/sysv.S
@@ -93,6 +93,7 @@ ffi_call_i386:
 L(UW0):
 	# cfi_startproc
 	_CET_ENDBR
+L(endbr):
 #if !HAVE_FASTCALL
 	movl	4(%esp), %ecx
 	movl	8(%esp), %edx
@@ -421,6 +422,16 @@ L(UW20):
 	# cfi_endproc
 ENDF(C(ffi_closure_i386))
 
+	.balign	16
+	.globl	C(ffi_closure_i386_alt)
+	FFI_HIDDEN(C(ffi_closure_i386_alt))
+C(ffi_closure_i386_alt):
+	_CET_ENDBR
+	movl	4(%esp), %eax
+	add	$8, %esp
+	jmp	C(ffi_closure_i386)
+ENDF(C(ffi_closure_i386_alt))
+
 	.balign	16
 	.globl	C(ffi_go_closure_STDCALL)
 	FFI_HIDDEN(C(ffi_go_closure_STDCALL))
@@ -466,6 +477,16 @@ L(UW26):
 	# cfi_endproc
 ENDF(C(ffi_closure_REGISTER))
 
+	.balign	16
+	.globl	C(ffi_closure_REGISTER_alt)
+	FFI_HIDDEN(C(ffi_closure_REGISTER_alt))
+C(ffi_closure_REGISTER_alt):
+	_CET_ENDBR
+	movl	(%esp), %eax
+	add	$4, %esp
+	jmp	C(ffi_closure_REGISTER)
+ENDF(C(ffi_closure_REGISTER_alt))
+
 /* For STDCALL (and others), we need to pop N bytes of arguments off
    the stack following the closure.  The amount needing to be popped
    is returned to us from ffi_closure_inner.  */
@@ -573,6 +594,93 @@ L(UW31):
 	# cfi_endproc
 ENDF(C(ffi_closure_STDCALL))
 
+	.balign	16
+	.globl	C(ffi_closure_STDCALL_alt)
+	FFI_HIDDEN(C(ffi_closure_STDCALL_alt))
+C(ffi_closure_STDCALL_alt):
+	_CET_ENDBR
+	movl	4(%esp), %eax
+	add	$8, %esp
+	jmp	C(ffi_closure_STDCALL)
+ENDF(C(ffi_closure_STDCALL_alt))
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * Below is the definition of the trampoline code table. Each element in
+ * the code table is a trampoline.
+ *
+ * Because we jump to the trampoline, we place a _CET_ENDBR at the
+ * beginning of the trampoline to mark it as a valid branch target. This is
+ * part of the the Intel CET (Control Flow Enforcement Technology).
+ *
+ * If CET is present, _CET_ENDBR is defined as the endbr32 instruction. Else,
+ * _CET_ENDBR is defined empty. Consequently, the size of the trampoline and
+ * the PC-relative offsets in the trampoline code also differ. So, two versions
+ * of the code table have been defined - one with the endbr32 instruction and
+ * one without. ffi_tramp_arch() figures out which version of the code table
+ * should be used by looking at ffi_cet_present (defined at the end of this
+ * file).
+ */
+/*
+ * The trampoline uses register eax.  It saves the original value of eax on
+ * the stack.
+ *
+ * The trampoline has two parameters - target code to jump to and data for
+ * the target code. The trampoline extracts the parameters from its parameter
+ * block (see tramp_table_map()). The trampoline saves the data address on
+ * the stack. Finally, it jumps to the target code.
+ *
+ * The target code can choose to:
+ *
+ * - restore the value of eax
+ * - load the data address in a register
+ * - restore the stack pointer to what it was when the trampoline was invoked.
+ */
+	.align	X86_TRAMP_MAP_SIZE
+	.globl	C(trampoline_code_table_cet)
+	FFI_HIDDEN(C(trampoline_code_table_cet))
+C(trampoline_code_table_cet):
+	.rept	X86_TRAMP_MAP_SIZE / X86_TRAMP_SIZE_CET
+	_CET_ENDBR
+	sub	$8, %esp
+	movl	%eax, (%esp)		/* Save %eax on stack */
+	call	1f			/* Get next PC into %eax */
+	movl	4081(%eax), %eax	/* Copy data into %eax */
+	movl	%eax, 4(%esp)		/* Save data on stack */
+	call	1f			/* Get next PC into %eax */
+	movl	4070(%eax), %eax	/* Copy data into %eax */
+	jmp	*%eax			/* Jump to code */
+1:
+	mov	(%esp), %eax
+	ret
+	nop				/* Pad to 4 byte boundary */
+	nop
+	.endr
+ENDF(C(trampoline_code_table_cet))
+
+	.align	X86_TRAMP_MAP_SIZE
+	.globl	C(trampoline_code_table)
+	FFI_HIDDEN(C(trampoline_code_table))
+C(trampoline_code_table):
+	.rept	X86_TRAMP_MAP_SIZE / X86_TRAMP_SIZE
+	sub	$8, %esp
+	movl	%eax, (%esp)		/* Save %eax on stack */
+	call	1f			/* Get next PC into %eax */
+	movl	4085(%eax), %eax	/* Copy data into %eax */
+	movl	%eax, 4(%esp)		/* Save data on stack */
+	call	1f			/* Get next PC into %eax */
+	movl	4074(%eax), %eax	/* Copy data into %eax */
+	jmp	*%eax			/* Jump to code */
+1:
+	mov	(%esp), %eax
+	ret
+	nop				/* Pad to 4 byte boundary */
+	nop
+	.endr
+ENDF(C(trampoline_code_table))
+	.align	X86_TRAMP_MAP_SIZE
+#endif /* FFI_EXEC_STATIC_TRAMP */
+
 #if !FFI_NO_RAW_API
 
 #define raw_closure_S_FS	(16+16+12)
@@ -1131,6 +1239,14 @@ L(EFDE9):
 #endif /* __APPLE__ */
 
 #endif /* ifndef _MSC_VER */
+
+	.section .rodata
+	.align 8
+	.globl ffi_cet_present
+ffi_cet_present:
+	.set	L10,L(endbr)-L(UW0)
+	.int	L10
+
 #endif /* ifdef __i386__ */
 
 #if defined __ELF__ && defined __linux__
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v3 4/5] arm64: Support for Static Trampolines
  2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
                     ` (2 preceding siblings ...)
  2021-01-15 18:46   ` [RFC PATCH v3 3/5] i386: " madvenka
@ 2021-01-15 18:46   ` madvenka
  2021-01-15 18:46   ` [RFC PATCH v3 5/5] arm: " madvenka
  2021-01-26 23:41   ` [RFC PATCH v3 0/5] Libffi " Anthony Green
  5 siblings, 0 replies; 30+ messages in thread
From: madvenka @ 2021-01-15 18:46 UTC (permalink / raw)
  To: libffi-discuss; +Cc: green, fweimer, dj, madvenka

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

	- Define the arch-specific initialization function ffi_tramp_arch ()
	  that returns trampoline size information to common code.

	- Define the trampoline code mapping and data mapping sizes.

	- Define the trampoline code table statically.

	- Introduce a tiny prolog for each ABI handling function. The ABI
	  handlers addressed are:

	  	- ffi_closure_SYSV
		- ffi_closure_SYSV_V

	  The prolog functions are called:

	  	- ffi_closure_SYSV_alt
		- ffi_closure_SYSV_V_alt

	  The legacy trampoline jumps to the ABI handler. The static
	  trampoline jumps to the prolog function. The prolog function uses
	  the information provided by the static trampoline, sets things up
	  for the ABI handler and then jumps to the ABI handler.

	- Call ffi_closure_tramp_init () in ffi_prep_closure_loc () to
	  initialize static trampoline parameters.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
 src/aarch64/ffi.c      | 36 ++++++++++++++++++++--
 src/aarch64/internal.h | 10 +++++++
 src/aarch64/sysv.S     | 68 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 3 deletions(-)

diff --git a/src/aarch64/ffi.c b/src/aarch64/ffi.c
index ef09f4d..0d7d8ac 100644
--- a/src/aarch64/ffi.c
+++ b/src/aarch64/ffi.c
@@ -781,7 +781,9 @@ ffi_call_go (ffi_cif *cif, void (*fn) (void), void *rvalue,
 /* Build a trampoline.  */
 
 extern void ffi_closure_SYSV (void) FFI_HIDDEN;
+extern void ffi_closure_SYSV_alt (void) FFI_HIDDEN;
 extern void ffi_closure_SYSV_V (void) FFI_HIDDEN;
+extern void ffi_closure_SYSV_V_alt (void) FFI_HIDDEN;
 
 ffi_status
 ffi_prep_closure_loc (ffi_closure *closure,
@@ -794,11 +796,18 @@ ffi_prep_closure_loc (ffi_closure *closure,
     return FFI_BAD_ABI;
 
   void (*start)(void);
+  void (*start_alt)(void);
   
   if (cif->flags & AARCH64_FLAG_ARG_V)
-    start = ffi_closure_SYSV_V;
+    {
+      start = ffi_closure_SYSV_V;
+      start_alt = ffi_closure_SYSV_V_alt;
+    }
   else
-    start = ffi_closure_SYSV;
+    {
+      start = ffi_closure_SYSV;
+      start_alt = ffi_closure_SYSV_alt;
+    }
 
 #if FFI_EXEC_TRAMPOLINE_TABLE
 #ifdef __MACH__
@@ -816,7 +825,15 @@ ffi_prep_closure_loc (ffi_closure *closure,
     0x00, 0x02, 0x1f, 0xd6	/* br	x16		*/
   };
   char *tramp = closure->tramp;
-  
+
+  if (ffi_tramp_is_present(closure))
+    {
+      /* Initialize the static trampoline's parameters. */
+      ffi_tramp_set_parms (closure->ftramp, start_alt, closure);
+      goto out;
+    }
+
+  /* Initialize the dynamic trampoline. */
   memcpy (tramp, trampoline, sizeof(trampoline));
   
   *(UINT64 *)(tramp + 16) = (uintptr_t)start;
@@ -832,6 +849,7 @@ ffi_prep_closure_loc (ffi_closure *closure,
   unsigned char *tramp_code = ffi_data_to_code_pointer (tramp);
   #endif
   ffi_clear_cache (tramp_code, tramp_code + FFI_TRAMPOLINE_SIZE);
+out:
 #endif
 
   closure->cif = cif;
@@ -1022,4 +1040,16 @@ ffi_closure_SYSV_inner (ffi_cif *cif,
   return flags;
 }
 
+#if defined(FFI_EXEC_STATIC_TRAMP)
+void *
+ffi_tramp_arch (size_t *tramp_size, size_t *map_size)
+{
+  extern void *trampoline_code_table;
+
+  *tramp_size = AARCH64_TRAMP_SIZE;
+  *map_size = AARCH64_TRAMP_MAP_SIZE;
+  return &trampoline_code_table;
+}
+#endif
+
 #endif /* (__aarch64__) || defined(__arm64__)|| defined (_M_ARM64)*/
diff --git a/src/aarch64/internal.h b/src/aarch64/internal.h
index 3d4d035..de55755 100644
--- a/src/aarch64/internal.h
+++ b/src/aarch64/internal.h
@@ -66,3 +66,13 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.  */
 #define N_X_ARG_REG		8
 #define N_V_ARG_REG		8
 #define CALL_CONTEXT_SIZE	(N_V_ARG_REG * 16 + N_X_ARG_REG * 8)
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * For the trampoline code table mapping, a mapping size of 16K is chosen to
+ * cover the base page sizes of 4K and 16K.
+ */
+#define AARCH64_TRAMP_MAP_SHIFT	14
+#define AARCH64_TRAMP_MAP_SIZE	(1 << AARCH64_TRAMP_MAP_SHIFT)
+#define AARCH64_TRAMP_SIZE	32
+#endif
diff --git a/src/aarch64/sysv.S b/src/aarch64/sysv.S
index b720a92..a7ba3cc 100644
--- a/src/aarch64/sysv.S
+++ b/src/aarch64/sysv.S
@@ -252,6 +252,19 @@ CNAME(ffi_closure_SYSV_V):
 	.size	CNAME(ffi_closure_SYSV_V), . - CNAME(ffi_closure_SYSV_V)
 #endif
 
+	.align 4
+CNAME(ffi_closure_SYSV_V_alt):
+	ldr	x17, [sp, #8]
+	add	sp, sp, #16
+	b	CNAME(ffi_closure_SYSV_V)
+
+	.globl	CNAME(ffi_closure_SYSV_V_alt)
+	FFI_HIDDEN(CNAME(ffi_closure_SYSV_V_alt))
+#ifdef __ELF__
+	.type	CNAME(ffi_closure_SYSV_V_alt), #function
+	.size	CNAME(ffi_closure_SYSV_V_alt), . - CNAME(ffi_closure_SYSV_V_alt)
+#endif
+
 	.align	4
 	cfi_startproc
 CNAME(ffi_closure_SYSV):
@@ -367,6 +380,61 @@ CNAME(ffi_closure_SYSV):
 	.size	CNAME(ffi_closure_SYSV), . - CNAME(ffi_closure_SYSV)
 #endif
 
+	.align 4
+CNAME(ffi_closure_SYSV_alt):
+	ldr	x17, [sp, #8]
+	add	sp, sp, #16
+	b	CNAME(ffi_closure_SYSV)
+
+	.globl	CNAME(ffi_closure_SYSV_alt)
+	FFI_HIDDEN(CNAME(ffi_closure_SYSV_alt))
+#ifdef __ELF__
+	.type	CNAME(ffi_closure_SYSV_alt), #function
+	.size	CNAME(ffi_closure_SYSV_alt), . - CNAME(ffi_closure_SYSV_alt)
+#endif
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * Below is the definition of the trampoline code table. Each element in
+ * the code table is a trampoline.
+ */
+/*
+ * The trampoline uses register x17. It saves the original value of x17 on
+ * the stack.
+ *
+ * The trampoline has two parameters - target code to jump to and data for
+ * the target code. The trampoline extracts the parameters from its parameter
+ * block (see tramp_table_map()). The trampoline saves the data address on
+ * the stack. Finally, it jumps to the target code.
+ *
+ * The target code can choose to:
+ *
+ * - restore the value of x17
+ * - load the data address in a register
+ * - restore the stack pointer to what it was when the trampoline was invoked.
+ */
+	.align	AARCH64_TRAMP_MAP_SHIFT
+CNAME(trampoline_code_table):
+	.rept	AARCH64_TRAMP_MAP_SIZE / AARCH64_TRAMP_SIZE
+	sub	sp, sp, #16		/* Make space on the stack */
+	str	x17, [sp]		/* Save x17 on stack */
+	adr	x17, #16376		/* Get data address */
+	ldr	x17, [x17]		/* Copy data into x17 */
+	str	x17, [sp, #8]		/* Save data on stack */
+	adr	x17, #16372		/* Get code address */
+	ldr	x17, [x17]		/* Load code address into x17 */
+	br	x17			/* Jump to code */
+	.endr
+
+	.globl CNAME(trampoline_code_table)
+	FFI_HIDDEN(CNAME(trampoline_code_table))
+#ifdef __ELF__
+	.type	CNAME(trampoline_code_table), #function
+	.size	CNAME(trampoline_code_table), . - CNAME(trampoline_code_table)
+#endif
+	.align	AARCH64_TRAMP_MAP_SHIFT
+#endif /* FFI_EXEC_STATIC_TRAMP */
+
 #if FFI_EXEC_TRAMPOLINE_TABLE
 
 #ifdef __MACH__
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v3 5/5] arm: Support for Static Trampolines
  2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
                     ` (3 preceding siblings ...)
  2021-01-15 18:46   ` [RFC PATCH v3 4/5] arm64: " madvenka
@ 2021-01-15 18:46   ` madvenka
  2021-01-26 23:41   ` [RFC PATCH v3 0/5] Libffi " Anthony Green
  5 siblings, 0 replies; 30+ messages in thread
From: madvenka @ 2021-01-15 18:46 UTC (permalink / raw)
  To: libffi-discuss; +Cc: green, fweimer, dj, madvenka

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

	- Define the arch-specific initialization function ffi_tramp_arch ()
	  that returns trampoline size information to common code.

	- Define the trampoline code mapping and data mapping sizes.

	- Define the trampoline code table statically.

	- Introduce a tiny prolog for each ABI handling function. The ABI
	  handlers addressed are:

	  	- ffi_closure_SYSV
		- ffi_closure_VFP

	  The prolog functions are called:

	  	- ffi_closure_SYSV_alt
		- ffi_closure_VFP_alt

	  The legacy trampoline jumps to the ABI handler. The static
	  trampoline jumps to the prolog function. The prolog function uses
	  the information provided by the static trampoline, sets things up
	  for the ABI handler and then jumps to the ABI handler.

	- Call ffi_closure_tramp_init () in ffi_prep_closure_loc () to
	  initialize static trampoline parameters.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
 src/arm/ffi.c      | 29 ++++++++++++++++++++++++++++-
 src/arm/internal.h | 10 ++++++++++
 src/arm/sysv.S     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/src/arm/ffi.c b/src/arm/ffi.c
index 0058390..7e3cf1a 100644
--- a/src/arm/ffi.c
+++ b/src/arm/ffi.c
@@ -570,7 +570,9 @@ ffi_closure_inner_VFP (ffi_cif *cif,
 }
 
 void ffi_closure_SYSV (void) FFI_HIDDEN;
+void ffi_closure_SYSV_alt (void) FFI_HIDDEN;
 void ffi_closure_VFP (void) FFI_HIDDEN;
+void ffi_closure_VFP_alt (void) FFI_HIDDEN;
 
 #ifdef FFI_GO_CLOSURES
 void ffi_go_closure_SYSV (void) FFI_HIDDEN;
@@ -596,12 +598,16 @@ ffi_prep_closure_loc (ffi_closure * closure,
 		      void *user_data, void *codeloc)
 {
   void (*closure_func) (void) = ffi_closure_SYSV;
+  void (*closure_func_alt) (void) = ffi_closure_SYSV_alt;
 
   if (cif->abi == FFI_VFP)
     {
       /* We only need take the vfp path if there are vfp arguments.  */
       if (cif->vfp_used)
-	closure_func = ffi_closure_VFP;
+	{
+	  closure_func = ffi_closure_VFP;
+	  closure_func_alt = ffi_closure_VFP_alt;
+	}
     }
   else if (cif->abi != FFI_SYSV)
     return FFI_BAD_ABI;
@@ -612,6 +618,14 @@ ffi_prep_closure_loc (ffi_closure * closure,
   config[1] = closure_func;
 #else
 
+  if (ffi_tramp_is_present(closure))
+    {
+      /* Initialize the static trampoline's parameters. */
+      ffi_tramp_set_parms (closure->ftramp, closure_func_alt, closure);
+      goto out;
+    }
+
+  /* Initialize the dynamic trampoline. */
 #ifndef _M_ARM
   memcpy(closure->tramp, ffi_arm_trampoline, 8);
 #else
@@ -633,6 +647,7 @@ ffi_prep_closure_loc (ffi_closure * closure,
 #else
   *(void (**)(void))(closure->tramp + 8) = closure_func;
 #endif
+out:
 #endif
 
   closure->cif = cif;
@@ -873,4 +888,16 @@ layout_vfp_args (ffi_cif * cif)
     }
 }
 
+#if defined(FFI_EXEC_STATIC_TRAMP)
+void *
+ffi_tramp_arch (size_t *tramp_size, size_t *map_size)
+{
+  extern void *trampoline_code_table;
+
+  *tramp_size = ARM_TRAMP_SIZE;
+  *map_size = ARM_TRAMP_MAP_SIZE;
+  return &trampoline_code_table;
+}
+#endif
+
 #endif /* __arm__ or _M_ARM */
diff --git a/src/arm/internal.h b/src/arm/internal.h
index 6cf0b2a..fa8ab0b 100644
--- a/src/arm/internal.h
+++ b/src/arm/internal.h
@@ -5,3 +5,13 @@
 #define ARM_TYPE_INT	4
 #define ARM_TYPE_VOID	5
 #define ARM_TYPE_STRUCT	6
+
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * For the trampoline table mapping, a mapping size of 4K (base page size)
+ * is chosen.
+ */
+#define ARM_TRAMP_MAP_SHIFT	12
+#define ARM_TRAMP_MAP_SIZE	(1 << ARM_TRAMP_MAP_SHIFT)
+#define ARM_TRAMP_SIZE		20
+#endif
diff --git a/src/arm/sysv.S b/src/arm/sysv.S
index 74bc53f..ea39465 100644
--- a/src/arm/sysv.S
+++ b/src/arm/sysv.S
@@ -260,6 +260,12 @@ ARM_FUNC_START(ffi_closure_SYSV)
 	UNWIND(.fnend)
 ARM_FUNC_END(ffi_closure_SYSV)
 
+ARM_FUNC_START(ffi_closure_SYSV_alt)
+	ldr	ip, [sp, #4]
+	add	sp, sp, 8
+	b	CNAME(ffi_closure_SYSV)
+ARM_FUNC_END(ffi_closure_SYSV_alt)
+
 ARM_FUNC_START(ffi_go_closure_VFP)
 	cfi_startproc
 	stmdb	sp!, {r0-r3}			@ save argument regs
@@ -310,6 +316,12 @@ ARM_FUNC_START(ffi_closure_VFP)
 	UNWIND(.fnend)
 ARM_FUNC_END(ffi_closure_VFP)
 
+ARM_FUNC_START(ffi_closure_VFP_alt)
+	ldr	ip, [sp, #4]
+	add	sp, sp, 8
+	b	CNAME(ffi_closure_VFP)
+ARM_FUNC_END(ffi_closure_VFP_alt)
+
 /* Load values returned in registers for both closure entry points.
    Note that we use LDM with SP in the register set.  This is deprecated
    by ARM, but not yet unpredictable.  */
@@ -354,6 +366,39 @@ E(ARM_TYPE_STRUCT)
 	cfi_endproc
 ARM_FUNC_END(ffi_closure_ret)
 
+#if defined(FFI_EXEC_STATIC_TRAMP)
+/*
+ * Below is the definition of the trampoline code table. Each element in
+ * the code table is a trampoline.
+ */
+/*
+ * The trampoline uses register ip (r12). It saves the original value of ip
+ * on the stack.
+ *
+ * The trampoline has two parameters - target code to jump to and data for
+ * the target code. The trampoline extracts the parameters from its parameter
+ * block (see tramp_table_map()). The trampoline saves the data address on
+ * the stack. Finally, it jumps to the target code.
+ *
+ * The target code can choose to:
+ *
+ * - restore the value of ip
+ * - load the data address in a register
+ * - restore the stack pointer to what it was when the trampoline was invoked.
+ */
+	.align	ARM_TRAMP_MAP_SHIFT
+ARM_FUNC_START(trampoline_code_table)
+	.rept	ARM_TRAMP_MAP_SIZE / ARM_TRAMP_SIZE
+	sub	sp, sp, #8		/* Make space on the stack */
+	str	ip, [sp]		/* Save ip on stack */
+	ldr	ip, [pc, #4080]		/* Copy data into ip */
+	str	ip, [sp, #4]		/* Save data on stack */
+	ldr	pc, [pc, #4076]		/* Copy code into PC */
+	.endr
+ARM_FUNC_END(trampoline_code_table)
+	.align	ARM_TRAMP_MAP_SHIFT
+#endif /* FFI_EXEC_STATIC_TRAMP */
+
 #if FFI_EXEC_TRAMPOLINE_TABLE
 
 #ifdef __MACH__
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
                     ` (4 preceding siblings ...)
  2021-01-15 18:46   ` [RFC PATCH v3 5/5] arm: " madvenka
@ 2021-01-26 23:41   ` Anthony Green
  2021-01-27 17:20     ` Madhavan T. Venkataraman
  5 siblings, 1 reply; 30+ messages in thread
From: Anthony Green @ 2021-01-26 23:41 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss, fweimer, DJ Delorie

Madhaven,

  Thank you for your continued efforts here.

On Fri, Jan 15, 2021 at 1:47 PM <madvenka@linux.microsoft.com> wrote:

> Trampoline API
> ==============
>
> There is a lot of dynamic code out there. They all have the same security
> issue. Dynamic code can be re-written into static code provided the data
> required by the static code can be passed to it just like we pass the
> closure
> pointer to an ABI handler.
>
> So, the same trampoline functions used by libffi internally need to be
> made available to the rest of the world in the form of an API. The
> following API has been defined in this solution:
>


I need a better explanation around why this new API is required, and this
new capability isn't just an implementation detail supporting the old API.

Thanks,

AG

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 1/5] Libffi Static Trampolines
  2021-01-15 18:46   ` [RFC PATCH v3 1/5] " madvenka
@ 2021-01-27  3:31     ` DJ Delorie
  2021-01-27 21:51       ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: DJ Delorie @ 2021-01-27  3:31 UTC (permalink / raw)
  To: madvenka; +Cc: libffi-discuss


I've reviewed these patches from the point of view of the details -
comments, constructs, etc.  Whether the overall design is good I leave
to Anthony ;-)

I only reviewed the x86-64 port, the others looked similar enough that
my rough comments may apply.

madvenka@linux.microsoft.com writes:

> -		src/raw_api.c src/java_raw_api.c src/closures.c
> +		src/raw_api.c src/java_raw_api.c src/closures.c \
> +		src/tramp.c

Ok

> -AC_CHECK_FUNCS([mmap mkostemp])
> +AC_CHECK_FUNCS([mmap mkostemp mkstemp])

Note that src/closure.c has code for finding a temp file that has EXEC
permission.  A bit of code sharing would be a good future update ;-)

> +case "$target" in
> +     *-linux*)
> +       AC_DEFINE(FFI_EXEC_STATIC_TRAMP, 1,
> +                 [Define this if you want statically defined trampolines])
> +     ;;
> +esac

Ok.  Might want to conditional for the arches that are supported too.

> -  char tramp[FFI_TRAMPOLINE_SIZE];
> +  union {
> +    char tramp[FFI_TRAMPOLINE_SIZE];
> +    void *ftramp;
> +  };

Ok.

> +/* ---- Static Trampoline Definitions -------------------------------------- */
> +
> +FFI_API int ffi_tramp_is_supported(void);
> +FFI_API void *ffi_tramp_alloc (int flags);
> +FFI_API int ffi_tramp_set_parms (void *tramp, void *data, void *code);
> +FFI_API void *ffi_tramp_get_addr (void *tramp);
> +FFI_API void ffi_tramp_free (void *tramp);

Are we assuming that applications will have to be modified to use this
new functionality?  It looks like it's used internally for the existing
API but is it intended to be used directly also?

> +/* The arch code calls this to determine if a given closure has a
> +   static trampoline. */
> +int ffi_tramp_is_present (void *closure);

Ok

> +
> +LIBFFI_STATIC_TRAMP_8.0 {
> +  global:
> +	ffi_tramp_is_supported;
> +	ffi_tramp_alloc;
> +	ffi_tramp_set_parms;
> +	ffi_tramp_get_addr;
> +	ffi_tramp_free;
> +} LIBFFI_BASE_8.0;

This is missing a suitable #if/#endif around it

> +
> +int
> +ffi_tramp_is_present (__attribute__((unused)) void *ptr)
> +{
> +  return 0;
> +}
>  #else /* !NetBSD with PROT_MPROTECT */

Ok.

>  #if !FFI_MMAP_EXEC_WRIT && !FFI_EXEC_TRAMPOLINE_TABLE
> @@ -843,6 +849,12 @@ dlmmap (void *start, size_t length, int prot,
>  	  && flags == (MAP_PRIVATE | MAP_ANONYMOUS)
>  	  && fd == -1 && offset == 0);
>  
> +  if (execfd == -1 && ffi_tramp_is_supported ())
> +    {
> +      ptr = mmap (start, length, prot & ~PROT_EXEC, flags, fd, offset);
> +      return ptr;
> +    }
> +

Should check for error, but not all existing cases do.  Do we expect to
allow a fallback when static trampolines are enabled?

> @@ -922,7 +934,7 @@ segment_holding_code (mstate m, char* addr)
>  void *
>  ffi_closure_alloc (size_t size, void **code)
>  {
> -  void *ptr;
> +  void *ptr, *ftramp;

Ok.

> @@ -934,6 +946,17 @@ ffi_closure_alloc (size_t size, void **code)
>        msegmentptr seg = segment_holding (gm, ptr);
>  
>        *code = add_segment_exec_offset (ptr, seg);
> +      if (!ffi_tramp_is_supported ())
> +        return ptr;
> +
> +      ftramp = ffi_tramp_alloc (0);
> +      if (ftramp == NULL)
> +      {
> +        dlfree (FFI_RESTORE_PTR (ptr));
> +        return NULL;
> +      }
> +      *code = ffi_tramp_get_addr (ftramp);
> +      ((ffi_closure *) ptr)->ftramp = ftramp;
>      }

Ok.

> @@ -943,12 +966,17 @@ void *
>  ffi_data_to_code_pointer (void *data)
>  {
>    msegmentptr seg = segment_holding (gm, data);
> +

Extraneous but ok.

>    /* We expect closures to be allocated with ffi_closure_alloc(), in
>       which case seg will be non-NULL.  However, some users take on the
>       burden of managing this memory themselves, in which case this
>       we'll just return data. */
>    if (seg)
> -    return add_segment_exec_offset (data, seg);
> +    {
> +      if (!ffi_tramp_is_supported ())
> +        return add_segment_exec_offset (data, seg);
> +      return ffi_tramp_get_addr (((ffi_closure *) data)->ftramp);
> +    }
>    else
>      return data;

Ok

> +  if (ffi_tramp_is_supported ())
> +    ffi_tramp_free (((ffi_closure *) ptr)->ftramp);

Ok.

> +int
> +ffi_tramp_is_present (void *ptr)
> +{
> +  msegmentptr seg = segment_holding (gm, ptr);
> +  return seg != NULL && ffi_tramp_is_supported();
> +}
> +

Ok.

> +int
> +ffi_tramp_is_present (__attribute__((unused)) void *ptr)
> +{
> +  return 0;
> +}
> +
>  # endif /* ! FFI_MMAP_EXEC_WRIT */
>  #endif /* FFI_CLOSURES */

Ok.

> diff --git a/src/tramp.c b/src/tramp.c
> new file mode 100644
> index 0000000..4151cc9
> --- /dev/null
> +++ b/src/tramp.c
> @@ -0,0 +1,718 @@
> +/* -----------------------------------------------------------------------
> +   tramp.c - Copyright (c) 2020 Madhavan T. Venkataraman

I'll leave it up to Anthony to decide on copyright issues

> +   API and support functions for managing statically defined closure
> +   trampolines.
> +
> +   Permission is hereby granted, free of charge, to any person obtaining
> +   a copy of this software and associated documentation files (the
> +   ``Software''), to deal in the Software without restriction, including
> +   without limitation the rights to use, copy, modify, merge, publish,
> +   distribute, sublicense, and/or sell copies of the Software, and to
> +   permit persons to whom the Software is furnished to do so, subject to
> +   the following conditions:
> +
> +   The above copyright notice and this permission notice shall be included
> +   in all copies or substantial portions of the Software.
> +
> +   THE SOFTWARE IS PROVIDED ``AS IS'', WITHOUT WARRANTY OF ANY KIND,
> +   EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +   MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> +   NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> +   HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> +   WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +   OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +   DEALINGS IN THE SOFTWARE.
> +   ----------------------------------------------------------------------- */
> +
> +#include <fficonfig.h>
> +
> +#ifdef FFI_EXEC_STATIC_TRAMP
> +
> +/* -------------------------- Headers and Definitions ---------------------*/
> +/*
> + * Add support for other OSes later. For now, it is just Linux.
> + */

This should be handled in configure, and this whole file #if'd based on
those results.

> +#if defined __linux__
> +#ifdef __linux__
> +#define _GNU_SOURCE 1
> +#endif
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <fcntl.h>
> +#include <pthread.h>
> +#include <sys/mman.h>
> +#ifdef __linux__
> +#include <linux/limits.h>
> +#include <linux/types.h>
> +#endif
> +#endif /* __linux__ */

Ok

> +/*
> + * Each architecture defines static code for a trampoline code table. The
> + * trampoline code table is mapped into the address space of a process.
> + *
> + * The following architecture specific function returns:
> + *
> + *	- the address of the trampoline code table in the text segment
> + *	- the size of each trampoline in the trampoline code table
> + *	- the size of the mapping for the whole trampoline code table
> + */
> +void __attribute__((weak)) *ffi_tramp_arch (size_t *tramp_size,
> +  size_t *map_size);

Should this be extern?

> +/* ------------------------- Trampoline Data Structures --------------------*/
> +
> +struct tramp;
> +
> +/*
> + * Trampoline table. Manages one trampoline code table and one trampoline
> + * parameter table.
> + *
> + * prev, next	Links in the global trampoline table list.
> + * code_table	Trampoline code table mapping.
> + * parm_table	Trampoline parameter table mapping.
> + * array	Array of trampolines malloced.
> + * free		List of free trampolines.
> + * nfree	Number of free trampolines.
> + */
> +struct tramp_table
> +{
> +  struct tramp_table *prev;
> +  struct tramp_table *next;
> +  void *code_table;
> +  void *parm_table;
> +  struct tramp *array;
> +  struct tramp *free;
> +  int nfree;
> +};
> +
> +/*
> + * Parameters for each trampoline.
> + *
> + * data
> + *	Data for the target code that the trampoline jumps to.
> + * target
> + *	Target code that the trampoline jumps to.
> + */
> +struct tramp_parm
> +{
> +  void *data;
> +  void *target;
> +};
> +
> +/*
> + * Trampoline structure for each trampoline.
> + *
> + * prev, next	Links in the trampoline free list of a trampoline table.
> + * table	Trampoline table to which this trampoline belongs.
> + * code		Address of this trampoline in the code table mapping.
> + * parm		Address of this trampoline's parameters in the parameter
> + *		table mapping.
> + */
> +struct tramp
> +{
> +  struct tramp *prev;
> +  struct tramp *next;
> +  struct tramp_table *table;
> +  void *code;
> +  struct tramp_parm *parm;
> +};
> +
> +enum gtramp_status {
> +	GTRAMP_UNINITIALIZED = 0,
> +	GTRAMP_PASSED,
> +	GTRAMP_FAILED,
> +};

Why is this one suddently called "gtramp" instead of "tramp" ?
Needs a comment.  Possibly rename to "global_tramp_status"

> +/*
> + * Trampoline globals.
> + *
> + * fd
> + *	File descriptor of binary file that contains the trampoline code table.
> + * offset
> + *	Offset of the trampoline code table in that file.
> + * text
> + *	Address of the trampoline code table in the text segment.
> + * map_size
> + *	Size of the trampoline code table mapping.
> + * size
> + *	Size of one trampoline in the trampoline code table.
> + * ntramp
> + *	Total number of trampolines in the trampoline code table.
> + * tables
> + *	List of trampoline tables that contain free trampolines.
> + * ntables
> + *	Number of trampoline tables that contain free trampolines.

Perhaps name this "n_nonfree_tables" ?  Based on the name, I would
assume it was "number of tables".


> + * status
> + *	Initialization status.
> + */
> +struct tramp_global
> +{
> +  int fd;
> +  off_t offset;
> +  void *text;
> +  size_t map_size;
> +  size_t size;
> +  int ntramp;
> +  struct tramp_table *tables;
> +  int ntables;
> +  enum gtramp_status status;
> +};
> +
> +static struct tramp_global gtramp;

Ok, but I'd prefer a more descriptive name than "gtramp"

I'll note that these are initialized to 0 but code below sets them to -1
on error; I assume this is intentionally different.

> +/* --------------------- Trampoline File Initialization --------------------*/
> +
> +/*
> + * The trampoline file is the file used to map the trampoline code table into
> + * the address space of a process. There are two ways to get this file:
> + *
> + * - From the OS. E.g., on Linux, /proc/<pid>/maps lists all the memory
> + *   mappings for <pid>. For file-backed mappings, maps supplies the file name
> + *   and the file offset. Using this, we can locate the mapping that maps
> + *   libffi and get the path to the libffi binary. And, we can compute the
> + *   offset of the trampoline code table within that binary.
> + *
> + * - Else, if we can create a temporary file, we can write the trampoline code
> + *   table from the text segment into the temporary file.
> + *
> + * The first method is the preferred one. If the OS security subsystem
> + * disallows mapping unsigned files with PROT_EXEC, then the second method
> + * will fail.
> + *
> + * If an OS allows the trampoline code table in the text segment to be
> + * directly remapped (e.g., MACH vm_remap ()), then we don't need the
> + * trampoline file.
> + */
> +static int tramp_table_alloc (void);
> +
> +#if defined __linux__

Ok

> +static int
> +ffi_tramp_get_libffi (void)
> +{
> +  FILE *fp;
> +  char file[PATH_MAX], line[PATH_MAX+100], perm[10], dev[10];
> +  unsigned long start, end, offset, inode;
> +  uintptr_t addr = (uintptr_t) gtramp.text;
> +  int nfields, found;
> +
> +  snprintf (file, PATH_MAX, "/proc/%d/maps", getpid());
> +  fp = fopen (file, "r");
> +  if (fp == NULL)
> +    return 0;
> +
> +  found = 0;
> +  while (feof (fp) == 0) {
> +    if (fgets (line, sizeof (line), fp) == 0)
> +      break;
> +
> +    nfields = sscanf (line, "%lx-%lx %s %lx %s %ld %s",
> +      &start, &end, perm, &offset, dev, &inode, file);

Should use field limiting for perm and dev, like %9s.  Yes, I know
/proc/pid/maps gives fixed fields, but plan for paranoia ;-)

> +    if (nfields != 7)
> +      continue;
> +
> +    if (addr >= start && addr < end) {
> +      gtramp.offset = offset + (addr - start);
> +      found = 1;
> +      break;
> +    }
> +  }
> +  fclose (fp);
> +
> +  if (!found)
> +    return 0;
> +
> +  gtramp.fd = open (file, O_RDONLY);
> +  if (gtramp.fd == -1)
> +    return 0;
> +
> +  /*
> +   * Allocate a trampoline table just to make sure that the trampoline code
> +   * table can be mapped.
> +   */
> +  if (!tramp_table_alloc ())
> +    {
> +      close (gtramp.fd);
> +      gtramp.fd = -1;
> +      return 0;
> +    }
> +  return 1;
> +}

Ok.

> +#endif /* __linux__ */
> +
> +#if defined __linux__

Messy conditionals

> +#if defined HAVE_MKSTEMP
> +
> +static int
> +ffi_tramp_get_temp_file (void)
> +{
> +  char template[12] = "/tmp/XXXXXX";
> +  ssize_t count;
> +
> +  gtramp.offset = 0;
> +  gtramp.fd = mkstemp (template);
> +  if (gtramp.fd == -1)
> +    return 0;
> +
> +  unlink (template);
> +  /*
> +   * Write the trampoline code table into the temporary file and allocate a
> +   * trampoline table to make sure that the temporary file can be mapped.
> +   */
> +  count = write(gtramp.fd, gtramp.text, gtramp.map_size);
> +  if (count == gtramp.map_size && tramp_table_alloc ())
> +    return 1;
> +
> +  close (gtramp.fd);
> +  gtramp.fd = -1;
> +  return 0;
> +}
> +
> +#else /* !defined HAVE_MKSTEMP */
> +
> +/*
> + * TODO:
> + * Perhaps, libffi can supply its own version of mkstemp() if it is
> + * not natively available.
> + */
> +static int
> +ffi_tramp_get_temp_file (void)
> +{
> +  gtramp.offset = 0;
> +  gtramp.fd = -1;
> +  return 0;
> +}
> +
> +#endif /* defined HAVE_MKSTEMP */
> +
> +#endif /* __linux__ */

Ok.

> +/* ------------------------ OS-specific Initialization ----------------------*/
> +
> +#if defined __linux__
> +
> +static int
> +ffi_tramp_init_os (void)
> +{
> +  if (ffi_tramp_get_libffi ())
> +    return 1;
> +  return ffi_tramp_get_temp_file ();
> +}
> +
> +#endif /* __linux__ */

Ok.

> +/* --------------------------- OS-specific Locking -------------------------*/
> +
> +#if defined __linux__
> +
> +static pthread_mutex_t gtramp_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> +static void
> +ffi_tramp_lock(void)
> +{
> +  pthread_mutex_lock (&gtramp_mutex);
> +}
> +
> +static void
> +ffi_tramp_unlock()
> +{
> +  pthread_mutex_unlock (&gtramp_mutex);
> +}
> +
> +#endif /* __linux__ */

Ok.

> +/* ------------------------ OS-specific Memory Mapping ----------------------*/
> +
> +/*
> + * Create a trampoline code table mapping and a trampoline parameter table
> + * mapping. The two mappings must be adjacent to each other for PC-relative
> + * access.
> + *
> + * For each trampoline in the code table, there is a corresponding parameter
> + * block in the parameter table. The size of the parameter block is the same
> + * as the size of the trampoline. This means that the parameter block is at
> + * a fixed offset from its trampoline making it easy for a trampoline to find
> + * its parameters using PC-relative access.
> + *
> + * The parameter block will contain a struct tramp_parm. This means that
> + * sizeof (struct tramp_parm) cannot exceed the size of a parameter block.
> + */
> +
> +#if defined __linux__
> +
> +static int
> +tramp_table_map (struct tramp_table *table)
> +{
> +  char *addr;
> +
> +  /*
> +   * Create an anonymous mapping twice the map size. The top half will be used
> +   * for the code table. The bottom half will be used for the parameter table.
> +   */
> +  addr = mmap (NULL, gtramp.map_size * 2, PROT_READ | PROT_WRITE,
> +    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +  if (addr == MAP_FAILED)
> +    return 0;
> +
> +  /*
> +   * Replace the top half of the anonymous mapping with the code table mapping.
> +   */
> +  table->code_table = mmap (addr, gtramp.map_size, PROT_READ | PROT_EXEC,
> +    MAP_PRIVATE | MAP_FIXED, gtramp.fd, gtramp.offset);
> +  if (table->code_table == MAP_FAILED)
> +    {
> +      (void) munmap (addr, gtramp.map_size * 2);
> +      return 0;
> +    }
> +  table->parm_table = table->code_table + gtramp.map_size;
> +  return 1;
> +}
> +
> +static void
> +tramp_table_unmap (struct tramp_table *table)
> +{
> +  (void) munmap (table->code_table, gtramp.map_size);
> +  (void) munmap (table->parm_table, gtramp.map_size);
> +}
> +
> +#endif /* __linux__ */

Ok.

> +/* ------------------------ Trampoline Initialization ----------------------*/
> +
> +/*
> + * Initialize the static trampoline feature.
> + */
> +static int
> +ffi_tramp_init (void)
> +{
> +  if (gtramp.status == GTRAMP_PASSED)
> +    return 1;
> +
> +  if (gtramp.status == GTRAMP_FAILED)
> +    return 0;
> +
> +  if (ffi_tramp_arch == NULL)
> +    {
> +      gtramp.status = GTRAMP_FAILED;
> +      return 0;
> +    }
> +
> +  gtramp.tables = NULL;
> +  gtramp.ntables = 0;
> +
> +  /*
> +   * Get trampoline code table information from the architecture.
> +   */
> +  gtramp.text = ffi_tramp_arch (&gtramp.size, &gtramp.map_size);
> +  gtramp.ntramp = gtramp.map_size / gtramp.size;
> +
> +  if (sysconf (_SC_PAGESIZE) > gtramp.map_size)
> +    return 0;
> +
> +  if (ffi_tramp_init_os ())
> +    {
> +      gtramp.status = GTRAMP_PASSED;
> +      return 1;
> +    }
> +
> +  gtramp.status = GTRAMP_FAILED;
> +  return 0;
> +}

Ok.

> +/* ---------------------- Trampoline Table functions ---------------------- */
> +
> +/* This code assumes that malloc () is available on all OSes. */
> +
> +static void tramp_add (struct tramp *tramp);
> +
> +/*
> + * Allocate and initialize a trampoline table.
> + */
> +static int
> +tramp_table_alloc (void)
> +{
> +  struct tramp_table *table;
> +  struct tramp *tramp_array, *tramp;
> +  size_t size;
> +  char *code, *parm;
> +  int i;
> +
> +  /*
> +   * If we already have tables with free trampolines, there is no need to
> +   * allocate a new table.
> +   */
> +  if (gtramp.ntables > 0)
> +    return 1;

success because there's a free slot somewhere

> +  /*
> +   * Allocate a new trampoline table structure.
> +   */
> +  table = malloc (sizeof (*table));
> +  if (table == NULL)
> +    return 0;

Failure because malloc failure.

> +  /*
> +   * Allocate new trampoline structures.
> +   */
> +  tramp_array = malloc (sizeof (*tramp) * gtramp.ntramp);
> +  if (tramp_array == NULL)
> +    goto free_table;

Failure because malloc failure.


> +  /*
> +   * Map a code table and a parameter table into the caller's address space.
> +   */
> +  if (!tramp_table_map (table))
> +    goto free_tramp_array;

Mapping failure

> +  /*
> +   * Initialize the trampoline table.
> +   */
> +  table->array = tramp_array;
> +  table->free = NULL;
> +  table->nfree = 0;
> +
> +  /*
> +   * Populate the trampoline table free list. This will also add the trampoline
> +   * table to the global list of trampoline tables.
> +   */
> +  size = gtramp.size;
> +  code = table->code_table;
> +  parm = table->parm_table;
> +  for (i = 0; i < gtramp.ntramp; i++)
> +    {
> +      tramp = &tramp_array[i];
> +      tramp->table = table;
> +      tramp->code = code;
> +      tramp->parm = (struct tramp_parm *) parm;
> +      tramp_add (tramp);
> +
> +      code += size;
> +      parm += size;
> +    }
> +  return 1;

Sucess

> +free_tramp_array:
> +  free (tramp_array);
> +free_table:
> +  free (table);
> +  return 0;
> +}

Ok

> +/*
> + * Free a trampoline table.
> + */
> +static void
> +tramp_table_free (struct tramp_table *table)
> +{
> +  tramp_table_unmap (table);
> +  free (table->array);
> +  free (table);
> +}

Ok.

> +/*
> + * Add a new trampoline table to the global table list.
> + */
> +static void
> +tramp_table_add (struct tramp_table *table)
> +{
> +  table->next = gtramp.tables;
> +  table->prev = NULL;
> +  if (gtramp.tables != NULL)
> +    gtramp.tables->prev = table;
> +  gtramp.tables = table;
> +  gtramp.ntables++;

This assumes that the table being added has free slots.  Do we only ever
add empty tables?

> +}



> +/*
> + * Delete a trampoline table from the global table list.
> + */
> +static void
> +tramp_table_del (struct tramp_table *table)
> +{
> +  gtramp.ntables--;

Same here.

> +  if (table->prev != NULL)
> +    table->prev->next = table->next;
> +  if (table->next != NULL)
> +    table->next->prev = table->prev;
> +  if (gtramp.tables == table)
> +    gtramp.tables = table->next;
> +}

Otherwise OK

> +/* ------------------------- Trampoline functions ------------------------- */
> +
> +/*
> + * Add a trampoline to its trampoline table.
> + */
> +static void
> +tramp_add (struct tramp *tramp)
> +{
> +  struct tramp_table *table = tramp->table;
> +
> +  tramp->next = table->free;
> +  tramp->prev = NULL;
> +  if (table->free != NULL)
> +    table->free->prev = tramp;
> +  table->free = tramp;
> +  table->nfree++;
> +
> +  if (table->nfree == 1)
> +    tramp_table_add (table);
> +
> +  /*
> +   * We don't want to keep too many free trampoline tables lying around.
> +   */
> +  if (table->nfree == gtramp.ntramp && gtramp.ntables > 1)

You can't compare these.  The first is a count of *trampolines* and the
second is a count of *tables*.

> +    {
> +      tramp_table_del (table);
> +      tramp_table_free (table);
> +    }
> +}

> +/*
> + * Remove a trampoline from its trampoline table.
> + */
> +static void
> +tramp_del (struct tramp *tramp)
> +{
> +  struct tramp_table *table = tramp->table;
> +
> +  table->nfree--;
> +  if (tramp->prev != NULL)
> +    tramp->prev->next = tramp->next;
> +  if (tramp->next != NULL)
> +    tramp->next->prev = tramp->prev;
> +  if (table->free == tramp)
> +    table->free = tramp->next;
> +
> +  if (table->nfree == 0)
> +    tramp_table_del (table);
> +}

tramp_table_del unlinks the table, but doesn't deallocate its
resources.  Are these resources lost/leaked?

> +/* ------------------------ Trampoline API functions ------------------------ */
> +
> +int
> +ffi_tramp_is_supported(void)
> +{
> +  int ret;
> +
> +  ffi_tramp_lock();
> +  ret = ffi_tramp_init ();
> +  ffi_tramp_unlock();
> +  return ret;
> +}

Ok.

> +/*
> + * Allocate a trampoline and return its opaque address.
> + */
> +void *
> +ffi_tramp_alloc (int flags)
> +{
> +  struct tramp *tramp;
> +
> +  ffi_tramp_lock();
> +
> +  if (!ffi_tramp_init () || flags != 0)
> +    {
> +      ffi_tramp_unlock();
> +      return NULL;
> +    }
> +
> +  if (!tramp_table_alloc ())
> +    {
> +      ffi_tramp_unlock();
> +      return NULL;
> +    }
> +
> +  tramp = gtramp.tables->free;
> +  tramp_del (tramp);
> +
> +  ffi_tramp_unlock();
> +
> +  return tramp;
> +}

Ok.

> +/*
> + * Set the parameters for a trampoline.
> + */
> +void
> +ffi_tramp_set_parms (void *arg, void *target, void *data)
> +{
> +  struct tramp *tramp = arg;
> +
> +  ffi_tramp_lock();
> +  tramp->parm->target = target;
> +  tramp->parm->data = data;
> +  ffi_tramp_unlock();
> +}

Ok.

> +/*
> + * Get the invocation address of a trampoline.
> + */
> +void *
> +ffi_tramp_get_addr (void *arg)
> +{
> +  struct tramp *tramp = arg;
> +  void *addr;
> +
> +  ffi_tramp_lock();
> +  addr = tramp->code;
> +  ffi_tramp_unlock();
> +
> +  return addr;
> +}

Ok.

> +/*
> + * Free a trampoline.
> + */
> +void
> +ffi_tramp_free (void *arg)
> +{
> +  struct tramp *tramp = arg;
> +
> +  ffi_tramp_lock();
> +  tramp_add (tramp);
> +  ffi_tramp_unlock();
> +}

Ok.

> +/* ------------------------------------------------------------------------- */
> +
> +#else /* !FFI_EXEC_STATIC_TRAMP */
> +
> +#include <stddef.h>
> +
> +int
> +ffi_tramp_is_supported(void)
> +{
> +  return 0;
> +}
> +
> +void *
> +ffi_tramp_alloc (int flags)
> +{
> +  return NULL;
> +}
> +
> +void
> +ffi_tramp_set_parms (void *arg, void *target, void *data)
> +{
> +}
> +
> +void *
> +ffi_tramp_get_addr (void *arg)
> +{
> +  return NULL;
> +}
> +
> +void
> +ffi_tramp_free (void *arg)
> +{
> +}
> +
> +#endif /* FFI_EXEC_STATIC_TRAMP */

Ok.

> +#ifndef FFI_EXEC_STATIC_TRAMP
> +  /* With static trampolines, the codeloc does not point to closure */
>    CHECK(memcmp(pcl, codeloc, sizeof(*pcl)) == 0);
> +#endif

Ok.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-15 18:46   ` [RFC PATCH v3 2/5] x86: Support for " madvenka
@ 2021-01-27  3:31     ` DJ Delorie
  2021-01-28 21:59       ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: DJ Delorie @ 2021-01-27  3:31 UTC (permalink / raw)
  To: madvenka; +Cc: libffi-discuss


madvenka@linux.microsoft.com writes:

> diff --git a/src/x86/ffi64.c b/src/x86/ffi64.c
> index 39f9598..2a5cf5a 100644
> --- a/src/x86/ffi64.c
> +++ b/src/x86/ffi64.c
> @@ -713,7 +713,9 @@ ffi_call_go (ffi_cif *cif, void (*fn)(void), void *rvalue,
>  #endif /* FFI_GO_CLOSURES */
>  
>  extern void ffi_closure_unix64(void) FFI_HIDDEN;
> +extern void ffi_closure_unix64_alt(void) FFI_HIDDEN;
>  extern void ffi_closure_unix64_sse(void) FFI_HIDDEN;
> +extern void ffi_closure_unix64_sse_alt(void) FFI_HIDDEN;

Extern, but local to this port, yes?

> @@ -742,6 +744,7 @@ ffi_prep_closure_loc (ffi_closure* closure,
>      0x0f, 0x1f, 0x80, 0x00, 0x00, 0x00, 0x00
>    };
>    void (*dest)(void);
> +  void (*dest_alt)(void);
>    char *tramp = closure->tramp;

Ok

> @@ -752,13 +755,28 @@ ffi_prep_closure_loc (ffi_closure* closure,
>      return FFI_BAD_ABI;
>  
>    if (cif->flags & UNIX64_FLAG_XMM_ARGS)
> -    dest = ffi_closure_unix64_sse;
> +    {
> +      dest = ffi_closure_unix64_sse;
> +      dest_alt = ffi_closure_unix64_sse_alt;
> +    }
>    else
> -    dest = ffi_closure_unix64;
> +    {
> +      dest = ffi_closure_unix64;
> +      dest_alt = ffi_closure_unix64_alt;
> +    }
>  
> +  if (ffi_tramp_is_present(closure))
> +    {
> +      /* Initialize the static trampoline's parameters. */
> +      ffi_tramp_set_parms (closure->ftramp, dest_alt, closure);
> +      goto out;
> +    }
> +
> +  /* Initialize the dynamic trampoline. */

Should these new APIs be inside #if FFI_EXEC_STATIC_TRAMP ?

> +#if defined(FFI_EXEC_STATIC_TRAMP)
> +void *
> +ffi_tramp_arch (size_t *tramp_size, size_t *map_size)
> +{
> +  extern void *trampoline_code_table_cet;
> +  extern void *trampoline_code_table;
> +  extern int ffi_cet_present;
> +
> +  *map_size = UNIX64_TRAMP_MAP_SIZE;
> +  if (ffi_cet_present) {
> +    *tramp_size = UNIX64_TRAMP_SIZE_CET;
> +    return &trampoline_code_table_cet;
> +  }
> +  *tramp_size = UNIX64_TRAMP_SIZE;
> +  return &trampoline_code_table;
> +}
> +#endif

Ok.

> diff --git a/src/x86/ffiw64.c b/src/x86/ffiw64.c
> index a43a9eb..df81d66 100644
> --- a/src/x86/ffiw64.c
> +++ b/src/x86/ffiw64.c
> @@ -187,6 +187,7 @@ EFI64(ffi_call_go)(ffi_cif *cif, void (*fn)(void), void *rvalue,
>  
>  
>  extern void ffi_closure_win64(void) FFI_HIDDEN;
> +extern void ffi_closure_win64_alt(void) FFI_HIDDEN;
>  
>  #ifdef FFI_GO_CLOSURES
>  extern void ffi_go_closure_win64(void) FFI_HIDDEN;
> @@ -220,9 +221,18 @@ EFI64(ffi_prep_closure_loc)(ffi_closure* closure,
>        return FFI_BAD_ABI;
>      }
>  
> +  if (ffi_tramp_is_present(closure))
> +    {
> +      /* Initialize the static trampoline's parameters. */
> +      ffi_tramp_set_parms (closure->ftramp, ffi_closure_win64_alt, closure);
> +      goto out;
> +    }
> +
> +  /* Initialize the dynamic trampoline. */
>    memcpy (tramp, trampoline, sizeof(trampoline));
>    *(UINT64 *)(tramp + sizeof (trampoline)) = (uintptr_t)ffi_closure_win64;
>  
> +out:
>    closure->cif = cif;
>    closure->fun = fun;
>    closure->user_data = user_data;

Ok.

> diff --git a/src/x86/internal64.h b/src/x86/internal64.h
> index 512e955..410bdf2 100644
> --- a/src/x86/internal64.h
> +++ b/src/x86/internal64.h
> @@ -20,3 +20,14 @@
>  #define UNIX64_FLAG_RET_IN_MEM	(1 << 10)
>  #define UNIX64_FLAG_XMM_ARGS	(1 << 11)
>  #define UNIX64_SIZE_SHIFT	12
> +
> +#if defined(FFI_EXEC_STATIC_TRAMP)
> +/*
> + * For the trampoline code table mapping, a mapping size of 4K (base page size)
> + * is chosen.
> + */
> +#define UNIX64_TRAMP_MAP_SHIFT	12
> +#define UNIX64_TRAMP_MAP_SIZE	(1 << UNIX64_TRAMP_MAP_SHIFT)
> +#define UNIX64_TRAMP_SIZE_CET	40
> +#define UNIX64_TRAMP_SIZE	32
> +#endif

Ok.

> diff --git a/src/x86/unix64.S b/src/x86/unix64.S
> index 89d7db1..e26ea2c 100644
> --- a/src/x86/unix64.S
> +++ b/src/x86/unix64.S
> @@ -63,6 +63,7 @@
>  C(ffi_call_unix64):
>  L(UW0):
>  	_CET_ENDBR
> +L(endbr):

This hack to detect CET should be replaced by the logic in ffitarget.h,
or add a #define CET_ENABLED to ffitarget.h

> @@ -270,6 +271,17 @@ L(UW6):
>  L(UW7):
>  ENDF(C(ffi_closure_unix64_sse))
>  
> +	.balign	2
> +	.globl	C(ffi_closure_unix64_sse_alt)
> +	FFI_HIDDEN(C(ffi_closure_unix64_sse_alt))
> +
> +C(ffi_closure_unix64_sse_alt):
> +	_CET_ENDBR
> +	movq	8(%rsp), %r10
> +	addq	$16, %rsp

Copies first argument to %r10, discards return address and arg - closure
will return to whoever called it's caller.  I'm not sure how this works,
which means *at least* a comment needs to be here ;-)

> +	jmp	C(ffi_closure_unix64_sse)
> +ENDF(C(ffi_closure_unix64_sse_alt))
> +
>  	.balign	2
>  	.globl	C(ffi_closure_unix64)
>  	FFI_HIDDEN(C(ffi_closure_unix64))
> @@ -400,6 +412,17 @@ L(la):	call	PLT(C(abort))
>  L(UW11):
>  ENDF(C(ffi_closure_unix64))
>  
> +	.balign	8
> +	.globl	C(ffi_closure_unix64_alt)
> +	FFI_HIDDEN(C(ffi_closure_unix64_alt))
> +
> +C(ffi_closure_unix64_alt):
> +	_CET_ENDBR
> +	movq	8(%rsp), %r10
> +	addq	$16, %rsp
> +	jmp	C(ffi_closure_unix64)
> +	ENDF(C(ffi_closure_unix64_alt))
> +
>  	.balign	2
>  	.globl	C(ffi_go_closure_unix64_sse)
>  	FFI_HIDDEN(C(ffi_go_closure_unix64_sse))

Likewise.

> +/*
> + * The trampoline uses register r10. It saves the original value of r10 on
> + * the stack.
> + *
> + * The trampoline has two parameters - target code to jump to and data for
> + * the target code. The trampoline extracts the parameters from its parameter
> + * block (see tramp_table_map()). The trampoline saves the data address on
> + * the stack. Finally, it jumps to the target code.
> + *
> + * The target code can choose to:
> + *
> + * - restore the value of r10
> + * - load the data address in a register
> + * - restore the stack pointer to what it was when the trampoline was invoked.
> + */
> +
> +	.align	UNIX64_TRAMP_MAP_SIZE
> +	.globl	trampoline_code_table_cet
> +	FFI_HIDDEN(C(trampoline_code_table_cet))
> +
> +C(trampoline_code_table_cet):
> +	.rept	UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE_CET
> +	_CET_ENDBR
> +	subq	$16, %rsp		/* Make space on the stack */
> +	movq	%r10, (%rsp)		/* Save %r10 on stack */
> +	movq	4077(%rip), %r10	/* Copy data into %r10 */
> +	movq	%r10, 8(%rsp)		/* Save data on stack */
> +	movq	4073(%rip), %r10	/* Copy code into %r10 */
> +	jmp	*%r10			/* Jump to code */
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	.endr
> +ENDF(C(trampoline_code_table_cet))
> +
> +	.align	UNIX64_TRAMP_MAP_SIZE
> +	.globl	trampoline_code_table
> +	FFI_HIDDEN(C(trampoline_code_table))
> +
> +C(trampoline_code_table):
> +	.rept	UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE
> +	subq	$16, %rsp		/* Make space on the stack */
> +	movq	%r10, (%rsp)		/* Save %r10 on stack */
> +	movq	4081(%rip), %r10	/* Copy data into %r10 */
> +	movq	%r10, 8(%rsp)		/* Save data on stack */
> +	movq	4077(%rip), %r10	/* Copy code into %r10 */
> +	jmp	*%r10			/* Jump to code */
> +	nop
> +	nop
> +	.endr
> +ENDF(C(trampoline_code_table))
> +	.align	UNIX64_TRAMP_MAP_SIZE
> +#endif /* FFI_EXEC_STATIC_TRAMP */

Why does the longer trampoline (with endbr) have *more* nops?  Is it for
8-byte alignment?  If so, comment ;-)

> @@ -615,6 +712,13 @@ L(EFDE5):
>  	.quad    0
>  #endif
>  
> +	.section .rodata
> +	.align 8
> +	.globl ffi_cet_present
> +ffi_cet_present:
> +	.set	L6,L(endbr)-L(UW0)
> +	.int	L6
> +

Again, there are preprocessor directives that do this better.

> diff --git a/src/x86/win64.S b/src/x86/win64.S
> index 8315e8b..6ca3068 100644
> --- a/src/x86/win64.S
> +++ b/src/x86/win64.S
> @@ -234,6 +234,18 @@ C(ffi_closure_win64):
>  
>  	cfi_endproc
>  	SEH(.seh_endproc)
> +
> +	.align	8
> +	.globl	C(ffi_closure_win64_alt)
> +	FFI_HIDDEN(C(ffi_closure_win64_alt))
> +
> +	SEH(.seh_proc ffi_closure_win64_alt)
> +C(ffi_closure_win64_alt):
> +	_CET_ENDBR
> +	movq	8(%rsp), %r10
> +	addq	$16, %rsp
> +	jmp	C(ffi_closure_win64)
> +	SEH(.seh_endproc)
>  #endif /* __x86_64__ */

Ok.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-26 23:41   ` [RFC PATCH v3 0/5] Libffi " Anthony Green
@ 2021-01-27 17:20     ` Madhavan T. Venkataraman
  2021-01-27 18:00       ` Anthony Green
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-27 17:20 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, fweimer, DJ Delorie



On 1/26/21 5:41 PM, Anthony Green wrote:
> Madhaven,
> 
>   Thank you for your continued efforts here.
> 
> On Fri, Jan 15, 2021 at 1:47 PM <madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com>> wrote:
> 
>     Trampoline API
>     ==============
> 
>     There is a lot of dynamic code out there. They all have the same security
>     issue. Dynamic code can be re-written into static code provided the data
>     required by the static code can be passed to it just like we pass the closure
>     pointer to an ABI handler.
> 
>     So, the same trampoline functions used by libffi internally need to be
>     made available to the rest of the world in the form of an API. The
>     following API has been defined in this solution:
> 
> 
> 
> I need a better explanation around why this new API is required, and this new capability isn't just an implementation detail supporting the old API.
> 

Example 1
=========

Let us say that an application or a library has a simple trampoline - load a value or
an address in a register or the stack and transfer control to some target function.
It would have dynamic code to do that just like libffi currently does.

Dynamic code has a security issue as I have explained in the cover letter. It needs
to be loaded in an executable page. A clever hacker may be able to inject his code
in that page. To prevent that, dynamic code has to be converted into static code
so that it is in a read-only text segment.

I have solved this problem for libffi and I have provided statically defined trampolines.

The application or library that is currently using dynamic code for its trampoline could
instead use the libffi API and replace its trampoline. This would eliminate dynamic code
from that application or library and make it more secure.

The only thing is that the target function should now expect its data in the libffi
designated scratch register instead of whatever it was using. This is trivial to do.

Example 2
==========

Let us say that an application or library uses something more complicated than the
libffi trampoline. For example, a piece of dynamic code that unmarshalls parameters
from a structure and calls a target function.

The application developer is interested in eliminating this piece of dynamic code
for security purposes. The developer could rewrite the dynamic code into static
code. The only thing is that the structure pointer needs to be passed to the static
code. Either the application developer can write his own code (using PC-relative
accesses, etc) to pass data. Or, he can use the libffi API to do it.

So, the only effort for the developer/maintainer of the application/library is to
rewrite the unmarshalling dynamic code into static code. That should be straight forward.

Example 3
=========

Let us say that an application or library has a piece of dynamic code to perform
some computation and produce results. It is written as dynamic code because
the exact set of arguments and the exact computation is known only at runtime.
So, the dynamic code is generated at runtime accordingly. When the code is
generated, the data needed by the code is embedded in the generated code.

In this case, the developer needs to do a little extra work. E.g.,

- Define a union of structures with each structure defining a particular
  computation and set of arguments.

- Write static code to accept a pointer to the union and perform
  different computations and return results in that union.

- Then, the static code can be invoked using the libffi API and the union
  pointer can be passed to it.

Summary
=======

There are a lot of applications that use dynamic code out there. Dynamic code
should be eliminated as far as possible for security purposes.

I believe most of them can be converted to static code by the maintainers. All
they need is a method to pass data to their static code without having to implement
the method themselves. The libffi API provides that.

The libffi API can also be supported easily for multiple architectures so that
applications can use the API in an architecture-independent way.

Is this sufficient justification for the API?

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-27 17:20     ` Madhavan T. Venkataraman
@ 2021-01-27 18:00       ` Anthony Green
  2021-01-27 19:45         ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: Anthony Green @ 2021-01-27 18:00 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss, fweimer, DJ Delorie

Thanks, Madhaven.   I think I understand now.   Are these statements true?:

(a) These patches implement trampoline tables, similar to what is
implemented in the iOS port.  This hardens the library by eliminating the
requirement for writable executable memory.
(b) These patches expose a new public API for hardened trampolines.  (a)
uses (b), but doesn't require that (b) be public.
(c) We can release libffi with (a) and not (b).

Is this correct?

AG

On Wed, Jan 27, 2021 at 12:20 PM Madhavan T. Venkataraman <
madvenka@linux.microsoft.com> wrote:

>
>
> On 1/26/21 5:41 PM, Anthony Green wrote:
> > Madhaven,
> >
> >   Thank you for your continued efforts here.
> >
> > On Fri, Jan 15, 2021 at 1:47 PM <madvenka@linux.microsoft.com <mailto:
> madvenka@linux.microsoft.com>> wrote:
> >
> >     Trampoline API
> >     ==============
> >
> >     There is a lot of dynamic code out there. They all have the same
> security
> >     issue. Dynamic code can be re-written into static code provided the
> data
> >     required by the static code can be passed to it just like we pass
> the closure
> >     pointer to an ABI handler.
> >
> >     So, the same trampoline functions used by libffi internally need to
> be
> >     made available to the rest of the world in the form of an API. The
> >     following API has been defined in this solution:
> >
> >
> >
> > I need a better explanation around why this new API is required, and
> this new capability isn't just an implementation detail supporting the old
> API.
> >
>
> Example 1
> =========
>
> Let us say that an application or a library has a simple trampoline - load
> a value or
> an address in a register or the stack and transfer control to some target
> function.
> It would have dynamic code to do that just like libffi currently does.
>
> Dynamic code has a security issue as I have explained in the cover letter.
> It needs
> to be loaded in an executable page. A clever hacker may be able to inject
> his code
> in that page. To prevent that, dynamic code has to be converted into
> static code
> so that it is in a read-only text segment.
>
> I have solved this problem for libffi and I have provided statically
> defined trampolines.
>
> The application or library that is currently using dynamic code for its
> trampoline could
> instead use the libffi API and replace its trampoline. This would
> eliminate dynamic code
> from that application or library and make it more secure.
>
> The only thing is that the target function should now expect its data in
> the libffi
> designated scratch register instead of whatever it was using. This is
> trivial to do.
>
> Example 2
> ==========
>
> Let us say that an application or library uses something more complicated
> than the
> libffi trampoline. For example, a piece of dynamic code that unmarshalls
> parameters
> from a structure and calls a target function.
>
> The application developer is interested in eliminating this piece of
> dynamic code
> for security purposes. The developer could rewrite the dynamic code into
> static
> code. The only thing is that the structure pointer needs to be passed to
> the static
> code. Either the application developer can write his own code (using
> PC-relative
> accesses, etc) to pass data. Or, he can use the libffi API to do it.
>
> So, the only effort for the developer/maintainer of the
> application/library is to
> rewrite the unmarshalling dynamic code into static code. That should be
> straight forward.
>
> Example 3
> =========
>
> Let us say that an application or library has a piece of dynamic code to
> perform
> some computation and produce results. It is written as dynamic code because
> the exact set of arguments and the exact computation is known only at
> runtime.
> So, the dynamic code is generated at runtime accordingly. When the code is
> generated, the data needed by the code is embedded in the generated code.
>
> In this case, the developer needs to do a little extra work. E.g.,
>
> - Define a union of structures with each structure defining a particular
>   computation and set of arguments.
>
> - Write static code to accept a pointer to the union and perform
>   different computations and return results in that union.
>
> - Then, the static code can be invoked using the libffi API and the union
>   pointer can be passed to it.
>
> Summary
> =======
>
> There are a lot of applications that use dynamic code out there. Dynamic
> code
> should be eliminated as far as possible for security purposes.
>
> I believe most of them can be converted to static code by the maintainers.
> All
> they need is a method to pass data to their static code without having to
> implement
> the method themselves. The libffi API provides that.
>
> The libffi API can also be supported easily for multiple architectures so
> that
> applications can use the API in an architecture-independent way.
>
> Is this sufficient justification for the API?
>
> Madhavan
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-27 18:00       ` Anthony Green
@ 2021-01-27 19:45         ` Madhavan T. Venkataraman
  2021-01-28 14:21           ` Anthony Green
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-27 19:45 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, fweimer, DJ Delorie



On 1/27/21 12:00 PM, Anthony Green wrote:
> Thanks, Madhaven.   I think I understand now.   Are these statements true?:
> 
> (a) These patches implement trampoline tables, similar to what is implemented in the iOS port.  This hardens the library by eliminating the requirement for writable executable memory.
> (b) These patches expose a new public API for hardened trampolines.  (a) uses (b), but doesn't require that (b) be public.
> (c) We can release libffi with (a) and not (b).
> 
> Is this correct?  
> 

Yes. This is correct. The public API part is not required.

It is just to provide the open source community with a way to help convert their dynamic code.
We could also consider exposing the public API at a later time when the changes are considered
well exercised.

Madhavan

> AG
> 
> On Wed, Jan 27, 2021 at 12:20 PM Madhavan T. Venkataraman <madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com>> wrote:
> 
> 
> 
>     On 1/26/21 5:41 PM, Anthony Green wrote:
>     > Madhaven,
>     >
>     >   Thank you for your continued efforts here.
>     >
>     > On Fri, Jan 15, 2021 at 1:47 PM <madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com> <mailto:madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com>>> wrote:
>     >
>     >     Trampoline API
>     >     ==============
>     >
>     >     There is a lot of dynamic code out there. They all have the same security
>     >     issue. Dynamic code can be re-written into static code provided the data
>     >     required by the static code can be passed to it just like we pass the closure
>     >     pointer to an ABI handler.
>     >
>     >     So, the same trampoline functions used by libffi internally need to be
>     >     made available to the rest of the world in the form of an API. The
>     >     following API has been defined in this solution:
>     >
>     >
>     >
>     > I need a better explanation around why this new API is required, and this new capability isn't just an implementation detail supporting the old API.
>     >
> 
>     Example 1
>     =========
> 
>     Let us say that an application or a library has a simple trampoline - load a value or
>     an address in a register or the stack and transfer control to some target function.
>     It would have dynamic code to do that just like libffi currently does.
> 
>     Dynamic code has a security issue as I have explained in the cover letter. It needs
>     to be loaded in an executable page. A clever hacker may be able to inject his code
>     in that page. To prevent that, dynamic code has to be converted into static code
>     so that it is in a read-only text segment.
> 
>     I have solved this problem for libffi and I have provided statically defined trampolines.
> 
>     The application or library that is currently using dynamic code for its trampoline could
>     instead use the libffi API and replace its trampoline. This would eliminate dynamic code
>     from that application or library and make it more secure.
> 
>     The only thing is that the target function should now expect its data in the libffi
>     designated scratch register instead of whatever it was using. This is trivial to do.
> 
>     Example 2
>     ==========
> 
>     Let us say that an application or library uses something more complicated than the
>     libffi trampoline. For example, a piece of dynamic code that unmarshalls parameters
>     from a structure and calls a target function.
> 
>     The application developer is interested in eliminating this piece of dynamic code
>     for security purposes. The developer could rewrite the dynamic code into static
>     code. The only thing is that the structure pointer needs to be passed to the static
>     code. Either the application developer can write his own code (using PC-relative
>     accesses, etc) to pass data. Or, he can use the libffi API to do it.
> 
>     So, the only effort for the developer/maintainer of the application/library is to
>     rewrite the unmarshalling dynamic code into static code. That should be straight forward.
> 
>     Example 3
>     =========
> 
>     Let us say that an application or library has a piece of dynamic code to perform
>     some computation and produce results. It is written as dynamic code because
>     the exact set of arguments and the exact computation is known only at runtime.
>     So, the dynamic code is generated at runtime accordingly. When the code is
>     generated, the data needed by the code is embedded in the generated code.
> 
>     In this case, the developer needs to do a little extra work. E.g.,
> 
>     - Define a union of structures with each structure defining a particular
>       computation and set of arguments.
> 
>     - Write static code to accept a pointer to the union and perform
>       different computations and return results in that union.
> 
>     - Then, the static code can be invoked using the libffi API and the union
>       pointer can be passed to it.
> 
>     Summary
>     =======
> 
>     There are a lot of applications that use dynamic code out there. Dynamic code
>     should be eliminated as far as possible for security purposes.
> 
>     I believe most of them can be converted to static code by the maintainers. All
>     they need is a method to pass data to their static code without having to implement
>     the method themselves. The libffi API provides that.
> 
>     The libffi API can also be supported easily for multiple architectures so that
>     applications can use the API in an architecture-independent way.
> 
>     Is this sufficient justification for the API?
> 
>     Madhavan
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 1/5] Libffi Static Trampolines
  2021-01-27  3:31     ` DJ Delorie
@ 2021-01-27 21:51       ` Madhavan T. Venkataraman
  2021-01-27 22:15         ` DJ Delorie
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-27 21:51 UTC (permalink / raw)
  To: DJ Delorie; +Cc: libffi-discuss

Thanks for reviewing this. See inline..

On 1/26/21 9:31 PM, DJ Delorie wrote:
> 
> I've reviewed these patches from the point of view of the details -
> comments, constructs, etc.  Whether the overall design is good I leave
> to Anthony ;-)
> 
> I only reviewed the x86-64 port, the others looked similar enough that
> my rough comments may apply.
> 

ok.

> madvenka@linux.microsoft.com writes:
> 
>> -		src/raw_api.c src/java_raw_api.c src/closures.c
>> +		src/raw_api.c src/java_raw_api.c src/closures.c \
>> +		src/tramp.c
> 
> Ok
> 
>> -AC_CHECK_FUNCS([mmap mkostemp])
>> +AC_CHECK_FUNCS([mmap mkostemp mkstemp])
> 
> Note that src/closure.c has code for finding a temp file that has EXEC
> permission.  A bit of code sharing would be a good future update ;-)
> 

Agreed.

>> +case "$target" in
>> +     *-linux*)
>> +       AC_DEFINE(FFI_EXEC_STATIC_TRAMP, 1,
>> +                 [Define this if you want statically defined trampolines])
>> +     ;;
>> +esac
> 
> Ok.  Might want to conditional for the arches that are supported too.
> 

ok. I will add the arches although the code currently handles that by testing
ffi_tramp_arch == NULL.


>> -  char tramp[FFI_TRAMPOLINE_SIZE];
>> +  union {
>> +    char tramp[FFI_TRAMPOLINE_SIZE];
>> +    void *ftramp;
>> +  };
> 
> Ok.
> 
>> +/* ---- Static Trampoline Definitions -------------------------------------- */
>> +
>> +FFI_API int ffi_tramp_is_supported(void);
>> +FFI_API void *ffi_tramp_alloc (int flags);
>> +FFI_API int ffi_tramp_set_parms (void *tramp, void *data, void *code);
>> +FFI_API void *ffi_tramp_get_addr (void *tramp);
>> +FFI_API void ffi_tramp_free (void *tramp);
> 
> Are we assuming that applications will have to be modified to use this
> new functionality?  It looks like it's used internally for the existing
> API but is it intended to be used directly also?
> 

My intention is to allow applications and libraries that currently use dynamic code
to use the libffi API to eliminate their dynamic code. The maintainers of these
apps/libs would rewrite their dynamic code into static code. All they need is a
way to pass data to their static code (that uses PC-relative access). The libffi
API will let them do that.

I have supplied 3 examples in my reply to Anthony's email. Please take a look
there for more details.

>> +/* The arch code calls this to determine if a given closure has a
>> +   static trampoline. */
>> +int ffi_tramp_is_present (void *closure);
> 
> Ok
> 
>> +
>> +LIBFFI_STATIC_TRAMP_8.0 {
>> +  global:
>> +	ffi_tramp_is_supported;
>> +	ffi_tramp_alloc;
>> +	ffi_tramp_set_parms;
>> +	ffi_tramp_get_addr;
>> +	ffi_tramp_free;
>> +} LIBFFI_BASE_8.0;
> 
> This is missing a suitable #if/#endif around it
> 

My intention is to always have the API functions defined in the library. If
FFI_EXEC_STATIC_TRAMP is defined, then, the API will supply static trampolines.
Else, the stub API functions will return an error and the caller can fall back
on his existing method if any.

This is just so applications don't have to worry about symbol resolution while
linking with libffi.

>> +
>> +int
>> +ffi_tramp_is_present (__attribute__((unused)) void *ptr)
>> +{
>> +  return 0;
>> +}
>>  #else /* !NetBSD with PROT_MPROTECT */
> 
> Ok.
> 
>>  #if !FFI_MMAP_EXEC_WRIT && !FFI_EXEC_TRAMPOLINE_TABLE
>> @@ -843,6 +849,12 @@ dlmmap (void *start, size_t length, int prot,
>>  	  && flags == (MAP_PRIVATE | MAP_ANONYMOUS)
>>  	  && fd == -1 && offset == 0);
>>  
>> +  if (execfd == -1 && ffi_tramp_is_supported ())
>> +    {
>> +      ptr = mmap (start, length, prot & ~PROT_EXEC, flags, fd, offset);
>> +      return ptr;
>> +    }
>> +
> 
> Should check for error, but not all existing cases do.  Do we expect to
> allow a fallback when static trampolines are enabled?
> 

In this section of the code (under FFI_MMAP_EXEC_WRIT), all attempts to get
memory use mmap(). Static trampolines and emutramp use RW permissions. All
other attempts use RWX. If the mmap() above fails, all other attempts to
get memory will fail as well.

So, if ffi_tramp_is_supported(), there is no fallback if mmap() fails.

The only fallback associated with static trampolines is that if static
trampolines are not supported, all API functions including ffi_tramp_is_supported()
will return 0. Then, the code falls back to using the current method for
getting dlmmap() memory, allocating the closure from there and using the
legacy trampolines.

>> @@ -922,7 +934,7 @@ segment_holding_code (mstate m, char* addr)
>>  void *
>>  ffi_closure_alloc (size_t size, void **code)
>>  {
>> -  void *ptr;
>> +  void *ptr, *ftramp;
> 
> Ok.
> 
>> @@ -934,6 +946,17 @@ ffi_closure_alloc (size_t size, void **code)
>>        msegmentptr seg = segment_holding (gm, ptr);
>>  
>>        *code = add_segment_exec_offset (ptr, seg);
>> +      if (!ffi_tramp_is_supported ())
>> +        return ptr;
>> +
>> +      ftramp = ffi_tramp_alloc (0);
>> +      if (ftramp == NULL)
>> +      {
>> +        dlfree (FFI_RESTORE_PTR (ptr));
>> +        return NULL;
>> +      }
>> +      *code = ffi_tramp_get_addr (ftramp);
>> +      ((ffi_closure *) ptr)->ftramp = ftramp;
>>      }
> 
> Ok.
> 
>> @@ -943,12 +966,17 @@ void *
>>  ffi_data_to_code_pointer (void *data)
>>  {
>>    msegmentptr seg = segment_holding (gm, data);
>> +
> 
> Extraneous but ok.

This is needed. When ffi_prep_closure_loc() calls this function, we do not
know if the closure was allocated by calling ffi_closure_alloc() or the
libffi user allocated it outside of libffi.

Since the static trampoline for a closure is created in ffi_closure_alloc(),
we have to check this first.

If seg is NULL, then it means that it was allocated outside of libffi.
At least, that is my understanding. If seg is non-NULL, then the closure
was allocated by calling ffi_closure_alloc(). Then, the code can proceeds
to check if static trampolines need to be used.

> 
>>    /* We expect closures to be allocated with ffi_closure_alloc(), in
>>       which case seg will be non-NULL.  However, some users take on the
>>       burden of managing this memory themselves, in which case this
>>       we'll just return data. */
>>    if (seg)
>> -    return add_segment_exec_offset (data, seg);
>> +    {
>> +      if (!ffi_tramp_is_supported ())
>> +        return add_segment_exec_offset (data, seg);
>> +      return ffi_tramp_get_addr (((ffi_closure *) data)->ftramp);
>> +    }
>>    else
>>      return data;
> 
> Ok
> 
>> +  if (ffi_tramp_is_supported ())
>> +    ffi_tramp_free (((ffi_closure *) ptr)->ftramp);
> 
> Ok.
> 
>> +int
>> +ffi_tramp_is_present (void *ptr)
>> +{
>> +  msegmentptr seg = segment_holding (gm, ptr);
>> +  return seg != NULL && ffi_tramp_is_supported();
>> +}
>> +
> 
> Ok.
> 
>> +int
>> +ffi_tramp_is_present (__attribute__((unused)) void *ptr)
>> +{
>> +  return 0;
>> +}
>> +
>>  # endif /* ! FFI_MMAP_EXEC_WRIT */
>>  #endif /* FFI_CLOSURES */
> 
> Ok.
> 
>> diff --git a/src/tramp.c b/src/tramp.c
>> new file mode 100644
>> index 0000000..4151cc9
>> --- /dev/null
>> +++ b/src/tramp.c
>> @@ -0,0 +1,718 @@
>> +/* -----------------------------------------------------------------------
>> +   tramp.c - Copyright (c) 2020 Madhavan T. Venkataraman
> 
> I'll leave it up to Anthony to decide on copyright issues

ok.

> 
>> +   API and support functions for managing statically defined closure
>> +   trampolines.
>> +
>> +   Permission is hereby granted, free of charge, to any person obtaining
>> +   a copy of this software and associated documentation files (the
>> +   ``Software''), to deal in the Software without restriction, including
>> +   without limitation the rights to use, copy, modify, merge, publish,
>> +   distribute, sublicense, and/or sell copies of the Software, and to
>> +   permit persons to whom the Software is furnished to do so, subject to
>> +   the following conditions:
>> +
>> +   The above copyright notice and this permission notice shall be included
>> +   in all copies or substantial portions of the Software.
>> +
>> +   THE SOFTWARE IS PROVIDED ``AS IS'', WITHOUT WARRANTY OF ANY KIND,
>> +   EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +   MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> +   NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>> +   HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
>> +   WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> +   OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> +   DEALINGS IN THE SOFTWARE.
>> +   ----------------------------------------------------------------------- */
>> +
>> +#include <fficonfig.h>
>> +
>> +#ifdef FFI_EXEC_STATIC_TRAMP
>> +
>> +/* -------------------------- Headers and Definitions ---------------------*/
>> +/*
>> + * Add support for other OSes later. For now, it is just Linux.
>> + */
> 
> This should be handled in configure, and this whole file #if'd based on
> those results.
> 

It is, actually. configure defines FFI_EXEC_STATIC_TRAMP for linux.

fficonfig.h is the generated file that contains the define, if it is defined.

And, all of the code in this file is included like this:

#ifdef FFI_EXEC_STATIC_TRAMP

Static trampoline API and support functions

#else

Stub API for static trampolines.

#endif

>> +#if defined __linux__
>> +#ifdef __linux__
>> +#define _GNU_SOURCE 1
>> +#endif
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +#include <fcntl.h>
>> +#include <pthread.h>
>> +#include <sys/mman.h>
>> +#ifdef __linux__
>> +#include <linux/limits.h>
>> +#include <linux/types.h>
>> +#endif
>> +#endif /* __linux__ */
> 
> Ok
> 
>> +/*
>> + * Each architecture defines static code for a trampoline code table. The
>> + * trampoline code table is mapped into the address space of a process.
>> + *
>> + * The following architecture specific function returns:
>> + *
>> + *	- the address of the trampoline code table in the text segment
>> + *	- the size of each trampoline in the trampoline code table
>> + *	- the size of the mapping for the whole trampoline code table
>> + */
>> +void __attribute__((weak)) *ffi_tramp_arch (size_t *tramp_size,
>> +  size_t *map_size);
> 
> Should this be extern?

I don't know the libffi requirement for this. For linux kernel code, they
recommend omitting the extern. I am influenced by that, I could add the extern
if that is what is desired.

> 
>> +/* ------------------------- Trampoline Data Structures --------------------*/
>> +
>> +struct tramp;
>> +
>> +/*
>> + * Trampoline table. Manages one trampoline code table and one trampoline
>> + * parameter table.
>> + *
>> + * prev, next	Links in the global trampoline table list.
>> + * code_table	Trampoline code table mapping.
>> + * parm_table	Trampoline parameter table mapping.
>> + * array	Array of trampolines malloced.
>> + * free		List of free trampolines.
>> + * nfree	Number of free trampolines.
>> + */
>> +struct tramp_table
>> +{
>> +  struct tramp_table *prev;
>> +  struct tramp_table *next;
>> +  void *code_table;
>> +  void *parm_table;
>> +  struct tramp *array;
>> +  struct tramp *free;
>> +  int nfree;
>> +};
>> +
>> +/*
>> + * Parameters for each trampoline.
>> + *
>> + * data
>> + *	Data for the target code that the trampoline jumps to.
>> + * target
>> + *	Target code that the trampoline jumps to.
>> + */
>> +struct tramp_parm
>> +{
>> +  void *data;
>> +  void *target;
>> +};
>> +
>> +/*
>> + * Trampoline structure for each trampoline.
>> + *
>> + * prev, next	Links in the trampoline free list of a trampoline table.
>> + * table	Trampoline table to which this trampoline belongs.
>> + * code		Address of this trampoline in the code table mapping.
>> + * parm		Address of this trampoline's parameters in the parameter
>> + *		table mapping.
>> + */
>> +struct tramp
>> +{
>> +  struct tramp *prev;
>> +  struct tramp *next;
>> +  struct tramp_table *table;
>> +  void *code;
>> +  struct tramp_parm *parm;
>> +};
>> +
>> +enum gtramp_status {
>> +	GTRAMP_UNINITIALIZED = 0,
>> +	GTRAMP_PASSED,
>> +	GTRAMP_FAILED,
>> +};
> 
> Why is this one suddently called "gtramp" instead of "tramp" ?
> Needs a comment.  Possibly rename to "global_tramp_status"

The "g" stands for global. I used "g" to keep the names short.
I will change g to global and G to GLOBAL.

> 
>> +/*
>> + * Trampoline globals.
>> + *
>> + * fd
>> + *	File descriptor of binary file that contains the trampoline code table.
>> + * offset
>> + *	Offset of the trampoline code table in that file.
>> + * text
>> + *	Address of the trampoline code table in the text segment.
>> + * map_size
>> + *	Size of the trampoline code table mapping.
>> + * size
>> + *	Size of one trampoline in the trampoline code table.
>> + * ntramp
>> + *	Total number of trampolines in the trampoline code table.
>> + * tables
>> + *	List of trampoline tables that contain free trampolines.
>> + * ntables
>> + *	Number of trampoline tables that contain free trampolines.
> 
> Perhaps name this "n_nonfree_tables" ?  Based on the name, I would
> assume it was "number of tables".
> 

Yes. It is confusing. I will change the name to something appropriate.

> 
>> + * status
>> + *	Initialization status.
>> + */
>> +struct tramp_global
>> +{
>> +  int fd;
>> +  off_t offset;
>> +  void *text;
>> +  size_t map_size;
>> +  size_t size;
>> +  int ntramp;
>> +  struct tramp_table *tables;
>> +  int ntables;
>> +  enum gtramp_status status;
>> +};
>> +
>> +static struct tramp_global gtramp;
> 
> Ok, but I'd prefer a more descriptive name than "gtramp"
> 

I will change the name to be more descriptive.

> I'll note that these are initialized to 0 but code below sets them to -1
> on error; I assume this is intentionally different.
> 

Yes. That is for debugging purposes. If the failure happened before the trampoline
file was even opened, fd would be 0. Otherwise, fd would be 1.

>> +/* --------------------- Trampoline File Initialization --------------------*/
>> +
>> +/*
>> + * The trampoline file is the file used to map the trampoline code table into
>> + * the address space of a process. There are two ways to get this file:
>> + *
>> + * - From the OS. E.g., on Linux, /proc/<pid>/maps lists all the memory
>> + *   mappings for <pid>. For file-backed mappings, maps supplies the file name
>> + *   and the file offset. Using this, we can locate the mapping that maps
>> + *   libffi and get the path to the libffi binary. And, we can compute the
>> + *   offset of the trampoline code table within that binary.
>> + *
>> + * - Else, if we can create a temporary file, we can write the trampoline code
>> + *   table from the text segment into the temporary file.
>> + *
>> + * The first method is the preferred one. If the OS security subsystem
>> + * disallows mapping unsigned files with PROT_EXEC, then the second method
>> + * will fail.
>> + *
>> + * If an OS allows the trampoline code table in the text segment to be
>> + * directly remapped (e.g., MACH vm_remap ()), then we don't need the
>> + * trampoline file.
>> + */
>> +static int tramp_table_alloc (void);
>> +
>> +#if defined __linux__
> 
> Ok
> 
>> +static int
>> +ffi_tramp_get_libffi (void)
>> +{
>> +  FILE *fp;
>> +  char file[PATH_MAX], line[PATH_MAX+100], perm[10], dev[10];
>> +  unsigned long start, end, offset, inode;
>> +  uintptr_t addr = (uintptr_t) gtramp.text;
>> +  int nfields, found;
>> +
>> +  snprintf (file, PATH_MAX, "/proc/%d/maps", getpid());
>> +  fp = fopen (file, "r");
>> +  if (fp == NULL)
>> +    return 0;
>> +
>> +  found = 0;
>> +  while (feof (fp) == 0) {
>> +    if (fgets (line, sizeof (line), fp) == 0)
>> +      break;
>> +
>> +    nfields = sscanf (line, "%lx-%lx %s %lx %s %ld %s",
>> +      &start, &end, perm, &offset, dev, &inode, file);
> 
> Should use field limiting for perm and dev, like %9s.  Yes, I know
> /proc/pid/maps gives fixed fields, but plan for paranoia ;-)

Agreed. I will change it.

> 
>> +    if (nfields != 7)
>> +      continue;
>> +
>> +    if (addr >= start && addr < end) {
>> +      gtramp.offset = offset + (addr - start);
>> +      found = 1;
>> +      break;
>> +    }
>> +  }
>> +  fclose (fp);
>> +
>> +  if (!found)
>> +    return 0;
>> +
>> +  gtramp.fd = open (file, O_RDONLY);
>> +  if (gtramp.fd == -1)
>> +    return 0;
>> +
>> +  /*
>> +   * Allocate a trampoline table just to make sure that the trampoline code
>> +   * table can be mapped.
>> +   */
>> +  if (!tramp_table_alloc ())
>> +    {
>> +      close (gtramp.fd);
>> +      gtramp.fd = -1;
>> +      return 0;
>> +    }
>> +  return 1;
>> +}
> 
> Ok.
> 
>> +#endif /* __linux__ */
>> +
>> +#if defined __linux__
> 
> Messy conditionals

It looks that way because Linux is the only OS that is currently supported. In
version 2, I actually added support for 3 BSD variants as well. Some parts of this
code are common between linux and some BSD variants. Some are not.

I will add the support for BSD once this work gets accepted. Then, the conditionals
will not look messy. The header files and stuff are different across OSes.

> 
>> +#if defined HAVE_MKSTEMP
>> +
>> +static int
>> +ffi_tramp_get_temp_file (void)
>> +{
>> +  char template[12] = "/tmp/XXXXXX";
>> +  ssize_t count;
>> +
>> +  gtramp.offset = 0;
>> +  gtramp.fd = mkstemp (template);
>> +  if (gtramp.fd == -1)
>> +    return 0;
>> +
>> +  unlink (template);
>> +  /*
>> +   * Write the trampoline code table into the temporary file and allocate a
>> +   * trampoline table to make sure that the temporary file can be mapped.
>> +   */
>> +  count = write(gtramp.fd, gtramp.text, gtramp.map_size);
>> +  if (count == gtramp.map_size && tramp_table_alloc ())
>> +    return 1;
>> +
>> +  close (gtramp.fd);
>> +  gtramp.fd = -1;
>> +  return 0;
>> +}
>> +
>> +#else /* !defined HAVE_MKSTEMP */
>> +
>> +/*
>> + * TODO:
>> + * Perhaps, libffi can supply its own version of mkstemp() if it is
>> + * not natively available.
>> + */
>> +static int
>> +ffi_tramp_get_temp_file (void)
>> +{
>> +  gtramp.offset = 0;
>> +  gtramp.fd = -1;
>> +  return 0;
>> +}
>> +
>> +#endif /* defined HAVE_MKSTEMP */
>> +
>> +#endif /* __linux__ */
> 
> Ok.
> 
>> +/* ------------------------ OS-specific Initialization ----------------------*/
>> +
>> +#if defined __linux__
>> +
>> +static int
>> +ffi_tramp_init_os (void)
>> +{
>> +  if (ffi_tramp_get_libffi ())
>> +    return 1;
>> +  return ffi_tramp_get_temp_file ();
>> +}
>> +
>> +#endif /* __linux__ */
> 
> Ok.
> 
>> +/* --------------------------- OS-specific Locking -------------------------*/
>> +
>> +#if defined __linux__
>> +
>> +static pthread_mutex_t gtramp_mutex = PTHREAD_MUTEX_INITIALIZER;
>> +
>> +static void
>> +ffi_tramp_lock(void)
>> +{
>> +  pthread_mutex_lock (&gtramp_mutex);
>> +}
>> +
>> +static void
>> +ffi_tramp_unlock()
>> +{
>> +  pthread_mutex_unlock (&gtramp_mutex);
>> +}
>> +
>> +#endif /* __linux__ */
> 
> Ok.
> 
>> +/* ------------------------ OS-specific Memory Mapping ----------------------*/
>> +
>> +/*
>> + * Create a trampoline code table mapping and a trampoline parameter table
>> + * mapping. The two mappings must be adjacent to each other for PC-relative
>> + * access.
>> + *
>> + * For each trampoline in the code table, there is a corresponding parameter
>> + * block in the parameter table. The size of the parameter block is the same
>> + * as the size of the trampoline. This means that the parameter block is at
>> + * a fixed offset from its trampoline making it easy for a trampoline to find
>> + * its parameters using PC-relative access.
>> + *
>> + * The parameter block will contain a struct tramp_parm. This means that
>> + * sizeof (struct tramp_parm) cannot exceed the size of a parameter block.
>> + */
>> +
>> +#if defined __linux__
>> +
>> +static int
>> +tramp_table_map (struct tramp_table *table)
>> +{
>> +  char *addr;
>> +
>> +  /*
>> +   * Create an anonymous mapping twice the map size. The top half will be used
>> +   * for the code table. The bottom half will be used for the parameter table.
>> +   */
>> +  addr = mmap (NULL, gtramp.map_size * 2, PROT_READ | PROT_WRITE,
>> +    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +  if (addr == MAP_FAILED)
>> +    return 0;
>> +
>> +  /*
>> +   * Replace the top half of the anonymous mapping with the code table mapping.
>> +   */
>> +  table->code_table = mmap (addr, gtramp.map_size, PROT_READ | PROT_EXEC,
>> +    MAP_PRIVATE | MAP_FIXED, gtramp.fd, gtramp.offset);
>> +  if (table->code_table == MAP_FAILED)
>> +    {
>> +      (void) munmap (addr, gtramp.map_size * 2);
>> +      return 0;
>> +    }
>> +  table->parm_table = table->code_table + gtramp.map_size;
>> +  return 1;
>> +}
>> +
>> +static void
>> +tramp_table_unmap (struct tramp_table *table)
>> +{
>> +  (void) munmap (table->code_table, gtramp.map_size);
>> +  (void) munmap (table->parm_table, gtramp.map_size);
>> +}
>> +
>> +#endif /* __linux__ */
> 
> Ok.
> 
>> +/* ------------------------ Trampoline Initialization ----------------------*/
>> +
>> +/*
>> + * Initialize the static trampoline feature.
>> + */
>> +static int
>> +ffi_tramp_init (void)
>> +{
>> +  if (gtramp.status == GTRAMP_PASSED)
>> +    return 1;
>> +
>> +  if (gtramp.status == GTRAMP_FAILED)
>> +    return 0;
>> +
>> +  if (ffi_tramp_arch == NULL)
>> +    {
>> +      gtramp.status = GTRAMP_FAILED;
>> +      return 0;
>> +    }
>> +
>> +  gtramp.tables = NULL;
>> +  gtramp.ntables = 0;
>> +
>> +  /*
>> +   * Get trampoline code table information from the architecture.
>> +   */
>> +  gtramp.text = ffi_tramp_arch (&gtramp.size, &gtramp.map_size);
>> +  gtramp.ntramp = gtramp.map_size / gtramp.size;
>> +
>> +  if (sysconf (_SC_PAGESIZE) > gtramp.map_size)
>> +    return 0;
>> +
>> +  if (ffi_tramp_init_os ())
>> +    {
>> +      gtramp.status = GTRAMP_PASSED;
>> +      return 1;
>> +    }
>> +
>> +  gtramp.status = GTRAMP_FAILED;
>> +  return 0;
>> +}
> 
> Ok.
> 
>> +/* ---------------------- Trampoline Table functions ---------------------- */
>> +
>> +/* This code assumes that malloc () is available on all OSes. */
>> +
>> +static void tramp_add (struct tramp *tramp);
>> +
>> +/*
>> + * Allocate and initialize a trampoline table.
>> + */
>> +static int
>> +tramp_table_alloc (void)
>> +{
>> +  struct tramp_table *table;
>> +  struct tramp *tramp_array, *tramp;
>> +  size_t size;
>> +  char *code, *parm;
>> +  int i;
>> +
>> +  /*
>> +   * If we already have tables with free trampolines, there is no need to
>> +   * allocate a new table.
>> +   */
>> +  if (gtramp.ntables > 0)
>> +    return 1;
> 
> success because there's a free slot somewhere

I will add the comment.

> 
>> +  /*
>> +   * Allocate a new trampoline table structure.
>> +   */
>> +  table = malloc (sizeof (*table));
>> +  if (table == NULL)
>> +    return 0;
> 
> Failure because malloc failure.

Will add a comment.

> 
>> +  /*
>> +   * Allocate new trampoline structures.
>> +   */
>> +  tramp_array = malloc (sizeof (*tramp) * gtramp.ntramp);
>> +  if (tramp_array == NULL)
>> +    goto free_table;
> 
> Failure because malloc failure.

Will add a comment.

> 
> 
>> +  /*
>> +   * Map a code table and a parameter table into the caller's address space.
>> +   */
>> +  if (!tramp_table_map (table))
>> +    goto free_tramp_array;
> 
> Mapping failure

Will add a comment.

> 
>> +  /*
>> +   * Initialize the trampoline table.
>> +   */
>> +  table->array = tramp_array;
>> +  table->free = NULL;
>> +  table->nfree = 0;
>> +
>> +  /*
>> +   * Populate the trampoline table free list. This will also add the trampoline
>> +   * table to the global list of trampoline tables.
>> +   */
>> +  size = gtramp.size;
>> +  code = table->code_table;
>> +  parm = table->parm_table;
>> +  for (i = 0; i < gtramp.ntramp; i++)
>> +    {
>> +      tramp = &tramp_array[i];
>> +      tramp->table = table;
>> +      tramp->code = code;
>> +      tramp->parm = (struct tramp_parm *) parm;
>> +      tramp_add (tramp);
>> +
>> +      code += size;
>> +      parm += size;
>> +    }
>> +  return 1;
> 
> Sucess

Will add a comment.

> 
>> +free_tramp_array:
>> +  free (tramp_array);
>> +free_table:
>> +  free (table);
>> +  return 0;
>> +}
> 
> Ok
> 
>> +/*
>> + * Free a trampoline table.
>> + */
>> +static void
>> +tramp_table_free (struct tramp_table *table)
>> +{
>> +  tramp_table_unmap (table);
>> +  free (table->array);
>> +  free (table);
>> +}
> 
> Ok.
> 
>> +/*
>> + * Add a new trampoline table to the global table list.
>> + */
>> +static void
>> +tramp_table_add (struct tramp_table *table)
>> +{
>> +  table->next = gtramp.tables;
>> +  table->prev = NULL;
>> +  if (gtramp.tables != NULL)
>> +    gtramp.tables->prev = table;
>> +  gtramp.tables = table;
>> +  gtramp.ntables++;
> 
> This assumes that the table being added has free slots.  Do we only ever
> add empty tables?
> 

We only add tables that have at least one free slot. When all slots are filled,
we delete the table from the list.

>> +}
> 
> 
> 
>> +/*
>> + * Delete a trampoline table from the global table list.
>> + */
>> +static void
>> +tramp_table_del (struct tramp_table *table)
>> +{
>> +  gtramp.ntables--;
> 
> Same here.

When I change the name of the field from ntables to something that indicates
tables with free slots, this will make sense.

> 
>> +  if (table->prev != NULL)
>> +    table->prev->next = table->next;
>> +  if (table->next != NULL)
>> +    table->next->prev = table->prev;
>> +  if (gtramp.tables == table)
>> +    gtramp.tables = table->next;
>> +}
> 
> Otherwise OK
> 
>> +/* ------------------------- Trampoline functions ------------------------- */
>> +
>> +/*
>> + * Add a trampoline to its trampoline table.
>> + */
>> +static void
>> +tramp_add (struct tramp *tramp)
>> +{
>> +  struct tramp_table *table = tramp->table;
>> +
>> +  tramp->next = table->free;
>> +  tramp->prev = NULL;
>> +  if (table->free != NULL)
>> +    table->free->prev = tramp;
>> +  table->free = tramp;
>> +  table->nfree++;
>> +
>> +  if (table->nfree == 1)
>> +    tramp_table_add (table);
>> +
>> +  /*
>> +   * We don't want to keep too many free trampoline tables lying around.
>> +   */
>> +  if (table->nfree == gtramp.ntramp && gtramp.ntables > 1)
> 
> You can't compare these.  The first is a count of *trampolines* and the
> second is a count of *tables*.
> 

There are two conditions here:

If (table->nfree == gtramp.ntramp) is true, then all of the table's slots are free.

If (gtramp.ntables > 1) is true, then there is at least one other table that has
free slots.

My understanding of precedence rules is that the two conditions will first be evaluated
and then the results will be ANDed. Is that not correct?

So this is saying that if all slots of a table have become free, free the table and all of the
trampolines in it provided there is at least one other table that has free slots.


>> +    {
>> +      tramp_table_del (table);
>> +      tramp_table_free (table);
>> +    }
>> +}
> 
>> +/*
>> + * Remove a trampoline from its trampoline table.
>> + */
>> +static void
>> +tramp_del (struct tramp *tramp)
>> +{
>> +  struct tramp_table *table = tramp->table;
>> +
>> +  table->nfree--;
>> +  if (tramp->prev != NULL)
>> +    tramp->prev->next = tramp->next;
>> +  if (tramp->next != NULL)
>> +    tramp->next->prev = tramp->prev;
>> +  if (table->free == tramp)
>> +    table->free = tramp->next;
>> +
>> +  if (table->nfree == 0)
>> +    tramp_table_del (table);
>> +}
> 
> tramp_table_del unlinks the table, but doesn't deallocate its
> resources.  Are these resources lost/leaked?
> 

The table list contains tables that have free slots. If all slots of a table
are allocated, the table is removed from the list. Its trampoline resources
are held inside closures. When those closures are freed, the table will be back
on the list. When the table gets back all of its trampolines, it will be freed
along with all of its trampolines.


>> +/* ------------------------ Trampoline API functions ------------------------ */
>> +
>> +int
>> +ffi_tramp_is_supported(void)
>> +{
>> +  int ret;
>> +
>> +  ffi_tramp_lock();
>> +  ret = ffi_tramp_init ();
>> +  ffi_tramp_unlock();
>> +  return ret;
>> +}
> 
> Ok.
> 
>> +/*
>> + * Allocate a trampoline and return its opaque address.
>> + */
>> +void *
>> +ffi_tramp_alloc (int flags)
>> +{
>> +  struct tramp *tramp;
>> +
>> +  ffi_tramp_lock();
>> +
>> +  if (!ffi_tramp_init () || flags != 0)
>> +    {
>> +      ffi_tramp_unlock();
>> +      return NULL;
>> +    }
>> +
>> +  if (!tramp_table_alloc ())
>> +    {
>> +      ffi_tramp_unlock();
>> +      return NULL;
>> +    }
>> +
>> +  tramp = gtramp.tables->free;
>> +  tramp_del (tramp);
>> +
>> +  ffi_tramp_unlock();
>> +
>> +  return tramp;
>> +}
> 
> Ok.
> 
>> +/*
>> + * Set the parameters for a trampoline.
>> + */
>> +void
>> +ffi_tramp_set_parms (void *arg, void *target, void *data)
>> +{
>> +  struct tramp *tramp = arg;
>> +
>> +  ffi_tramp_lock();
>> +  tramp->parm->target = target;
>> +  tramp->parm->data = data;
>> +  ffi_tramp_unlock();
>> +}
> 
> Ok.
> 
>> +/*
>> + * Get the invocation address of a trampoline.
>> + */
>> +void *
>> +ffi_tramp_get_addr (void *arg)
>> +{
>> +  struct tramp *tramp = arg;
>> +  void *addr;
>> +
>> +  ffi_tramp_lock();
>> +  addr = tramp->code;
>> +  ffi_tramp_unlock();
>> +
>> +  return addr;
>> +}
> 
> Ok.
> 
>> +/*
>> + * Free a trampoline.
>> + */
>> +void
>> +ffi_tramp_free (void *arg)
>> +{
>> +  struct tramp *tramp = arg;
>> +
>> +  ffi_tramp_lock();
>> +  tramp_add (tramp);
>> +  ffi_tramp_unlock();
>> +}
> 
> Ok.
> 
>> +/* ------------------------------------------------------------------------- */
>> +
>> +#else /* !FFI_EXEC_STATIC_TRAMP */
>> +
>> +#include <stddef.h>
>> +
>> +int
>> +ffi_tramp_is_supported(void)
>> +{
>> +  return 0;
>> +}
>> +
>> +void *
>> +ffi_tramp_alloc (int flags)
>> +{
>> +  return NULL;
>> +}
>> +
>> +void
>> +ffi_tramp_set_parms (void *arg, void *target, void *data)
>> +{
>> +}
>> +
>> +void *
>> +ffi_tramp_get_addr (void *arg)
>> +{
>> +  return NULL;
>> +}
>> +
>> +void
>> +ffi_tramp_free (void *arg)
>> +{
>> +}
>> +
>> +#endif /* FFI_EXEC_STATIC_TRAMP */
> 
> Ok.
> 
>> +#ifndef FFI_EXEC_STATIC_TRAMP
>> +  /* With static trampolines, the codeloc does not point to closure */
>>    CHECK(memcmp(pcl, codeloc, sizeof(*pcl)) == 0);
>> +#endif
> 
> Ok.
> 

Thanks again for the thorough review. If I have missed addressing any of your
comments, please let me know.

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 1/5] Libffi Static Trampolines
  2021-01-27 21:51       ` Madhavan T. Venkataraman
@ 2021-01-27 22:15         ` DJ Delorie
  2021-01-27 22:43           ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: DJ Delorie @ 2021-01-27 22:15 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss

"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
>>> +case "$target" in
>>> +     *-linux*)
>>> +       AC_DEFINE(FFI_EXEC_STATIC_TRAMP, 1,
>>> +                 [Define this if you want statically defined trampolines])
>>> +     ;;
>>> +esac
>> 
>> Ok.  Might want to conditional for the arches that are supported too.
>> 
>
> ok. I will add the arches although the code currently handles that by testing
> ffi_tramp_arch == NULL.

/me ponders...

Is the intent here that a Linux client has to check for static
trampolines in configure (i.e. it's on linux) but *also* check if it's
supported at runtime?  If we could minimize the number of checks, that
would help the developer.  If the API is always present (or always
hidden, so that the client need do nothing), that's best.

Ideally, the client would need do nothing regardless, but that only
works if the static trampoline functionality is 100% hidden.

> My intention is to allow applications and libraries that currently use dynamic code
> to use the libffi API to eliminate their dynamic code. The maintainers of these
> apps/libs would rewrite their dynamic code into static code. All they need is a
> way to pass data to their static code (that uses PC-relative access). The libffi
> API will let them do that.

Ok, as long as there's a use case.

Note that adding public APIs is MUCH easier than removing them later...

> My intention is to always have the API functions defined in the library. If
> FFI_EXEC_STATIC_TRAMP is defined, then, the API will supply static trampolines.
> Else, the stub API functions will return an error and the caller can fall back
> on his existing method if any.

Ok.  I guess that answers one of my earlier questions (above) ;-)

>>> @@ -943,12 +966,17 @@ void *
>>>  ffi_data_to_code_pointer (void *data)
>>>  {
>>>    msegmentptr seg = segment_holding (gm, data);
>>> +
>> 
>> Extraneous but ok.
>
> This is needed.

I meant the extra blank line ;-)

>>> +  if (table->nfree == gtramp.ntramp && gtramp.ntables > 1)
>> 
>> You can't compare these.  The first is a count of *trampolines* and the
>> second is a count of *tables*.
>> 
>
> There are two conditions here:
>
> If (table->nfree == gtramp.ntramp) is true, then all of the table's slots are free.

This reads as "If the number of trampolines equals the number of tables"
which are not comparable.  If you're doing some math magic hack here to
get the right answer, it *really* needs a big comment explaining how it
works.

>> tramp_table_del unlinks the table, but doesn't deallocate its
>> resources.  Are these resources lost/leaked?
>> 
>
> The table list contains tables that have free slots. If all slots of a table
> are allocated, the table is removed from the list. Its trampoline resources
> are held inside closures. When those closures are freed, the table will be back
> on the list. When the table gets back all of its trampolines, it will be freed
> along with all of its trampolines.

Ah, ok.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 1/5] Libffi Static Trampolines
  2021-01-27 22:15         ` DJ Delorie
@ 2021-01-27 22:43           ` Madhavan T. Venkataraman
  0 siblings, 0 replies; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-27 22:43 UTC (permalink / raw)
  To: DJ Delorie; +Cc: libffi-discuss



On 1/27/21 4:15 PM, DJ Delorie wrote:
> "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
>>>> +case "$target" in
>>>> +     *-linux*)
>>>> +       AC_DEFINE(FFI_EXEC_STATIC_TRAMP, 1,
>>>> +                 [Define this if you want statically defined trampolines])
>>>> +     ;;
>>>> +esac
>>>
>>> Ok.  Might want to conditional for the arches that are supported too.
>>>
>>
>> ok. I will add the arches although the code currently handles that by testing
>> ffi_tramp_arch == NULL.
> 
> /me ponders...
> 
> Is the intent here that a Linux client has to check for static
> trampolines in configure (i.e. it's on linux) but *also* check if it's
> supported at runtime?  If we could minimize the number of checks, that
> would help the developer.  If the API is always present (or always
> hidden, so that the client need do nothing), that's best.
> 
> Ideally, the client would need do nothing regardless, but that only
> works if the static trampoline functionality is 100% hidden.
> 

libffi clients do not need to do anything. They do not even need to recompile.
They call the closure API which internally uses static trampolines (if supported)
or legacy trampolines (if not supported). As far as they are concerned, static
trampolines are 100% hidden.

For apps/libraries that wish to use the static trampoline API directly in the future,
there will be changes to use the API (and to refactor their code, if necessary). See
the examples I sent to AG.

The above ffi_tramp_arch check is done internally by the API to see if the
underlying architecture supports it or not. So, let us say libffi is running
on linux on SPARC. There is no ffi_tramp_arch() defined for SPARC. So, the API
functions will all return 0 and the caller (the closure code) will fallback to
using the legacy trampolines.

>> My intention is to allow applications and libraries that currently use dynamic code
>> to use the libffi API to eliminate their dynamic code. The maintainers of these
>> apps/libs would rewrite their dynamic code into static code. All they need is a
>> way to pass data to their static code (that uses PC-relative access). The libffi
>> API will let them do that.
> 
> Ok, as long as there's a use case.
> 

I know that there are a lot of use cases out there. But I have not been able to find
one in open source by googling. If someone can point me to an open source app/lib
that uses dynamic code, I could attempt to show how to use the static trampoline
API to replace the dynamic code with static code.

> Note that adding public APIs is MUCH easier than removing them later...
> 

Agreed. So, I have suggested to Anthony that we could expose the public API
after these changes have soaked in libffi for some time and have been well
exercised. I could submit a patch after, say, 6 months just to expose the
API.

>> My intention is to always have the API functions defined in the library. If
>> FFI_EXEC_STATIC_TRAMP is defined, then, the API will supply static trampolines.
>> Else, the stub API functions will return an error and the caller can fall back
>> on his existing method if any.
> 
> Ok.  I guess that answers one of my earlier questions (above) ;-)
> 
>>>> @@ -943,12 +966,17 @@ void *
>>>>  ffi_data_to_code_pointer (void *data)
>>>>  {
>>>>    msegmentptr seg = segment_holding (gm, data);
>>>> +
>>>
>>> Extraneous but ok.
>>
>> This is needed.
> 
> I meant the extra blank line ;-)

I am sorry I misunderstood. I will delete the extra blank line.

> 
>>>> +  if (table->nfree == gtramp.ntramp && gtramp.ntables > 1)
>>>
>>> You can't compare these.  The first is a count of *trampolines* and the
>>> second is a count of *tables*.
>>>
>>
>> There are two conditions here:
>>
>> If (table->nfree == gtramp.ntramp) is true, then all of the table's slots are free.
> 
> This reads as "If the number of trampolines equals the number of tables"
> which are not comparable.  If you're doing some math magic hack here to
> get the right answer, it *really* needs a big comment explaining how it
> works.
> 

gtramp.ntramp is not the number of tables. It is the number of trampolines
in a table. Here is the comment that describes the field:

/*
 * Trampoline globals.
...
 * ntramp
 *      Total number of trampolines in the trampoline code table.
...
 */

Here is how it gets initialized:

  /*
   * Get trampoline code table information from the architecture.
   */
  gtramp.text = ffi_tramp_arch (&gtramp.size, &gtramp.map_size);
  gtramp.ntramp = gtramp.map_size / gtramp.size;

gtramp.map_size is the size of a single code table mapping. gtramp.size
is the size of a single trampoline. So, gtramp.ntramp is the integral
number of trampolines that fit into a single code table.

>>> tramp_table_del unlinks the table, but doesn't deallocate its
>>> resources.  Are these resources lost/leaked?
>>>
>>
>> The table list contains tables that have free slots. If all slots of a table
>> are allocated, the table is removed from the list. Its trampoline resources
>> are held inside closures. When those closures are freed, the table will be back
>> on the list. When the table gets back all of its trampolines, it will be freed
>> along with all of its trampolines.
> 
> Ah, ok.
> 

Thanks!

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-27 19:45         ` Madhavan T. Venkataraman
@ 2021-01-28 14:21           ` Anthony Green
  2021-01-28 17:01             ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: Anthony Green @ 2021-01-28 14:21 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss, fweimer, DJ Delorie

On Wed, Jan 27, 2021 at 2:45 PM Madhavan T. Venkataraman <
madvenka@linux.microsoft.com> wrote:

> On 1/27/21 12:00 PM, Anthony Green wrote:
> > Thanks, Madhaven.   I think I understand now.   Are these statements
> true?:
> >
> > (a) These patches implement trampoline tables, similar to what is
> implemented in the iOS port.  This hardens the library by eliminating the
> requirement for writable executable memory.
> > (b) These patches expose a new public API for hardened trampolines.  (a)
> uses (b), but doesn't require that (b) be public.
> > (c) We can release libffi with (a) and not (b).
> >
> > Is this correct?
> >
>
> Yes. This is correct. The public API part is not required.
>

In this case, my ask is that you split this into two patches.  The first
one, (a), I would like to merge as soon as it seems ready.

The second one I'm not so sure about yet.  Is it a solution in search of a
problem?  Is libffi the right place for it?   Should it live in libffi, or
be broken out into something that libstatictramp -- that perhaps lives in
the libffi repo but doesn't pollute the libffi API?  I don't know yet, but
opinions welcome.

Also, please use the github PR process for the next round so we can get
some automated CI going.  And I prefer reviewing patches within the github
UI, tbh.

And again -- I appreciate your effort here.  This is something I've wanted
more broadly in libffi ever since Landon Fuller implemented something
similar in the iOS port.

AG

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-28 14:21           ` Anthony Green
@ 2021-01-28 17:01             ` Madhavan T. Venkataraman
  2021-02-05 18:20               ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-28 17:01 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, fweimer, DJ Delorie



On 1/28/21 8:21 AM, Anthony Green wrote:
> On Wed, Jan 27, 2021 at 2:45 PM Madhavan T. Venkataraman <madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com>> wrote:
> 
>     On 1/27/21 12:00 PM, Anthony Green wrote:
>     > Thanks, Madhaven.   I think I understand now.   Are these statements true?:
>     >
>     > (a) These patches implement trampoline tables, similar to what is implemented in the iOS port.  This hardens the library by eliminating the requirement for writable executable memory.
>     > (b) These patches expose a new public API for hardened trampolines.  (a) uses (b), but doesn't require that (b) be public.
>     > (c) We can release libffi with (a) and not (b).
>     >
>     > Is this correct?  
>     >
> 
>     Yes. This is correct. The public API part is not required.
> 
> 
> In this case, my ask is that you split this into two patches.  The first one, (a), I would like to merge as soon as it seems ready.  
> 

OK. I will remove the public API part, sync with the latest libffi and submit a PR.

> The second one I'm not so sure about yet.  Is it a solution in search of a problem?

The problem currently exists in a lot of applications. However, I have not been able to find
a good example in open source. It is all closed source.

> Is libffi the right place for it?   Should it live in libffi, or be broken out into something that libstatictramp -- that perhaps lives in the libffi repo but doesn't pollute the libffi API?  I don't know yet, but opinions welcome.

I like the idea of a libtramp living in the same repo publishing its own API.

First, let us just focus on (a).

Once that has been merged, I will work on the static trampoline library. I have never done library work
before. So, I will need guidance on library creation, configuration, how to expose an API, how to version it,
etc. I will pester you at that time with my questions. We will cross the bridge when we come to it.

> 
> Also, please use the github PR process for the next round so we can get some automated CI going.  And I prefer reviewing patches within the github UI, tbh.
> 

OK. Will do.

> And again -- I appreciate your effort here.  This is something I've wanted more broadly in libffi ever since Landon Fuller implemented something similar in the iOS port.

Thanks!

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-27  3:31     ` DJ Delorie
@ 2021-01-28 21:59       ` Madhavan T. Venkataraman
  2021-01-28 22:17         ` DJ Delorie
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-28 21:59 UTC (permalink / raw)
  To: DJ Delorie; +Cc: libffi-discuss

Thanks for reviewing this. My responses inline...

On 1/26/21 9:31 PM, DJ Delorie wrote:
> 
> madvenka@linux.microsoft.com writes:
> 
>> diff --git a/src/x86/ffi64.c b/src/x86/ffi64.c
>> index 39f9598..2a5cf5a 100644
>> --- a/src/x86/ffi64.c
>> +++ b/src/x86/ffi64.c
>> @@ -713,7 +713,9 @@ ffi_call_go (ffi_cif *cif, void (*fn)(void), void *rvalue,
>>  #endif /* FFI_GO_CLOSURES */
>>  
>>  extern void ffi_closure_unix64(void) FFI_HIDDEN;
>> +extern void ffi_closure_unix64_alt(void) FFI_HIDDEN;
>>  extern void ffi_closure_unix64_sse(void) FFI_HIDDEN;
>> +extern void ffi_closure_unix64_sse_alt(void) FFI_HIDDEN;
> 
> Extern, but local to this port, yes?
> 

Yes. So, is this declaration acceptable?

>> @@ -742,6 +744,7 @@ ffi_prep_closure_loc (ffi_closure* closure,
>>      0x0f, 0x1f, 0x80, 0x00, 0x00, 0x00, 0x00
>>    };
>>    void (*dest)(void);
>> +  void (*dest_alt)(void);
>>    char *tramp = closure->tramp;
> 
> Ok
> 
>> @@ -752,13 +755,28 @@ ffi_prep_closure_loc (ffi_closure* closure,
>>      return FFI_BAD_ABI;
>>  
>>    if (cif->flags & UNIX64_FLAG_XMM_ARGS)
>> -    dest = ffi_closure_unix64_sse;
>> +    {
>> +      dest = ffi_closure_unix64_sse;
>> +      dest_alt = ffi_closure_unix64_sse_alt;
>> +    }
>>    else
>> -    dest = ffi_closure_unix64;
>> +    {
>> +      dest = ffi_closure_unix64;
>> +      dest_alt = ffi_closure_unix64_alt;
>> +    }
>>  
>> +  if (ffi_tramp_is_present(closure))
>> +    {
>> +      /* Initialize the static trampoline's parameters. */
>> +      ffi_tramp_set_parms (closure->ftramp, dest_alt, closure);
>> +      goto out;
>> +    }
>> +
>> +  /* Initialize the dynamic trampoline. */
> 
> Should these new APIs be inside #if FFI_EXEC_STATIC_TRAMP ?
> 

Strictly speaking, these should be inside that ifdef. I did it this
way to avoid too many ifdefs in the code. If you prefer I put them
inside the ifdefs, I will do it. I will try to minimize the number of
ifdefs somehow.

Please confirm.

>> +#if defined(FFI_EXEC_STATIC_TRAMP)
>> +void *
>> +ffi_tramp_arch (size_t *tramp_size, size_t *map_size)
>> +{
>> +  extern void *trampoline_code_table_cet;
>> +  extern void *trampoline_code_table;
>> +  extern int ffi_cet_present;
>> +
>> +  *map_size = UNIX64_TRAMP_MAP_SIZE;
>> +  if (ffi_cet_present) {
>> +    *tramp_size = UNIX64_TRAMP_SIZE_CET;
>> +    return &trampoline_code_table_cet;
>> +  }
>> +  *tramp_size = UNIX64_TRAMP_SIZE;
>> +  return &trampoline_code_table;
>> +}
>> +#endif
> 
> Ok.
> 
>> diff --git a/src/x86/ffiw64.c b/src/x86/ffiw64.c
>> index a43a9eb..df81d66 100644
>> --- a/src/x86/ffiw64.c
>> +++ b/src/x86/ffiw64.c
>> @@ -187,6 +187,7 @@ EFI64(ffi_call_go)(ffi_cif *cif, void (*fn)(void), void *rvalue,
>>  
>>  
>>  extern void ffi_closure_win64(void) FFI_HIDDEN;
>> +extern void ffi_closure_win64_alt(void) FFI_HIDDEN;
>>  
>>  #ifdef FFI_GO_CLOSURES
>>  extern void ffi_go_closure_win64(void) FFI_HIDDEN;
>> @@ -220,9 +221,18 @@ EFI64(ffi_prep_closure_loc)(ffi_closure* closure,
>>        return FFI_BAD_ABI;
>>      }
>>  
>> +  if (ffi_tramp_is_present(closure))
>> +    {
>> +      /* Initialize the static trampoline's parameters. */
>> +      ffi_tramp_set_parms (closure->ftramp, ffi_closure_win64_alt, closure);
>> +      goto out;
>> +    }
>> +
>> +  /* Initialize the dynamic trampoline. */
>>    memcpy (tramp, trampoline, sizeof(trampoline));
>>    *(UINT64 *)(tramp + sizeof (trampoline)) = (uintptr_t)ffi_closure_win64;
>>  
>> +out:
>>    closure->cif = cif;
>>    closure->fun = fun;
>>    closure->user_data = user_data;
> 
> Ok.
> 
>> diff --git a/src/x86/internal64.h b/src/x86/internal64.h
>> index 512e955..410bdf2 100644
>> --- a/src/x86/internal64.h
>> +++ b/src/x86/internal64.h
>> @@ -20,3 +20,14 @@
>>  #define UNIX64_FLAG_RET_IN_MEM	(1 << 10)
>>  #define UNIX64_FLAG_XMM_ARGS	(1 << 11)
>>  #define UNIX64_SIZE_SHIFT	12
>> +
>> +#if defined(FFI_EXEC_STATIC_TRAMP)
>> +/*
>> + * For the trampoline code table mapping, a mapping size of 4K (base page size)
>> + * is chosen.
>> + */
>> +#define UNIX64_TRAMP_MAP_SHIFT	12
>> +#define UNIX64_TRAMP_MAP_SIZE	(1 << UNIX64_TRAMP_MAP_SHIFT)
>> +#define UNIX64_TRAMP_SIZE_CET	40
>> +#define UNIX64_TRAMP_SIZE	32
>> +#endif
> 
> Ok.
> 
>> diff --git a/src/x86/unix64.S b/src/x86/unix64.S
>> index 89d7db1..e26ea2c 100644
>> --- a/src/x86/unix64.S
>> +++ b/src/x86/unix64.S
>> @@ -63,6 +63,7 @@
>>  C(ffi_call_unix64):
>>  L(UW0):
>>  	_CET_ENDBR
>> +L(endbr):
> 
> This hack to detect CET should be replaced by the logic in ffitarget.h,
> or add a #define CET_ENABLED to ffitarget.h
> 

So, _CET_ENDBR for x64 is either defined as:

If CET is present:
	#define _CET_ENDBR	endbr64
Otherwise:
	#define _CET_ENDBR

So, it is always defined. So, I cannot do something like:

#ifdef _CET_ENDBR

This will always be true.

I did not find a standard, acceptable, compatible preprocessor way to
test the actual value of a preprocessor symbol. Like _CET_ENDBR == endbr64. A couple
of hacky solutions are mentioned in some posts when I googled the topic. There did
not seem to be a standard way.

The only way is for me to use information in cet.h directly. cet.h says this:

# if defined __CET__ && (__CET__ & 1) != 0
#  ifdef __x86_64__
#   define _CET_ENDBR endbr64
#  else
#   define _CET_ENDBR endbr32
#  endif
# else
#  define _CET_ENDBR
# endif

I wasn't sure if I am allowed to use (__CET__ & 1) != 0 in libffi. Is this internal to
cet.h? If not, I can implement what you requested.

Is this acceptable?


>> @@ -270,6 +271,17 @@ L(UW6):
>>  L(UW7):
>>  ENDF(C(ffi_closure_unix64_sse))
>>  
>> +	.balign	2
>> +	.globl	C(ffi_closure_unix64_sse_alt)
>> +	FFI_HIDDEN(C(ffi_closure_unix64_sse_alt))
>> +
>> +C(ffi_closure_unix64_sse_alt):
>> +	_CET_ENDBR
>> +	movq	8(%rsp), %r10
>> +	addq	$16, %rsp
> 
> Copies first argument to %r10, discards return address and arg - closure
> will return to whoever called it's caller.  I'm not sure how this works,
> which means *at least* a comment needs to be here ;-)
> 

Control is transferred to the alt entry points via the static trampoline.
Here is the comment above the static trampoline code table definition
about each individual trampoline in the table:

/*
 * The trampoline uses register r10. It saves the original value of r10 on
 * the stack.
 *
 * The trampoline has two parameters - target code to jump to and data for
 * the target code. The trampoline extracts the parameters from its parameter
 * block (see tramp_table_map()). The trampoline saves the data address on
 * the stack. Finally, it jumps to the target code.
 *
 * The target code can choose to:
 *
 * - restore the value of r10
 * - load the data address in a register
 * - restore the stack pointer to what it was when the trampoline was invoked.
 */

The target code in this case is the alt entry point.

This is what the alt code is doing:
	- load the data (closure) address in r10
	- discard the original value of r10 saved on the stack
	  since we are using r10 we don't need its original value
	- restore the stack back to what it was when the static trampoline was
	  invoked.

So, the above prolog sets it up the way ffi_closure_unix64_sse expects to find it.

I could add a small comment saying "see comment above trampoline_code_table".
Is that acceptable?

>> +	jmp	C(ffi_closure_unix64_sse)
>> +ENDF(C(ffi_closure_unix64_sse_alt))
>> +
>>  	.balign	2
>>  	.globl	C(ffi_closure_unix64)
>>  	FFI_HIDDEN(C(ffi_closure_unix64))
>> @@ -400,6 +412,17 @@ L(la):	call	PLT(C(abort))
>>  L(UW11):
>>  ENDF(C(ffi_closure_unix64))
>>  
>> +	.balign	8
>> +	.globl	C(ffi_closure_unix64_alt)
>> +	FFI_HIDDEN(C(ffi_closure_unix64_alt))
>> +
>> +C(ffi_closure_unix64_alt):
>> +	_CET_ENDBR
>> +	movq	8(%rsp), %r10
>> +	addq	$16, %rsp
>> +	jmp	C(ffi_closure_unix64)
>> +	ENDF(C(ffi_closure_unix64_alt))
>> +
>>  	.balign	2
>>  	.globl	C(ffi_go_closure_unix64_sse)
>>  	FFI_HIDDEN(C(ffi_go_closure_unix64_sse))
> 
> Likewise.
> 

See above.

>> +/*
>> + * The trampoline uses register r10. It saves the original value of r10 on
>> + * the stack.
>> + *
>> + * The trampoline has two parameters - target code to jump to and data for
>> + * the target code. The trampoline extracts the parameters from its parameter
>> + * block (see tramp_table_map()). The trampoline saves the data address on
>> + * the stack. Finally, it jumps to the target code.
>> + *
>> + * The target code can choose to:
>> + *
>> + * - restore the value of r10
>> + * - load the data address in a register
>> + * - restore the stack pointer to what it was when the trampoline was invoked.
>> + */
>> +
>> +	.align	UNIX64_TRAMP_MAP_SIZE
>> +	.globl	trampoline_code_table_cet
>> +	FFI_HIDDEN(C(trampoline_code_table_cet))
>> +
>> +C(trampoline_code_table_cet):
>> +	.rept	UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE_CET
>> +	_CET_ENDBR
>> +	subq	$16, %rsp		/* Make space on the stack */
>> +	movq	%r10, (%rsp)		/* Save %r10 on stack */
>> +	movq	4077(%rip), %r10	/* Copy data into %r10 */
>> +	movq	%r10, 8(%rsp)		/* Save data on stack */
>> +	movq	4073(%rip), %r10	/* Copy code into %r10 */
>> +	jmp	*%r10			/* Jump to code */
>> +	nop
>> +	nop
>> +	nop
>> +	nop
>> +	nop
>> +	nop
>> +	.endr
>> +ENDF(C(trampoline_code_table_cet))
>> +
>> +	.align	UNIX64_TRAMP_MAP_SIZE
>> +	.globl	trampoline_code_table
>> +	FFI_HIDDEN(C(trampoline_code_table))
>> +
>> +C(trampoline_code_table):
>> +	.rept	UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE
>> +	subq	$16, %rsp		/* Make space on the stack */
>> +	movq	%r10, (%rsp)		/* Save %r10 on stack */
>> +	movq	4081(%rip), %r10	/* Copy data into %r10 */
>> +	movq	%r10, 8(%rsp)		/* Save data on stack */
>> +	movq	4077(%rip), %r10	/* Copy code into %r10 */
>> +	jmp	*%r10			/* Jump to code */
>> +	nop
>> +	nop
>> +	.endr
>> +ENDF(C(trampoline_code_table))
>> +	.align	UNIX64_TRAMP_MAP_SIZE
>> +#endif /* FFI_EXEC_STATIC_TRAMP */
> 
> Why does the longer trampoline (with endbr) have *more* nops?  Is it for
> 8-byte alignment?  If so, comment ;-)
> 

Yes. It is for 8-byte alignment. I will add the comment.

>> @@ -615,6 +712,13 @@ L(EFDE5):
>>  	.quad    0
>>  #endif
>>  
>> +	.section .rodata
>> +	.align 8
>> +	.globl ffi_cet_present
>> +ffi_cet_present:
>> +	.set	L6,L(endbr)-L(UW0)
>> +	.int	L6
>> +
> 
> Again, there are preprocessor directives that do this better.
> 

See my explanation above and comment on whether the alternative is acceptable to you.

>> diff --git a/src/x86/win64.S b/src/x86/win64.S
>> index 8315e8b..6ca3068 100644
>> --- a/src/x86/win64.S
>> +++ b/src/x86/win64.S
>> @@ -234,6 +234,18 @@ C(ffi_closure_win64):
>>  
>>  	cfi_endproc
>>  	SEH(.seh_endproc)
>> +
>> +	.align	8
>> +	.globl	C(ffi_closure_win64_alt)
>> +	FFI_HIDDEN(C(ffi_closure_win64_alt))
>> +
>> +	SEH(.seh_proc ffi_closure_win64_alt)
>> +C(ffi_closure_win64_alt):
>> +	_CET_ENDBR
>> +	movq	8(%rsp), %r10
>> +	addq	$16, %rsp
>> +	jmp	C(ffi_closure_win64)
>> +	SEH(.seh_endproc)
>>  #endif /* __x86_64__ */
> 
> Ok.
> 

Again, thanks so much for the thorough review!
Appreciate it!

If I have missed anything, please let me know.

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-28 21:59       ` Madhavan T. Venkataraman
@ 2021-01-28 22:17         ` DJ Delorie
  2021-01-28 23:25           ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: DJ Delorie @ 2021-01-28 22:17 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss

"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
>> Extern, but local to this port, yes?
>
> Yes. So, is this declaration acceptable?

Yup!

>>> +  /* Initialize the dynamic trampoline. */
>> 
>> Should these new APIs be inside #if FFI_EXEC_STATIC_TRAMP ?
>> 
>
> Strictly speaking, these should be inside that ifdef. I did it this
> way to avoid too many ifdefs in the code. If you prefer I put them
> inside the ifdefs, I will do it. I will try to minimize the number of
> ifdefs somehow.

No, it's ok, I was just worried that if ffi_tramp_is_present was in an
#ifdef any callers would be too - but as you noted in another email,
there's always a ffi_tramp_is_present even if it always returns false.

>> This hack to detect CET should be replaced by the logic in ffitarget.h,
>> or add a #define CET_ENABLED to ffitarget.h
>> 
>
> So, _CET_ENDBR for x64 is either defined as:
>
> If CET is present:
> 	#define _CET_ENDBR	endbr64
> Otherwise:
> 	#define _CET_ENDBR
>
> So, it is always defined. So, I cannot do something like:

I was thinking of the conditionals in src/x86/ffitarget.h:

#if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \
    && defined(__CET__)

Obviously you'd omit the __ASSEMBLER__ one ;-)

If you duplicate the ffitarget.h logic you get the same results.

>> Copies first argument to %r10, discards return address and arg - closure
>> will return to whoever called it's caller.  I'm not sure how this works,
>> which means *at least* a comment needs to be here ;-)
>> 
> The target code in this case is the alt entry point.
>
> This is what the alt code is doing:
> 	- load the data (closure) address in r10
> 	- discard the original value of r10 saved on the stack
> 	  since we are using r10 we don't need its original value
> 	- restore the stack back to what it was when the static trampoline was
> 	  invoked.

These kinds of short notes should be useful inline comments in the assembler:

	movq	8(%rsp), %r10	/* load closure */

> I could add a small comment saying "see comment above trampoline_code_table".
> Is that acceptable?

That would be good too :-)


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-28 22:17         ` DJ Delorie
@ 2021-01-28 23:25           ` Madhavan T. Venkataraman
  2021-01-29  2:09             ` DJ Delorie
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-28 23:25 UTC (permalink / raw)
  To: DJ Delorie; +Cc: libffi-discuss



On 1/28/21 4:17 PM, DJ Delorie wrote:
> "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
>>> Extern, but local to this port, yes?
>>
>> Yes. So, is this declaration acceptable?
> 
> Yup!

Great.

> 
>>>> +  /* Initialize the dynamic trampoline. */
>>>
>>> Should these new APIs be inside #if FFI_EXEC_STATIC_TRAMP ?
>>>
>>
>> Strictly speaking, these should be inside that ifdef. I did it this
>> way to avoid too many ifdefs in the code. If you prefer I put them
>> inside the ifdefs, I will do it. I will try to minimize the number of
>> ifdefs somehow.
> 
> No, it's ok, I was just worried that if ffi_tramp_is_present was in an
> #ifdef any callers would be too - but as you noted in another email,
> there's always a ffi_tramp_is_present even if it always returns false.
> 

ok.

>>> This hack to detect CET should be replaced by the logic in ffitarget.h,
>>> or add a #define CET_ENABLED to ffitarget.h
>>>
>>
>> So, _CET_ENDBR for x64 is either defined as:
>>
>> If CET is present:
>> 	#define _CET_ENDBR	endbr64
>> Otherwise:
>> 	#define _CET_ENDBR
>>
>> So, it is always defined. So, I cannot do something like:
> 
> I was thinking of the conditionals in src/x86/ffitarget.h:
> 
> #if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \
>     && defined(__CET__)
> 
> Obviously you'd omit the __ASSEMBLER__ one ;-)

So, the definition is like this:

#if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \
    && defined(__CET__)
# include <cet.h>
# define _CET_NOTRACK notrack
#else
# define _CET_ENDBR
# define _CET_NOTRACK
#endif

Where we include cet.h, CET_ENDBR may still be defined either
as endbr64 or empty depending on __CET__ & 1 being non-zero or
zero, right?

So, would something like this work?

#if !defined(GENERATE_LIBFFI_MAP) && defined(__CET__)
# include <cet.h>
# if (__CET__ & 1) != 0
#  define ENDBR_PRESENT
# endif
# define _CET_NOTRACK notrack
#else
# define _CET_ENDBR
# define _CET_NOTRACK
#endif

Then, I can do:

#ifdef ENDBR_PRESENT
blah blah

> 
> If you duplicate the ffitarget.h logic you get the same results.
> 
>>> Copies first argument to %r10, discards return address and arg - closure
>>> will return to whoever called it's caller.  I'm not sure how this works,
>>> which means *at least* a comment needs to be here ;-)
>>>
>> The target code in this case is the alt entry point.
>>
>> This is what the alt code is doing:
>> 	- load the data (closure) address in r10
>> 	- discard the original value of r10 saved on the stack
>> 	  since we are using r10 we don't need its original value
>> 	- restore the stack back to what it was when the static trampoline was
>> 	  invoked.
> 
> These kinds of short notes should be useful inline comments in the assembler:
> 
> 	movq	8(%rsp), %r10	/* load closure */
> 

I will add this.

>> I could add a small comment saying "see comment above trampoline_code_table".
>> Is that acceptable?
> 
> That would be good too :-)
> 

I will add this as well.

Thanks!

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-28 23:25           ` Madhavan T. Venkataraman
@ 2021-01-29  2:09             ` DJ Delorie
  2021-01-29  2:38               ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: DJ Delorie @ 2021-01-29  2:09 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss

"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
> So, the definition is like this:
>
> #if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \
>     && defined(__CET__)
> # include <cet.h>
> # define _CET_NOTRACK notrack
> #else
> # define _CET_ENDBR
> # define _CET_NOTRACK
> #endif
>
> Where we include cet.h, CET_ENDBR may still be defined either
> as endbr64 or empty depending on __CET__ & 1 being non-zero or
> zero, right?
>
> So, would something like this work?

I suspect you only need one trampoline template, with __CET_ENDBR in it.  Use
.align instead of hardcoding the right number of NOPs, and other logic
to figure out the repeat counts.  It's safe to execute endbr64 when CET
is not enabled.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-29  2:09             ` DJ Delorie
@ 2021-01-29  2:38               ` Madhavan T. Venkataraman
  2021-01-29  2:48                 ` DJ Delorie
  2021-02-01 19:46                 ` DJ Delorie
  0 siblings, 2 replies; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-29  2:38 UTC (permalink / raw)
  To: DJ Delorie; +Cc: libffi-discuss



On 1/28/21 8:09 PM, DJ Delorie wrote:
> "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
>> So, the definition is like this:
>>
>> #if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \
>>     && defined(__CET__)
>> # include <cet.h>
>> # define _CET_NOTRACK notrack
>> #else
>> # define _CET_ENDBR
>> # define _CET_NOTRACK
>> #endif
>>
>> Where we include cet.h, CET_ENDBR may still be defined either
>> as endbr64 or empty depending on __CET__ & 1 being non-zero or
>> zero, right?
>>
>> So, would something like this work?
> 
> I suspect you only need one trampoline template, with __CET_ENDBR in it.  Use
> .align instead of hardcoding the right number of NOPs, and other logic
> to figure out the repeat counts.  It's safe to execute endbr64 when CET
> is not enabled.
> 

Yes. But the problem I ran into is that if libffi is compiled with old
compilers that have not yet been updated for Intel CET, the builds fail
because the assembler cannot recognize endbr64 and endbr32.

This happened in CI testing. In the previous version, I had the endbr64 in the code
like you suggested.

As for the repeat count, here is the reason.

I need to pack as many trampolines as possible in a page to conserve memory.
I can do this in one of three ways:

1. Map an anon page and copy the template multiple times into the page creating
   a code table.

   This is only possible if the security settings allow anon memory to be
   mapped with exec permissions.

2. Create a file and write the template multiple times into the file and map
   the file with executable permissions.

   Again, security settings must allow it. They do today. But allowing temp files
   to be mapped with exec permissions presents a security problem. That will
   be addressed in Linux. Microsoft has submitted a Linux Security Module
   implementation which will only allow a file to be mapped with exec
   permissions if the file is a signed file and the sign can be verified.
   Temp files will not have that.

   In our case, we are only writing a read-only buffer in the text segment into
   the temp file. So, it does not really pose a security problem in the sense
   that a hacker cannot modify anything. But the kernel does not know that.
   It may blindly prevent the mapping of a temp file with exec permissions
   because of its inability to validate the contents.

   Because of this reason, I use the temp file approach as a fallback method
   if the 3rd method below fails.
   
3. Create a code table like I have done and map it directly from the text
   segment. With the code table, the .align will not work because of the
   repeat of the code sequence. Hence, the nops.

I know it is not pretty. But this is my justification for that.

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-29  2:38               ` Madhavan T. Venkataraman
@ 2021-01-29  2:48                 ` DJ Delorie
  2021-01-29  3:24                   ` Madhavan T. Venkataraman
  2021-02-01 19:46                 ` DJ Delorie
  1 sibling, 1 reply; 30+ messages in thread
From: DJ Delorie @ 2021-01-29  2:48 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss


Sure, you still need to use the __CET_ENDBR macro in your trampoline.
And it might be tricky to compute the number of iterations per page (I
understand about the packing ;)

I'm just saying you don't need both CET and non-CET trampolines.

A CET-enabled trampoline should work in a CET-disabled system.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-29  2:48                 ` DJ Delorie
@ 2021-01-29  3:24                   ` Madhavan T. Venkataraman
  2021-01-29  6:07                     ` DJ Delorie
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-01-29  3:24 UTC (permalink / raw)
  To: DJ Delorie; +Cc: libffi-discuss



On 1/28/21 8:48 PM, DJ Delorie wrote:
> 
> Sure, you still need to use the __CET_ENDBR macro in your trampoline.
> And it might be tricky to compute the number of iterations per page (I
> understand about the packing ;)
> 
> I'm just saying you don't need both CET and non-CET trampolines.
> 
> A CET-enabled trampoline should work in a CET-disabled system.
> 

You are right. I only needed two versions because of the nops for aligment.

However, I think I can address your concern. Now that we are able to
define ENDBR_PRESENT, we can define the trampoline size in the header like
this

#ifdef ENDBR_PRESENT
#define	UNIX64_TRAMP_SIZE	40
#else
#define UNIX64_TRAMP_SIZE	32
#endif

Then, in the trampoline code table:

C(trampoline_code_table):
        .rept   UNIX64_TRAMP_MAP_SIZE / UNIX64_TRAMP_SIZE
        _CET_ENDBR
        subq    $16, %rsp               /* Make space on the stack */
        movq    %r10, (%rsp)            /* Save %r10 on stack */
        movq    4077(%rip), %r10        /* Copy data into %r10 */
        movq    %r10, 8(%rsp)           /* Save data on stack */
        movq    4073(%rip), %r10        /* Copy code into %r10 */
        jmp     *%r10                   /* Jump to code */
	nop
	nop
#ifdef ENDBR_PRESENT
        nop
        nop
        nop
        nop
#endif
        .endr
ENDF(C(trampoline_code_table))


This is cool. We only need one table.

Thanks.

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-29  3:24                   ` Madhavan T. Venkataraman
@ 2021-01-29  6:07                     ` DJ Delorie
  0 siblings, 0 replies; 30+ messages in thread
From: DJ Delorie @ 2021-01-29  6:07 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss

"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
> 	nop
> 	nop
> #ifdef ENDBR_PRESENT
>         nop
>         nop
>         nop
>         nop
> #endif

You can just use an .align directive here


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines
  2021-01-29  2:38               ` Madhavan T. Venkataraman
  2021-01-29  2:48                 ` DJ Delorie
@ 2021-02-01 19:46                 ` DJ Delorie
  1 sibling, 0 replies; 30+ messages in thread
From: DJ Delorie @ 2021-02-01 19:46 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss

"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
> Yes. But the problem I ran into is that if libffi is compiled with old
> compilers that have not yet been updated for Intel CET, the builds fail
> because the assembler cannot recognize endbr64 and endbr32.
>
> This happened in CI testing. In the previous version, I had the endbr64 in the code
> like you suggested.

I suggested keeping the __CET_ENDBR macro in, not the endbr64 code itself.

> As for the repeat count, here is the reason.

Yes, I know why we're *using* repeat.  I'm suggesting some clever math
to *calculate* the repeat count, instead of hard-coding it.  That way
the math gives you the most copies per page regardless of whether
__CET_ENDBR is defined or empty.

Of course, you could just use the largest repeate count you'll need, and
ignore any parts of the template that fall past the end of the page.
But you still need to know how many trampolines are usable in that page.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-01-28 17:01             ` Madhavan T. Venkataraman
@ 2021-02-05 18:20               ` Madhavan T. Venkataraman
  2021-02-05 18:46                 ` Anthony Green
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-02-05 18:20 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, fweimer, DJ Delorie

Hi Anthony,

>> In this case, my ask is that you split this into two patches.  The first one, (a), I would like to merge as soon as it seems ready.  
>>
> 
> OK. I will remove the public API part, sync with the latest libffi and submit a PR.
> 

I have removed the public API. I have pushed the latest changes in static_tramp_v4.
I have addressed all the comments from DJ Delorie (Thanks DJ!!!).

The current patchset contains support for X64, i386, arm64 and arm for Linux.
I will add BSD support in a separate patch once this gets accepted.

>>
>> Also, please use the github PR process for the next round so we can get some automated CI going.  And I prefer reviewing patches within the github UI, tbh.

I have created the PR. Please review.

Do I also need to send the patchset to the libffi alias? Please advise.

Thanks.

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-02-05 18:20               ` Madhavan T. Venkataraman
@ 2021-02-05 18:46                 ` Anthony Green
  2021-02-05 19:38                   ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: Anthony Green @ 2021-02-05 18:46 UTC (permalink / raw)
  To: Madhavan T. Venkataraman; +Cc: libffi-discuss, fweimer, DJ Delorie

On Fri, Feb 5, 2021 at 1:20 PM Madhavan T. Venkataraman <
madvenka@linux.microsoft.com> wrote:

> I have created the PR. Please review.
>

Thank you!


> Do I also need to send the patchset to the libffi alias? Please advise.
>

No need.

I appreciate your efforts!

AG

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-02-05 18:46                 ` Anthony Green
@ 2021-02-05 19:38                   ` Madhavan T. Venkataraman
  2021-02-07 16:09                     ` Madhavan T. Venkataraman
  0 siblings, 1 reply; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-02-05 19:38 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, fweimer, DJ Delorie



On 2/5/21 12:46 PM, Anthony Green wrote:
> On Fri, Feb 5, 2021 at 1:20 PM Madhavan T. Venkataraman <madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com>> wrote:
> 
>     I have created the PR. Please review.
> 
> 
> Thank you!
>  
> 
>     Do I also need to send the patchset to the libffi alias? Please advise.
> 
> 
> No need.  
> 
> I appreciate your efforts!
> 
> AG
> 

The Appveyor CI tests are showing green. But the test logs still show a build
failure somewhere. Let me fix that if I can. If you want to hold off on the review till
then, that is fine.

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v3 0/5] Libffi Static Trampolines
  2021-02-05 19:38                   ` Madhavan T. Venkataraman
@ 2021-02-07 16:09                     ` Madhavan T. Venkataraman
  0 siblings, 0 replies; 30+ messages in thread
From: Madhavan T. Venkataraman @ 2021-02-07 16:09 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, fweimer, DJ Delorie



On 2/5/21 1:38 PM, Madhavan T. Venkataraman wrote:
> 
> 
> On 2/5/21 12:46 PM, Anthony Green wrote:
>> On Fri, Feb 5, 2021 at 1:20 PM Madhavan T. Venkataraman <madvenka@linux.microsoft.com <mailto:madvenka@linux.microsoft.com>> wrote:
>>
>>     I have created the PR. Please review.
>>
>>
>> Thank you!
>>  
>>
>>     Do I also need to send the patchset to the libffi alias? Please advise.
>>
>>
>> No need.  
>>
>> I appreciate your efforts!
>>
>> AG
>>
> 
> The Appveyor CI tests are showing green. But the test logs still show a build
> failure somewhere. Let me fix that if I can. If you want to hold off on the review till
> then, that is fine.
> 
> Madhavan
> 

I have submitted static_tramp_v5. Here is the PR:

https://github.com/libffi/libffi/pull/624

All CI checks pass.

Please review.

Thanks!

Madhavan

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-02-07 16:09 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1ef5c7e1c9a6ebb140a476ba555ec955681f4fba>
2021-01-15 18:46 ` [RFC PATCH v3 0/5] Libffi Static Trampolines madvenka
2021-01-15 18:46   ` [RFC PATCH v3 1/5] " madvenka
2021-01-27  3:31     ` DJ Delorie
2021-01-27 21:51       ` Madhavan T. Venkataraman
2021-01-27 22:15         ` DJ Delorie
2021-01-27 22:43           ` Madhavan T. Venkataraman
2021-01-15 18:46   ` [RFC PATCH v3 2/5] x86: Support for " madvenka
2021-01-27  3:31     ` DJ Delorie
2021-01-28 21:59       ` Madhavan T. Venkataraman
2021-01-28 22:17         ` DJ Delorie
2021-01-28 23:25           ` Madhavan T. Venkataraman
2021-01-29  2:09             ` DJ Delorie
2021-01-29  2:38               ` Madhavan T. Venkataraman
2021-01-29  2:48                 ` DJ Delorie
2021-01-29  3:24                   ` Madhavan T. Venkataraman
2021-01-29  6:07                     ` DJ Delorie
2021-02-01 19:46                 ` DJ Delorie
2021-01-15 18:46   ` [RFC PATCH v3 3/5] i386: " madvenka
2021-01-15 18:46   ` [RFC PATCH v3 4/5] arm64: " madvenka
2021-01-15 18:46   ` [RFC PATCH v3 5/5] arm: " madvenka
2021-01-26 23:41   ` [RFC PATCH v3 0/5] Libffi " Anthony Green
2021-01-27 17:20     ` Madhavan T. Venkataraman
2021-01-27 18:00       ` Anthony Green
2021-01-27 19:45         ` Madhavan T. Venkataraman
2021-01-28 14:21           ` Anthony Green
2021-01-28 17:01             ` Madhavan T. Venkataraman
2021-02-05 18:20               ` Madhavan T. Venkataraman
2021-02-05 18:46                 ` Anthony Green
2021-02-05 19:38                   ` Madhavan T. Venkataraman
2021-02-07 16:09                     ` Madhavan T. Venkataraman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).