[inline assembly compliance] Issues and patches
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" ); }
Dear developpers,
We are looking forward to hear news about the patches. Did you have time to take a look?
We are trying to keep track of the state of the assembly in projects in which we found issues.
Please recall that they should be small, very local, easy to review changes and please contact us if you need more information about them.
Regards, Frédéric Recoules
De: "FRÉDÉRIC RECOULES" frederic.recoules@univ-grenoble-alpes.fr À: "alsa-devel" alsa-devel@alsa-project.org Cc: "Richard Bonichon" richard.bonichon@gmail.com, "Sébastien Bardin" sebastien.bardin@cea.fr Envoyé: Mardi 7 Avril 2020 10:45:08 Objet: [inline assembly compliance] Issues and patches
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" ); }
On Wed, 22 Apr 2020 17:19:27 +0200, FRÉDÉRIC RECOULES wrote:
Dear developpers,
We are looking forward to hear news about the patches. Did you have time to take a look?
We are trying to keep track of the state of the assembly in projects in which we found issues.
Please recall that they should be small, very local, easy to review changes and please contact us if you need more information about them.
The changes look good to me. I'll give some tests, but meanwhile, could you re-submit patches in a proper format that is applicable via git-am? Especially the good changelog text in the patch description is important and we want to have your Signed-off-by line like the kernel patch.
Thanks!
Takashi
Regards, Frédéric Recoules
De: "FRÉDÉRIC RECOULES" frederic.recoules@univ-grenoble-alpes.fr À: "alsa-devel" alsa-devel@alsa-project.org Cc: "Richard Bonichon" richard.bonichon@gmail.com, "Sébastien Bardin" sebastien.bardin@cea.fr Envoyé: Mardi 7 Avril 2020 10:45:08 Objet: [inline assembly compliance] Issues and patches
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.
- "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
- 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.
- 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.
- 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"
); }
participants (2)
-
FRÉDÉRIC RECOULES -
Takashi Iwai