[PATCH v5] x86: Disallow GOT memory access beyond its GOT slot

[Jan Beulich]

On 13.02.2025 10:52, H.J. Lu wrote:
> On Thu, Feb 13, 2025 at 5:18 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 13.02.2025 09:25, H.J. Lu wrote:
>>> On Thu, Feb 13, 2025 at 4:11 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 13.02.2025 03:16, H.J. Lu wrote:
>>>>> On Wed, Feb 12, 2025 at 7:22 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 11.02.2025 22:47, H.J. Lu wrote:
>>>>>>> @@ -12725,6 +12739,51 @@ imm_size (unsigned int n)
>>>>>>>    return size;
>>>>>>>  }
>>>>>>>
>>>>>>> +/* Since GOT slot size is 32 bits for i386 and 64 bits for x86-64,
>>>>>>> +   disallow GOT memory access beyond its GOT slot.  */
>>>>>>> +
>>>>>>> +static bool
>>>>>>> +check_GOT_memory (enum bfd_reloc_code_real reloc_type, unsigned int n)
>>>>>>> +{
>>>>>>> +  bool qword;
>>>>>>> +
>>>>>>> +  qword = false;
>>>>>>> +  if (reloc_type == BFD_RELOC_32_PCREL)
>>>>>>> +    {
>>>>>>> +      if (!GOT_symbol || GOT_symbol != i.op[n].disps->X_op_symbol)
>>>>>>> +     return true;
>>>>>>> +      qword = true;
>>>>>>> +    }
>>>>>>> +  else if (reloc_type != BFD_RELOC_386_GOT32)
>>>>>>> +    return true;
>>>>>>
>>>>>> What about BFD_RELOC_X86_64_GOT32? The 64-bit ABI simply refers to the
>>>>>> 32-bit one for many reloc types, including R_X86_64_GOT32.
>>>>>
>>>>> It can be added later.
>>>>
>>>> Wow, I'm impressed by how you try to really have entirely unpredictable
>>>> behavior in gas.
>>>>
>>>>>>> +  /* Disallow AMX TILE configuration load and store instructions.  */
>>>>>>> +  if (is_cpu (&i.tm, CpuAMX_TILE))
>>>>>>> +    return false;
>>>>>>> +
>>>>>>> +  /* Disallow instructions with 6-byte and 10-byte memory access.  */
>>>>>>> +  if (i.tm.operand_types[n].bitfield.fword
>>>>>>> +      || i.tm.operand_types[n].bitfield.tbyte)
>>>>>>> +    return false;
>>>>>>
>>>>>> Coming back to my concern regarding .insn: Can you explain to me how this
>>>>>> (as an example) isn't going to affect .insn (using Intel syntax), when
>>>>>> s_insn() has
>>>>>>
>>>>>>       for (j = 0; j < i.operands; ++j)
>>>>>>         i.tm.operand_types[j] = i.types[j];
>>>>>
>>>>> A testcase?
>>>>
>>>> Well, no. You're to prove (or at least make plausible) that your patch
>>>> (which shouldn't be committed anyway, considering my objection to the
>>>> overall idea) is correct (also applies to the similar other reply you
>>>> sent). Plus you can't really expect me to put yet more time in something
>>>> I think should never have been proposed in the first place, for not
>>>> really offering any benefit. Any .insn with a @gotpcrel(%rip) operand
>>>> and an "fword ptr" or "tbyte ptr" size specifier ought to do. I.e.
>>>> without having tried it, perhaps:
>>>>
>>>>         .insn 0x89, tbyte ptr sym@gotpcrel[rip], rax
>>>
>>> [hjl@gnu-zen4-1 tmp]$ gcc -c x.s
>>> x.s: Assembler messages:
>>> x.s:2: Error: invalid GOT memory operand
>>> [hjl@gnu-zen4-1 tmp]$
>>
>> Yet as you agreed with earlier, .insn should remain unaffected by this kind
>> of change.
> 
> I changed my mind.

When upon my explicit question on v3 "And you've convinced yourself that
.insn is unaffected by your change?" you replied "Correct"? And then
above you asked for a testcase? And then you took what I provided and
replied back with entirely uncommented assembler output? I'm sorry to say
this, but that's entirely suspect behavior.

In any event, since I suspect you're going to ignore my objections and
put in the change sooner or later anyway, and since I'm not going to
engage in a revert fight, let me make clear that if you don't make sure
.insn is entirely unaffected here, I would add respective code afterwards.
.insn was specifically introduced to give people the option of doing
things they can't to by other means. We should put as few restrictions as
possible on what can be done with it. (In fact I'd be happy for people to
come up with ideas on how to remove some that are presently there.)

Jan