[inline assembly compliance] Issues and patches

* [inline assembly compliance] Issues and patches
@ 2020-04-07  8:45 FRÉDÉRIC RECOULES
  2020-04-22 15:19 ` FRÉDÉRIC RECOULES
  0 siblings, 1 reply; 3+ messages in thread
From: FRÉDÉRIC RECOULES @ 2020-04-07  8:45 UTC (permalink / raw)
  To: alsa-devel; +Cc: Richard Bonichon, Sébastien Bardin

Dear developpers,



we are academic researchers working in automated program analysis.
We are currently interested in checking compliance of inline asm chunks
as found in C programs.

While benchmarking our tool and technique, we found 5 significant issues in
ALSA. We report them to you, as well as adequate patches with explanations.

* All these bugs are related to compliance between the block of asm and its
surrounding "contract" (in gcc-style notation). They are akin to undefined or
implementation-defined behaviours  in C: they currently do not manifest
themselves in your program, but at some point in time with compiler
optimizations becoming more and more aggressive or changes in undocumented
compiler choices regarding asm chunks, they can suddenly trigger a
(hard-to-find) bug.

* The typical problems come from the compiler missing dataflow information
and performing undue optimizations on this wrong basis, or the compiler
allocating an already used register. Actually, we demonstrate "in lab" problems
with all these categories of bugs in case of inlining
(especially with LTO enabler) or code refactoring.

* Some of those issues may seems benign or irrealistic but it cost nothing
to patch so, why not do it?



We would be very interested to hear your opinion on these matters.
Are you interested in such errors and patches?
Also, besides the patches, we are currently working on a code analyzer
prototype designed to check asm compliance and to propose patches when the
chunk is not compliant. This is still work in progress and we are finalizing it.
The errors and patches I reported to you came from my prototype.
In case such a prototype would be made available, would you consider using it?



Best regards

Frédéric Recoules


-----------------------------------Details------------------------------------------


The issues are the sames for the five macro-functions defined in
pcm/pcm_dmix_i386.h.

1. "memory" clobber is missing
------------------------------

The memory is read from src and written from dst and sum.
For instance, the compiler may believe that the memory pointed by src has
never been read and decide to get rid of its initialization
(dead store elimination).

Patched by adding "memory" to the clobbers

2. the read-only input size is clobbered
----------------------------------------

The compiler may assume the old value of size is still present at the
location and may misuse the new value (i.e. 0).

Patched by moving size from read-only input to read-write output.

3. ebx is not declared in clobbers
----------------------------------

We know that prior to the version 5.0, GCC refused to give ebx as clobber.
It is no longer true and may become risky as it can be used to compute the
address of one of the others memory inputs.

Patched by creating a switch to add ebx in clobber
(and removing the manual save/restore) if GCC is newer than 5.0 or other
compiler is used.


The following issue concern the MIX_AREAS_16_MMX macro-function.

4. mm0 is clobbered, mm1 is used uninitialized
----------------------------------------------

Patched by adding mm0 and mm1 to clobbers and adding a pxor instruction for mm1.


Then here are some minor improvements:
- add "cc" to the clobbers even if it is, for now, redundant;
- allow to use a register (i.e. ebp) for size
- allow to pass dst_step, src_step and sum_step through immediates


-----------------------------------Patches------------------------------------------

diff --git a/src/pcm/pcm_dmix_i386.h b/src/pcm/pcm_dmix_i386.h
index 2778cb1d..af2f4630 100644
--- a/src/pcm/pcm_dmix_i386.h
+++ b/src/pcm/pcm_dmix_i386.h
@@ -26,6 +26,13 @@
  *
  */
 
+#define COMMA ,
+#if __GNUC__ < 5 && defined(__PIC__)
+#  define GCC_PIC_SWITCH(before,after) before
+#else
+#  define GCC_PIC_SWITCH(before,after) after
+#endif
+
 /*
  *  for plain i386
  */
@@ -47,13 +54,14 @@ static void MIX_AREAS_16(unsigned int size,
 	__asm__ __volatile__ (
 		"\n"
 
-		"\tmovl %%ebx, %7\n"	/* ebx is GOT pointer (-fPIC) */
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
 		/*
 		 *  initialization, load ESI, EDI, EBX registers
 		 */
-		"\tmovl %1, %%edi\n"
-		"\tmovl %2, %%esi\n"
-		"\tmovl %3, %%ebx\n"
+		"\tmovl %2, %%edi\n"
+		"\tmovl %3, %%esi\n"
+		"\tmovl %4, %%ebx\n"
 		"\tcmpl $0, %0\n"
 		"\tjnz 2f\n"
 		"\tjmp 7f\n"
@@ -64,9 +72,9 @@ static void MIX_AREAS_16(unsigned int size,
 		 */
 		"\t.p2align 4,,15\n"
 		"1:"
-		"\tadd %4, %%edi\n"
-		"\tadd %5, %%esi\n"
-		"\tadd %6, %%ebx\n"
+		"\tadd %5, %%edi\n"
+		"\tadd %6, %%esi\n"
+		"\tadd %7, %%ebx\n"
 
 		/*
 		 *   sample = *src;
@@ -138,15 +146,16 @@ static void MIX_AREAS_16(unsigned int size,
 		"\tjnz 4b\n"
 		"\tdecl %0\n"
 		"\tjnz 1b\n"
-		
-		"7:"
-		"\tmovl %7, %%ebx\n"	/* ebx is GOT pointer (-fPIC) */
 
-		: /* no output regs */
-		: "m" (size), "m" (dst), "m" (src),
-		  "m" (sum), "m" (dst_step), "m" (src_step),
-		  "m" (sum_step), "m" (old_ebx)
-		: "esi", "edi", "edx", "ecx", "eax"
+		"7:"
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
+
+		: "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
+		: "m" (dst), "m" (src), "m" (sum),
+		  "im" (dst_step), "im" (src_step), "im" (sum_step)
+		: "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
+		  "memory", "cc"
 	);
 }
 
@@ -171,22 +180,24 @@ static void MIX_AREAS_16_MMX(unsigned int size,
 	__asm__ __volatile__ (
 		"\n"
 
-		"\tmovl %%ebx, %7\n"	/* ebx is GOT pointer (-fPIC) */
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
 		/*
-		 *  initialization, load ESI, EDI, EBX registers
+		 *  initialization, load ESI, EDI, EBX registers, clear MM1
 		 */
-		"\tmovl %1, %%edi\n"
-		"\tmovl %2, %%esi\n"
-		"\tmovl %3, %%ebx\n"
+		"\tpxor %%mm1, %%mm1\n"
+		"\tmovl %2, %%edi\n"
+		"\tmovl %3, %%esi\n"
+		"\tmovl %4, %%ebx\n"
 		"\tcmpl $0, %0\n"
 		"\tjnz 2f\n"
 		"\tjmp 5f\n"
 
 		"\t.p2align 4,,15\n"
 		"1:"
-		"\tadd %4, %%edi\n"
-		"\tadd %5, %%esi\n"
-		"\tadd %6, %%ebx\n"
+		"\tadd %5, %%edi\n"
+		"\tadd %6, %%esi\n"
+		"\tadd %7, %%ebx\n"
 
 		"2:"
 		/*
@@ -230,13 +241,14 @@ static void MIX_AREAS_16_MMX(unsigned int size,
 		"\tjnz 1b\n"
 		"\temms\n"
                 "5:"
-		"\tmovl %7, %%ebx\n"	/* ebx is GOT pointer (-fPIC) */
-
-		: /* no output regs */
-		: "m" (size), "m" (dst), "m" (src),
-		  "m" (sum), "m" (dst_step), "m" (src_step),
-		  "m" (sum_step), "m" (old_ebx)
-		: "esi", "edi", "edx", "ecx", "eax"
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
+
+		: "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
+		: "m" (dst), "m" (src), "m" (sum),
+		  "im" (dst_step), "im" (src_step), "im" (sum_step)
+		: "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
+		  "mm0", "mm1", "memory", "cc"
 	);
 }
 
@@ -261,13 +273,14 @@ static void MIX_AREAS_32(unsigned int size,
 	__asm__ __volatile__ (
 		"\n"
 
-		"\tmovl %%ebx, %7\n"	/* ebx is GOT pointer (-fPIC) */
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
 		/*
 		 *  initialization, load ESI, EDI, EBX registers
 		 */
-		"\tmovl %1, %%edi\n"
-		"\tmovl %2, %%esi\n"
-		"\tmovl %3, %%ebx\n"
+		"\tmovl %2, %%edi\n"
+		"\tmovl %3, %%esi\n"
+		"\tmovl %4, %%ebx\n"
 		"\tcmpl $0, %0\n"
 		"\tjnz 1f\n"
 		"\tjmp 6f\n"
@@ -337,19 +350,20 @@ static void MIX_AREAS_32(unsigned int size,
 		 */
 		"\tdecl %0\n"
 		"\tjz 6f\n"
-		"\tadd %4, %%edi\n"
-		"\tadd %5, %%esi\n"
-		"\tadd %6, %%ebx\n"
+		"\tadd %5, %%edi\n"
+		"\tadd %6, %%esi\n"
+		"\tadd %7, %%ebx\n"
 		"\tjmp 1b\n"
-		
-		"6:"
-		"\tmovl %7, %%ebx\n"	/* ebx is GOT pointer (-fPIC) */
 
-		: /* no output regs */
-		: "m" (size), "m" (dst), "m" (src),
-		  "m" (sum), "m" (dst_step), "m" (src_step),
-		  "m" (sum_step), "m" (old_ebx)
-		: "esi", "edi", "edx", "ecx", "eax"
+		"6:"
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
+
+		: "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
+		: "m" (dst), "m" (src), "m" (sum),
+		  "im" (dst_step), "im" (src_step), "im" (sum_step)
+		: "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
+		  "memory", "cc"
 	);
 }
 
@@ -374,13 +388,14 @@ static void MIX_AREAS_24(unsigned int size,
 	__asm__ __volatile__ (
 		"\n"
 
-		"\tmovl %%ebx, %7\n"	/* ebx is GOT pointer (-fPIC) */
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
 		/*
 		 *  initialization, load ESI, EDI, EBX registers
 		 */
-		"\tmovl %1, %%edi\n"
-		"\tmovl %2, %%esi\n"
-		"\tmovl %3, %%ebx\n"
+		"\tmovl %2, %%edi\n"
+		"\tmovl %3, %%esi\n"
+		"\tmovl %4, %%ebx\n"
 		"\tcmpl $0, %0\n"
 		"\tjnz 1f\n"
 		"\tjmp 6f\n"
@@ -443,19 +458,20 @@ static void MIX_AREAS_24(unsigned int size,
 		 */
 		"\tdecl %0\n"
 		"\tjz 6f\n"
-		"\tadd %4, %%edi\n"
-		"\tadd %5, %%esi\n"
-		"\tadd %6, %%ebx\n"
+		"\tadd %5, %%edi\n"
+		"\tadd %6, %%esi\n"
+		"\tadd %7, %%ebx\n"
 		"\tjmp 1b\n"
-		
-		"6:"
-		"\tmovl %7, %%ebx\n"	/* ebx is GOT pointer (-fPIC) */
 
-		: /* no output regs */
-		: "m" (size), "m" (dst), "m" (src),
-		  "m" (sum), "m" (dst_step), "m" (src_step),
-		  "m" (sum_step), "m" (old_ebx)
-		: "esi", "edi", "edx", "ecx", "eax"
+		"6:"
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
+
+		: "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
+		: "m" (dst), "m" (src), "m" (sum),
+		  "im" (dst_step), "im" (src_step), "im" (sum_step)
+		: "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
+		  "memory", "cc"
 	);
 }
 
@@ -480,13 +496,14 @@ static void MIX_AREAS_24_CMOV(unsigned int size,
 	__asm__ __volatile__ (
 		"\n"
 
-		"\tmovl %%ebx, %7\n"	/* ebx is GOT pointer (-fPIC) */
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
 		/*
 		 *  initialization, load ESI, EDI, EBX registers
 		 */
-		"\tmovl %1, %%edi\n"
-		"\tmovl %2, %%esi\n"
-		"\tmovl %3, %%ebx\n"
+		"\tmovl %2, %%edi\n"
+		"\tmovl %3, %%esi\n"
+		"\tmovl %4, %%ebx\n"
 		"\tcmpl $0, %0\n"
 		"\tjz 6f\n"
 
@@ -541,19 +558,20 @@ static void MIX_AREAS_24_CMOV(unsigned int size,
 		/*
 		 * while (size-- > 0)
 		 */
-		"\tadd %4, %%edi\n"
-		"\tadd %5, %%esi\n"
-		"\tadd %6, %%ebx\n"
+		"\tadd %5, %%edi\n"
+		"\tadd %6, %%esi\n"
+		"\tadd %7, %%ebx\n"
 		"\tdecl %0\n"
 		"\tjnz 1b\n"
-		
-		"6:"
-		"\tmovl %7, %%ebx\n"	/* ebx is GOT pointer (-fPIC) */
 
-		: /* no output regs */
-		: "m" (size), "m" (dst), "m" (src),
-		  "m" (sum), "m" (dst_step), "m" (src_step),
-		  "m" (sum_step), "m" (old_ebx)
-		: "esi", "edi", "edx", "ecx", "eax"
+		"6:"
+		/* ebx is GOT pointer (-fPIC) */
+		GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
+
+		: "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
+		: "m" (dst), "m" (src), "m" (sum),
+		  "im" (dst_step), "im" (src_step), "im" (sum_step)
+		: "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
+		  "memory", "cc"
 	);
 }

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