[PATCH] x86: Add tls check in gas
H.J. Lu
hjl.tools@gmail.com
Fri Aug 30 14:34:18 GMT 2024
On Tue, Aug 27, 2024 at 08:59:32AM +0200, Jan Beulich wrote:
> On 27.08.2024 08:07, Cui, Lili wrote:
> > This patch is to add tls check in gas, and the following two changes are special:
> > 1. Removed BFD_RELOC_386_TLS_LE_32 kmov test, as the linker does not change it.
>
> Yet that, at some point, was specifically added (by or upon request from
> H.J.). This needs clarifying. The more that you remove at least one check
> in a testcase, which was deliberately put there iirc.
>
> Also aiui this isn't meant to be part of the patch description (i.e. what
> would eventually be committed), yet the reasoning here needs to be part
> of the commit.
>
> > 2. Added -mtls-check=no to gas reloc32/reloc64 test files as there are some illegal instructions. I'm not sure if we should remove them.
>
> This, otoh, is legitimately a remark which doesn't need committing. For
> reference (also for colleagues of yours), elsewhere the canonical way of
> submitting patches is: Patch description, optionally a --- separator
> followed by remarks (incl revision log; optionally intermediate ---
> separators), optionally another --- separators followed by the diffstat,
> actual patch hunks.
>
> By following a fixed scheme it becomes clear to all involved parties
> what parts of a submission are meant to be part of the eventual commit.
> And it also is more predictable for readers what to look for where.
>
> As to the remark itself: I think keeping those tests as they are, with
> just the option added, is the way to go.
>
> > Thanks,
> > Lili.
> >
> > Assembler shouldn't accept invalid TLS instructions, TLS relocations
> > can only be used with specific instructions as specified in TLS psABI
> > and linker issues an error when TLS relocations are used with wrong
> > instructions. It is inconvenient for gcc to rely on linker to report
> > errors, adding tls check in the assembler stage so that gcc can know
> > tls errors earlier.
> >
> > gas/ChangeLog:
> >[...]
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -38,6 +38,9 @@
> > #include "opcodes/i386-mnem.h"
> > #include <limits.h>
> >
> > +#define _BFD_MAKE_TABLE_bfd_reloc_code_real
> > +#include "bfd/libbfd.h"
>
> This looks like a hack, without any explanation in the description. What
> is this about? Having got to the bottom of the changes in the file, I
> guesst this is so you can use bfd_reloc_code_real_names[]. Yet I don't
> think that's how it's supposed to be done. But see a respective comment
> further down - the BFD names anyway aren't what should be used, imo.
>
> > @@ -717,6 +720,9 @@ lfence_before_ret;
> > static int generate_relax_relocations
> > = DEFAULT_GENERATE_X86_RELAX_RELOCATIONS;
> >
> > +/* 1 if the assembler should check tls relocation. */
> > +static int tls_check = DEFAULT_TLS_CHECK;
>
> bool please for booleans. Question though is whether we want this to be
> a tristate (off, warning, error).
>
> > @@ -6229,6 +6235,108 @@ static INLINE bool may_need_pass2 (const insn_template *t)
> > && (t->base_opcode | 8) == 0x2c);
> > }
> >
> > +static bool x86_check_tls_relocation (unsigned int r_type)
static bool
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), %eax
> > + call *x@tlsdesc(%eax)
> > + */
> > + if (i.tm.mnem_off == MN_call)
> > + return true;
>
> There are two forms of CALL - I doubt both are allowed.
>
> > + /* fall through */
> > + case BFD_RELOC_386_TLS_GD:
> > + /* Check transition from GD access model:
> > + leal foo@tlsgd(,%ebx,1), %eax
> > + */
> > + case BFD_RELOC_386_TLS_LDM:
> > + /* Check transition from LDM access model:
> > + leal foo@tlsldm(%ebx), %eax
> > + */
> > + if (i.tm.mnem_off == MN_lea)
> > + return true;
>
> And which base/index registers are used doesn't matter? Nor operand
> size?
We need to check base and index registers as well as operand size.
Only EBX is allowed. This applies to all relocations. Please check
elf32-i386.c and elf64-x86-64.c to see the exact requirements.
>
> > + break;
> > + case BFD_RELOC_X86_64_TLSGD:
> > + /* Check transition from GD access model:
> > + leaq foo@tlsgd(%rip), %rdi
> > + */
>
> Unlike above order is reversed here: Only LEA is mentioned, yet ...
>
> > + case BFD_RELOC_X86_64_GOTPC32_TLSDESC:
> > + /* Check transition from GOTPC32 TLSDESC access model:
> > +
> > + --- LP64 mode ---
> > + leaq x@tlsdesc(%rip), %rax
> > + call *x@tlsdesc(%rax)
> > +
> > + --- X32 mode ---
> > + rex leal x@tlsdesc(%rip), %eax
> > + call *x@tlsdesc(%eax)
> > + */
> > + if (i.tm.mnem_off == MN_call)
> > + return true;
>
> ... CALL is being permitted. In fact ...
>
> > + /* fall through */
> > + case BFD_RELOC_X86_64_TLSLD:
> > + /* Check transition from LD access model:
> > + leaq foo@tlsld(%rip), %rdi
> > + */
> > + if (i.tm.mnem_off == MN_lea
> > + && i.base_reg
> > + && i.base_reg->reg_num == RegIP)
> > + return true;
>
> ... this check looks to match what (as per the comment) X86_64_TLSGD
> also needs.
>
> > + 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
> > + */
> > + 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
> > + */
> > + if (i.tm.mnem_off == MN_sub)
> > + return true;
> > + /* fall through */
> > + case BFD_RELOC_386_TLS_IE:
> > + /* Check transition from IE access model:
> > + movl foo@indntpoff, %eax
> > + movl foo@indntpoff, %reg
> > + addl foo@indntpoff, %reg
> > + */
> > + if (i.tm.mnem_off == MN_add || i.tm.mnem_off == MN_mov)
> > + return true;
>
> Same question again here (and below): Are all insn forms permitted?
>
> > + break;
> > + case BFD_RELOC_X86_64_GOTTPOFF:
> > + /* Check transition from GOTTPOFF access model:
> > + mov foo@gottpoff(%rip), %reg
> > + add foo@gottpoff(%rip), %reg.
> > + */
>
> Here you even omit the insn suffixes, raising yet more questions.
>
> > + if ((i.tm.mnem_off == MN_add || i.tm.mnem_off == MN_mov)
> > + && i.base_reg
> > + && i.base_reg->reg_num == RegIP)
> > + return true;
> > + break;
> > + case BFD_RELOC_386_TLS_DESC_CALL:
> > + /* Check transition from GDesc access model:
> > + call *x@tlscall(%eax).
> > + */
> > + 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.
> > + */
> > + if (i.tm.mnem_off == MN_call)
> > + return true;
> > + break;
>
> Throughout I think avoiding if() and instead using return directly
> would make things quite a bit more clear. In particular one then won't
> need to go from early in the switch() statement to the bottom of the
> function to see what (really: no) further checks are done. I.e. for
> the case here simply:
>
> return i.tm.mnem_off == MN_call;
>
> (assuming of course - see comments further up - that's really the
> only thing which needs checking).
>
> For legibility may I further ask that non-fall-through case blocks be
> separated by a blank line?
>
> > @@ -6567,6 +6675,18 @@ i386_assemble (char *line)
> > i.prefix[LOCK_PREFIX] = 0;
> > }
> >
> > + if (tls_check)
> > + {
> > + for (j = i.imm_operands; j < i.operands; ++j)
> > + if (!x86_check_tls_relocation (i.reloc[j]))
> > + {
> > + as_bad (_("`%s' relocation cannot be used with `%s'"),
> > + bfd_reloc_code_real_names[i.reloc[j]],
> > + insn_name (&i.tm));
> > + return;
> > + }
> > + }
>
> Why do you skip immediate operands, which also may have relocations?
>
> > --- a/gas/configure.ac
> > +++ b/gas/configure.ac
> > @@ -95,6 +95,17 @@ AC_ARG_ENABLE(x86_relax_relocations,
> > no) ac_default_x86_relax_relocations=0 ;;
> > esac])dnl
> >
> > +# PR gas/32022
> > +# Decide if check tls relocation.
> > +ac_default_tls_check=unset
> > +# Provide a configure time option to override our default.
> > +AC_ARG_ENABLE(tls_check,
> > + AS_HELP_STRING([--enable-tls-check],
> > + [check tls relocation by default]),
> > +[case "${enableval}" in
> > + no) ac_default_tls_check=0 ;;
> > +esac])dnl
>
> Like for the other one visible in context: Should there be "x86" in the
> name? Or are there plans to extend this to other architectures?
>
> > @@ -737,6 +748,13 @@ AC_DEFINE_UNQUOTED(DEFAULT_GENERATE_X86_RELAX_RELOCATIONS,
> > $ac_default_x86_relax_relocations,
> > [Define to 1 if you want to generate x86 relax relocations by default.])
> >
> > +if test ${ac_default_tls_check} = unset; then
> > + ac_default_tls_check=1
> > +fi
> > +AC_DEFINE_UNQUOTED(DEFAULT_TLS_CHECK,
> > + $ac_default_tls_check,
> > + [Define to 1 if you want to check tls relocation by default.])
>
> If the default is "enabled", shouldn't the option be --disable-tls-check?
>
> > --- a/gas/testsuite/gas/i386/inval-tls.l
> > +++ b/gas/testsuite/gas/i386/inval-tls.l
> > @@ -1,3 +1,9 @@
> > .*: Assembler messages:
> > -.*:3: Error: .* `kmovd'
> > -.*:4: Error: .* `kmovd'
> > +.*:3: Error: `BFD_RELOC_386_TLS_GOTIE' relocation cannot be used with `kmovd'
> > +.*:6: Error: `BFD_RELOC_386_TLS_GD' relocation cannot be used with `add'
> > +.*:9: Error: `BFD_RELOC_386_TLS_LDM' relocation cannot be used with `add'
> > +.*:12: Error: `BFD_RELOC_386_TLS_GOTDESC' relocation cannot be used with `add'
> > +.*:15: Error: `BFD_RELOC_386_TLS_IE' relocation cannot be used with `sub'
> > +.*:18: Error: `BFD_RELOC_386_TLS_GOTIE' relocation cannot be used with `lea'
> > +.*:21: Error: `BFD_RELOC_386_TLS_IE_32' relocation cannot be used with `lea'
> > +.*:24: Error: `BFD_RELOC_386_TLS_DESC_CALL' relocation cannot be used with `lea'
>
> BFD-internal names in diagnostics aren't very helpful. The diagnostics
> should mention either the reloc specifier that was used in source code
> (preferred, for making it easiest to associate the diagnostic with the
> respective location in source code), or - all of this being ELF-only
> anyway if I'm not mistaken - the ELF relocation names.
>
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/tls.d
> > @@ -0,0 +1,28 @@
> > +#as:
> > +#objdump: -dw
>
> Maybe better -drw (also for the 64-bit counterpart)?
True.
H.J.
More information about the Binutils
mailing list