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

[H.J. Lu]

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.
>
> Jan

I changed my mind.


-- 
H.J.