diff mbox series

[v3,7/7] x86: move bad-use-of-TLS-reloc check

Message ID e8014ebd-d16b-17a7-9f34-3700fc164136@suse.com
State New, archived
Headers
Series x86: suffix handling changes |

Commit Message

Jan Beulich Oct. 5, 2022, 7:25 a.m. UTC 
  Having it in match_template() is unhelpful. Neither does looking for the
next template to possibly match make any sense in that case, nor is the
resulting diagnostic making clear what the problem is.

While moving the check, also generalize it to include all SIMD and VEX-
encoded insns. This way an existing conditional can be re-used in
md_assemble(). Note though that this still leaves a lof of insns which
are also wrong to use with these relocations.

Further fold the remaining check (BFD_RELOC_386_GOT32) with the XRELEASE
related one a few lines down. This again allows re-using an existing
conditional.
---
Is the set of relocations enumerated here actually complete?
---
v2: New.

Comments

H.J. Lu Oct. 11, 2022, 5:57 p.m. UTC | #1 
On Wed, Oct 5, 2022 at 12:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Having it in match_template() is unhelpful. Neither does looking for the
> next template to possibly match make any sense in that case, nor is the
> resulting diagnostic making clear what the problem is.
>
> While moving the check, also generalize it to include all SIMD and VEX-
> encoded insns. This way an existing conditional can be re-used in
> md_assemble(). Note though that this still leaves a lof of insns which
> are also wrong to use with these relocations.

These TLS instruction sequences are generated by compilers.
The check is only meant to check for invalid master registers.

> Further fold the remaining check (BFD_RELOC_386_GOT32) with the XRELEASE
> related one a few lines down. This again allows re-using an existing
> conditional.
> ---
> Is the set of relocations enumerated here actually complete?
> ---
> v2: New.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5119,14 +5119,30 @@ md_assemble (char *line)
>        return;
>      }
>
> -  /* Check for data size prefix on VEX/XOP/EVEX encoded and SIMD insns.  */
> -  if (i.prefix[DATA_PREFIX]
> -      && (is_any_vex_encoding (&i.tm)
> -         || i.tm.operand_types[i.imm_operands].bitfield.class >= RegMMX
> -         || i.tm.operand_types[i.imm_operands + 1].bitfield.class >= RegMMX))
> +  if (is_any_vex_encoding (&i.tm)
> +      || i.tm.operand_types[i.imm_operands].bitfield.class >= RegMMX
> +      || i.tm.operand_types[i.imm_operands + 1].bitfield.class >= RegMMX)
>      {
> -      as_bad (_("data size prefix invalid with `%s'"), i.tm.name);
> -      return;
> +      /* Check for data size prefix on VEX/XOP/EVEX encoded and SIMD insns.  */
> +      if (i.prefix[DATA_PREFIX])
> +       {
> +         as_bad (_("data size prefix invalid with `%s'"), i.tm.name);
> +         return;
> +       }
> +
> +      /* Don't allow e.g. KMOV in TLS code sequences.  */
> +      for (j = i.imm_operands; j < i.operands; ++j)
> +       switch (i.reloc[j])
> +         {
> +         case BFD_RELOC_386_TLS_GOTIE:
> +         case BFD_RELOC_386_TLS_LE_32:
> +         case BFD_RELOC_X86_64_GOTTPOFF:
> +         case BFD_RELOC_X86_64_TLSLD:
> +           as_bad (_("TLS relocation cannot be used with `%s'"), i.tm.name);
> +           return;
> +         default:
> +           break;
> +         }
>      }
>
>    /* Check if HLE prefix is OK.  */
> @@ -6767,26 +6783,6 @@ match_template (char mnem_suffix)
>             }
>         }
>
> -      switch (i.reloc[0])
> -       {
> -       case BFD_RELOC_386_GOT32:
> -         /* Force 0x8b encoding for "mov foo@GOT, %eax".  */
> -         if (t->base_opcode == 0xa0
> -             && t->opcode_modifier.opcodespace == SPACE_BASE)
> -           continue;
> -         break;
> -       case BFD_RELOC_386_TLS_GOTIE:
> -       case BFD_RELOC_386_TLS_LE_32:
> -       case BFD_RELOC_X86_64_GOTTPOFF:
> -       case BFD_RELOC_X86_64_TLSLD:
> -         /* Don't allow KMOV in TLS code sequences.  */
> -         if (t->opcode_modifier.vex)
> -           continue;
> -         break;
> -       default:
> -         break;
> -       }
> -
>        /* We check register size if needed.  */
>        if (t->opcode_modifier.checkregsize)
>         {
> @@ -6817,12 +6813,19 @@ match_template (char mnem_suffix)
>               && i.types[1].bitfield.instance == Accum
>               && i.types[1].bitfield.dword)
>             continue;
> -         /* xrelease mov %eax, <disp> is another special case. It must not
> -            match the accumulator-only encoding of mov.  */
> -         if (i.hle_prefix
> -             && t->base_opcode == 0xa0
> +
> +         if (t->base_opcode == MOV_AX_DISP32
>               && t->opcode_modifier.opcodespace == SPACE_BASE)
> -           continue;
> +           {
> +             /* Force 0x8b encoding for "mov foo@GOT, %eax".  */
> +             if (i.reloc[0] == BFD_RELOC_386_GOT32)
> +               continue;
> +
> +             /* xrelease mov %eax, <disp> is another special case. It must not
> +                match the accumulator-only encoding of mov.  */
> +             if (i.hle_prefix)
> +               continue;
> +           }
>           /* Fall through.  */
>
>         case 3:
>

OK.

Thanks.
Jan Beulich Oct. 12, 2022, 7:13 a.m. UTC | #2 
On 11.10.2022 19:57, H.J. Lu wrote:
> On Wed, Oct 5, 2022 at 12:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Having it in match_template() is unhelpful. Neither does looking for the
>> next template to possibly match make any sense in that case, nor is the
>> resulting diagnostic making clear what the problem is.
>>
>> While moving the check, also generalize it to include all SIMD and VEX-
>> encoded insns. This way an existing conditional can be re-used in
>> md_assemble(). Note though that this still leaves a lof of insns which
>> are also wrong to use with these relocations.
> 
> These TLS instruction sequences are generated by compilers.
> The check is only meant to check for invalid master registers.

The sequence involving a mask register was generated (aiui) by mistake.
Why would it not be possible to see another similar mistake involve an
XMM register, with a legacy, VEX-, or EVEX-encoded insn?

Anyway - thanks for approving the patch.

Jan
H.J. Lu Oct. 12, 2022, 5:02 p.m. UTC | #3 
On Wed, Oct 12, 2022 at 12:13 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 11.10.2022 19:57, H.J. Lu wrote:
> > On Wed, Oct 5, 2022 at 12:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Having it in match_template() is unhelpful. Neither does looking for the
> >> next template to possibly match make any sense in that case, nor is the
> >> resulting diagnostic making clear what the problem is.
> >>
> >> While moving the check, also generalize it to include all SIMD and VEX-
> >> encoded insns. This way an existing conditional can be re-used in
> >> md_assemble(). Note though that this still leaves a lof of insns which
> >> are also wrong to use with these relocations.
> >
> > These TLS instruction sequences are generated by compilers.
> > The check is only meant to check for invalid master registers.
>
> The sequence involving a mask register was generated (aiui) by mistake.
> Why would it not be possible to see another similar mistake involve an
> XMM register, with a legacy, VEX-, or EVEX-encoded insn?

We haven't seen it yet.

> Anyway - thanks for approving the patch.
>
> Jan
diff mbox series

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5119,14 +5119,30 @@  md_assemble (char *line)
       return;
     }
 
-  /* Check for data size prefix on VEX/XOP/EVEX encoded and SIMD insns.  */
-  if (i.prefix[DATA_PREFIX]
-      && (is_any_vex_encoding (&i.tm)
-	  || i.tm.operand_types[i.imm_operands].bitfield.class >= RegMMX
-	  || i.tm.operand_types[i.imm_operands + 1].bitfield.class >= RegMMX))
+  if (is_any_vex_encoding (&i.tm)
+      || i.tm.operand_types[i.imm_operands].bitfield.class >= RegMMX
+      || i.tm.operand_types[i.imm_operands + 1].bitfield.class >= RegMMX)
     {
-      as_bad (_("data size prefix invalid with `%s'"), i.tm.name);
-      return;
+      /* Check for data size prefix on VEX/XOP/EVEX encoded and SIMD insns.  */
+      if (i.prefix[DATA_PREFIX])
+	{
+	  as_bad (_("data size prefix invalid with `%s'"), i.tm.name);
+	  return;
+	}
+
+      /* Don't allow e.g. KMOV in TLS code sequences.  */
+      for (j = i.imm_operands; j < i.operands; ++j)
+	switch (i.reloc[j])
+	  {
+	  case BFD_RELOC_386_TLS_GOTIE:
+	  case BFD_RELOC_386_TLS_LE_32:
+	  case BFD_RELOC_X86_64_GOTTPOFF:
+	  case BFD_RELOC_X86_64_TLSLD:
+	    as_bad (_("TLS relocation cannot be used with `%s'"), i.tm.name);
+	    return;
+	  default:
+	    break;
+	  }
     }
 
   /* Check if HLE prefix is OK.  */
@@ -6767,26 +6783,6 @@  match_template (char mnem_suffix)
 	    }
 	}
 
-      switch (i.reloc[0])
-	{
-	case BFD_RELOC_386_GOT32:
-	  /* Force 0x8b encoding for "mov foo@GOT, %eax".  */
-	  if (t->base_opcode == 0xa0
-	      && t->opcode_modifier.opcodespace == SPACE_BASE)
-	    continue;
-	  break;
-	case BFD_RELOC_386_TLS_GOTIE:
-	case BFD_RELOC_386_TLS_LE_32:
-	case BFD_RELOC_X86_64_GOTTPOFF:
-	case BFD_RELOC_X86_64_TLSLD:
-	  /* Don't allow KMOV in TLS code sequences.  */
-	  if (t->opcode_modifier.vex)
-	    continue;
-	  break;
-	default:
-	  break;
-	}
-
       /* We check register size if needed.  */
       if (t->opcode_modifier.checkregsize)
 	{
@@ -6817,12 +6813,19 @@  match_template (char mnem_suffix)
 	      && i.types[1].bitfield.instance == Accum
 	      && i.types[1].bitfield.dword)
 	    continue;
-	  /* xrelease mov %eax, <disp> is another special case. It must not
-	     match the accumulator-only encoding of mov.  */
-	  if (i.hle_prefix
-	      && t->base_opcode == 0xa0
+
+	  if (t->base_opcode == MOV_AX_DISP32
 	      && t->opcode_modifier.opcodespace == SPACE_BASE)
-	    continue;
+	    {
+	      /* Force 0x8b encoding for "mov foo@GOT, %eax".  */
+	      if (i.reloc[0] == BFD_RELOC_386_GOT32)
+		continue;
+
+	      /* xrelease mov %eax, <disp> is another special case. It must not
+		 match the accumulator-only encoding of mov.  */
+	      if (i.hle_prefix)
+		continue;
+	    }
 	  /* Fall through.  */
 
 	case 3: