[PATCH v2] x86: Add tls check in gas
Cui, Lili
lili.cui@intel.com
Tue Sep 10 07:12:15 GMT 2024
> On 05.09.2024 10:57, Cui, Lili wrote:
> > @@ -6318,6 +6338,284 @@ static INLINE bool may_need_pass2 (const
> insn_template *t)
> > && (t->base_opcode | 8) == 0x2c); }
> >
> > +static enum x86_tls_error_type
> > +x86_check_tls_relocation (unsigned int r_type) {
> > + switch (r_type)
> > + {
> > + case BFD_RELOC_386_TLS_GOTDESC:
> > + /* Check transition from GDesc access model:
> > +
> > + leal x@tlsdesc(%ebx), %reg --> Memory reg must be %ebx and
> > + SIB is not supported.
> > + */
>
> What does "check transition" here mean? Aiui it's the linker doing the
> transitions (if/when possible).
>
Removed "transition from".
> > + if (i.tm.mnem_off != MN_lea)
> > + return x86_tls_error_insn;
> > + else if (i.index_reg)
> > + return x86_tls_error_sib;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_type.bitfield.instance != RegB)
> > + return x86_tls_error_RegB;
>
> As mentioned on other occasions before: While I understand this is to a fair
> degree a matter of style and hence personal preference, I view use of "else if"
> in such situations as misleading. Ommiting the "else" also leads to shorter
> code, i.e. (slightly) improved readability. (Note how further down you
> sometimes omit the "else". Such inconsistency is imo even worse than having
> the "else" everywhere.)
>
Jan, do you mean to use "if " instead "else if"?
> As to in particular the last check: 16-bit addressing using %bx is then also
> okay?
>
I removed the register size check by mistake, reloc() only checks the memory registers, I will add the register part check in this function.
> > + break;
> > +
> > + case BFD_RELOC_386_TLS_GD:
> > + /* Check transition from GD access model:
> > +
> > + leal foo@tlsgd(,%ebx,1), %eax --> Only this fixed format is
> supported.
> > + leal foo@tlsgd(%reg), %eax --> Dest reg must be '%eax'
> > + Memory reg can't be %eax.
> > + */
> > + if (i.tm.mnem_off != MN_lea)
> > + return x86_tls_error_insn;
> > + else if (i.index_reg)
> > + {
> > + if (i.base_reg)
> > + return x86_tls_error_base_reg_name;
> > + if (i.index_reg->reg_type.bitfield.instance != RegB)
> > + return x86_tls_error_index_RegB;
> > + else if (i.types[1].bitfield.instance != Accum)
> > + return x86_tls_error_dest_RegA;
> > + else if (i.log2_scale_factor)
> > + return x86_tls_error_scale_factor;
> > + }
> > + else
> > + {
> > + if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + if (i.base_reg->reg_type.bitfield.instance == Accum)
> > + return x86_tls_error_RegA;
> > + else if (i.types[1].bitfield.instance != Accum)
> > + return x86_tls_error_dest_RegA;
>
> Using i.types[] here looks fragile to me. Please consider using i.op[1].regs
> instead.
>
Ok.
>
> > + }
> > + break;
> > +
> > + case BFD_RELOC_386_TLS_LDM:
> > + /* Check transition from LDM access model:
> > +
> > + leal foo@tlsldm(%reg), %eax --> Dest reg must be '%eax'
> > + Memory reg can't be %eax and SIB
> > + is not supported.
> > + */
> > + if (i.tm.mnem_off != MN_lea)
> > + return x86_tls_error_insn;
> > + else if (i.index_reg)
> > + return x86_tls_error_sib;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_type.bitfield.instance == Accum)
> > + return x86_tls_error_RegA;
> > + else if (i.types[1].bitfield.instance != Accum)
> > + return x86_tls_error_dest_RegA;
> > + break;
> > +
> > + case BFD_RELOC_X86_64_GOTPC32_TLSDESC:
> > + /* Check transition from GOTPC32 TLSDESC access model:
> > +
> > + --- LP64 mode ---
> > + leaq x@tlsdesc(%rip), %rax --> Memory reg must be %rip.
> > +
> > + --- X32 mode ---
> > + rex leal x@tlsdesc(%rip), %eax --> Memory reg must be %rip.
> > +
> > + */
> > + if (i.tm.mnem_off != MN_lea)
> > + return x86_tls_error_insn;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_num != RegIP)
> > + return x86_tls_error_RegIP;
> > + break;
>
> And no check for the required REX prefix in x32 mode? Or is it gas that
> automatically adds it (could do with a few words in the comment then)?
>
For Egpr gas will automatically add rex2 for it.
For Gpr we need check REX prefix here.
Added REX check for Gpr and notes.
> > + case BFD_RELOC_X86_64_TLSGD:
> > + /* Check transition from GD access model:
> > +
> > + leaq foo@tlsgd(%rip), %rdi --> Only this fixed format is supported.
> > + */
> > + case BFD_RELOC_X86_64_TLSLD:
> > + /* Check transition from LD access model:
> > +
> > + leaq foo@tlsld(%rip), %rdi --> Only this fixed format is supported.
> > + */
> > + if (i.tm.mnem_off != MN_lea)
> > + return x86_tls_error_insn;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_num != RegIP)
> > + return x86_tls_error_RegIP;
> > + else if (i.types[1].bitfield.instance != RegDI)
> > + return x86_tls_error_dest_RegDI;
>
> For this you don't need to invent RegDI. You can check the register's number
> and flags.
>
Done.
> > + break;
> > +
> > + case BFD_RELOC_386_TLS_GOTIE:
> > + /* Check transition from GOTIE access model:
> > +
> > + subl foo@gotntpoff(%reg1), %reg2
> > + movl foo@gotntpoff(%reg1), %reg2
> > + addl foo@gotntpoff(%reg1), %reg2
> > +
> > + Memory operand: SIB is not supported.
> > + */
> > + case BFD_RELOC_386_TLS_IE_32:
> > + /* Check transition from IE_32 access model:
> > +
> > + subl foo@gottpoff(%reg1), %reg2
> > + movl foo@gottpoff(%reg1), %reg2
> > + addl foo@gottpoff(%reg1), %reg2
> > +
> > + Memory operand: SIB is not supported.
> > + */
> > + if (i.tm.mnem_off != MN_sub
> > + && i.tm.mnem_off != MN_add
> > + && i.tm.mnem_off != MN_mov)
> > + return x86_tls_error_insn;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.index_reg)
> > + return x86_tls_error_sib;
> > + break;
>
> And all MOV/ADD/SUB forms are permitted here? Plus what about operand
> size (perhaps also already further up)?
>
Added a dest register size check here, and reloc() will check memory register.
> > + case BFD_RELOC_386_TLS_IE:
> > + /* Check transition from IE access model:
> > +
> > + movl foo@indntpoff, %reg --> Mod == 00 && r/m == 5
> > + addl foo@indntpoff, %reg --> Mod == 00 && r/m == 5
> > + */
> > + if (i.tm.mnem_off != MN_add && i.tm.mnem_off != MN_mov)
> > + return x86_tls_error_insn;
> > + else if (i.base_reg || i.index_reg)
> > + return x86_tls_error_require_no_base_index_reg;
> > + break;
>
> Wouldn't this allow RegEIZ to pass, when that shouldn't pass?
>
Input:
addl foo@indntpoff(,%eiz,1), %eax ---> i.index_reg !=0
addl foo@indntpoff(%eiz), %eax
output:
Error: @INDNTPOFF operator should not have base/index register
Error: `foo@indntpoff(%eiz)' is not a valid base/index expression
Added dest register size check here.
> > + case BFD_RELOC_X86_64_GOTTPOFF:
> > + /* Check transition from GOTTPOFF access model:
> > +
> > + mov foo@gottpoff(%rip), %reg --> Memory Reg must be %rip.
> > + add foo@gottpoff(%rip), %reg --> Memory Reg must be %rip.
> > + */
> > + if (i.tm.mnem_off != MN_add && i.tm.mnem_off != MN_mov)
> > + return x86_tls_error_insn;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_num != RegIP)
> > + return x86_tls_error_RegIP;
> > + break;
> > +
> > + case BFD_RELOC_386_TLS_DESC_CALL:
> > + /* Check transition from GDesc access model:
> > +
> > + call *x@tlscall(%eax) --> Memory reg must be %eax and
> > + SIB is not supported.
> > + */
> > + if (i.tm.mnem_off != MN_call)
> > + return x86_tls_error_insn;
> > + else if (i.index_reg)
> > + return x86_tls_error_sib;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_type.bitfield.instance != Accum)
> > + return x86_tls_error_RegA;
> > + break;
>
> While the commentary for this is different from that for ...
>
> > + case BFD_RELOC_X86_64_TLSDESC_CALL:
> > + /* Check transition from GDesc access model:
> > +
> > + call *x@tlscall(%rax) <--- LP64 mode.
> > + call *x@tlscall(%eax) <--- X32 mode.
> > +
> > + Only these fixed formats are supported.
> > + */
> > + if (i.tm.mnem_off != MN_call)
> > + return x86_tls_error_insn;
> > + else if (i.index_reg)
> > + return x86_tls_error_sib;
> > + else if (!i.base_reg)
> > + return x86_tls_error_no_base_reg;
> > + else if (i.base_reg->reg_type.bitfield.instance != Accum)
> > + return x86_tls_error_RegA;
> > + break;
>
> ... this, the code is all the same. Fold both?
>
Done.
>
> > + default:
> > + return x86_tls_error_none;
> > + }
> > +
> > + return x86_tls_error_none;
> > +}
> > +
> > +static void
> > +x86_report_tls_error (enum x86_tls_error_type tls_error, unsigned int
> > +r_type) {
> > + unsigned int k;
> > + for (k = 0; k < ARRAY_SIZE (gotrel); k++)
> > + {
> > + if (gotrel[k].rel[object_64bit] == r_type)
> > + break;
> > + }
> > +
> > + switch (tls_error)
> > + {
> > + case x86_tls_error_insn:
> > + as_bad (_("@%s operator cannot be used with `%s'"),
> > + gotrel[k].str, insn_name (&i.tm));
> > + return;
> > +
> > + case x86_tls_error_sib:
> > + as_bad (_("@%s operator do not support SIB"), gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_no_base_reg:
> > + as_bad (_("@%s operator must have base regster"),
> > + gotrel[k].str);
>
> Nit: "register"
>
Done.
> > + return;
> > +
> > + case x86_tls_error_require_no_base_index_reg:
> > + as_bad (_("@%s operator should not have base/index register"),
>
> Nit: Odd wording. The operator never has any register. Perhaps better "...
> requires no base/index register"?
>
Done.
> > + gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_base_reg_name:
> > + as_bad (_("@%s operator should not have base register"),
> gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_index_RegB:
> > + as_bad (_("@%s operator only supports ebx/rbx as index register"),
> > + gotrel[k].str);
>
> Please properly print register names: With % prefix when such is required,
> and only the one register size that's actually permitted.
>
Removed "rbx". Considering Intel format, I did not add % prefix here.
> > + return;
> > +
> > + case x86_tls_error_RegA:
> > + as_bad (_("@%s operator only supports eax/rax as base register"),
> > + gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_RegB:
> > + as_bad (_("@%s operator only supports ebx/rbx as base register"),
> > + gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_RegIP:
> > + as_bad (_("@%s operator only supports rip as base register"),
> > + gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_dest_RegA:
> > + as_bad (_("@%s operator only supports eax/rax as dest register"),
> > + gotrel[k].str);
> > + return;
> > +
> > + case x86_tls_error_dest_RegDI:
> > + as_bad (_("@%s operator only supports eax/rax as dest register"),
> > + gotrel[k].str);
>
> Surely not again eax/rax?
>
Done.
> > + return;
> > +
> > + case x86_tls_error_scale_factor:
> > + as_bad (_("@%s operator only supports scale factor of 00"),
> > + gotrel[k].str);
>
> The scale _factor_ can never be 0; you mean 1 here.
>
Done.
> > @@ -16786,6 +17072,14 @@ md_parse_option (int c, const char *arg)
> > optimize_for_space = 0;
> > }
> > break;
> > + case OPTION_MTLS_CHECK:
> > + if (strcasecmp (arg, "yes") == 0)
> > + tls_check = 1;
> > + else if (strcasecmp (arg, "no") == 0)
> > + tls_check = 0;
>
> Nit: The variable has type bool - please use true/false in such cases.
>
Done.
> > --- a/opcodes/i386-gen.c
> > +++ b/opcodes/i386-gen.c
> > @@ -526,6 +526,7 @@ static const struct {
> > INSTANCE (RegC),
> > INSTANCE (RegD),
> > INSTANCE (RegB),
> > + INSTANCE (RegDI),
> > };
>
> In addition to what H.J. said here and below: If this was really needed (and
> then actually used somewhere in the opcode table), it would surely need
> some explanation in the description. Plus ...
>
Removed.
> > --- a/opcodes/i386-opc.h
> > +++ b/opcodes/i386-opc.h
> > @@ -843,6 +843,7 @@ enum operand_instance
> > RegC, /* %cl / %cx / %ecx / %rcx, e.g. register to hold shift count */
> > RegD, /* %dl / %dx / %edx / %rdx, e.g. register to hold I/O port addr */
> > RegB, /* %bl / %bx / %ebx / %rbx */
> > + RegDI, /* %rdx */
> > };
>
> ... the comment would then look wrong here. Yet as already indicated by H.J.,
> this likely all wants dropping again anyway.
>
Removed.
Thanks,
Lili.
> Jan
More information about the Binutils
mailing list