[PATCH V5] x86: Add tls check in gas

[Jan Beulich]

On 14.10.2024 12:06, Cui, Lili wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, October 11, 2024 6:04 PM
>> To: Cui, Lili <lili.cui@intel.com>
>> Cc: binutils@sourceware.org; hjl.tools@gmail.com
>> Subject: Re: [PATCH V5] x86: Add tls check in gas
>>
>> On 11.10.2024 11:28, Cui, Lili wrote:
>>>> On 19.09.2024 08:38, Cui, Lili wrote:
>>>>> +    case BFD_RELOC_386_TLS_GOTIE:
>>>>> +      /* Check 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 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;
>>>>> +      if (i.op[1].regs->reg_type.bitfield.class != Reg
>>>>> +	  || i.op[0].regs->reg_type.bitfield.class
>>>>> +	  || i.imm_operands)
>>>>
>>>> You may not de-reference i.op[0].regs for the intended forms at all.
>>>> If anything, i.op[0].disps may be de-referenced there.
>>>>
>>>> You may also not de-reference i.op[1].regs until you've checked that
>>>> operand 1 actually is a register. There are insn forms after all
>>>> where operand 1 is a memory one.
>>>>
>>>> Which member of the union is valid to de-reference can only be told
>>>> by inspecting i.types[] and/or i.tm.operand_types[] (when insns are
>>>> more constrained than mov/add/sub are, going from just
>>>> i.{reg,imm,mem,disp}_operands may also be possible, just to mention it).
>>>>
>>>> I expect you want to check for operand counts first: No immediate
>>>> one, a single disp one, and a single register one. Then check that
>>>> it's the destination that's the register one. There may then not be
>>>> any need to access any i.op[<n>].reg anymore.
>>>>
>>>
>>> Jan, do you mean to modify it like this?
>>>
>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c index
>>> 25ecaa66104..358d76e7bd2 100644
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -6545,9 +6545,10 @@ x86_check_tls_relocation (enum
>> bfd_reloc_code_real r_type)
>>>           && i.tm.mnem_off != MN_add
>>>           && i.tm.mnem_off != MN_mov)
>>>         return x86_tls_error_insn;
>>> -      if (i.op[1].regs->reg_type.bitfield.class != Reg
>>> -         || i.op[0].regs->reg_type.bitfield.class
>>> -         || i.imm_operands)
>>> +      if (i.imm_operands
>>> +         || i.disp_operands != 1
>>> +         || i.reg_operands != 1
>>> +         || i.types[1].bitfield.class != Reg)
>>>         return x86_tls_error_opcode;
>>
>> This goes in the right direction, but still isn't quite enough, I fear.
>> Knowing there's 1 disp and 1 reg operand still doesn't tell you which one is which
>> (both forms exist, after all). 
> 
> "i.types[1].bitfield.class != Reg" requires dest to be reg, so source should be disp.
> 
>> Plus because of APX there are also 3-operand forms
>> of SUB and ADD (but of course not MOV).
>>
> 
> For APX the 3-operand form, there are two types,
> 
> 1 imm + 2 others   :  “i.imm_operands”  can exclude this situation.
> 1 dis + 2 regs          :  “i.reg_operands != 1” can exclude this situation.

Oh, you're right. I wasn't properly looking at things, I'm sorry.

Jan