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

[H.J. Lu]

On Mon, Feb 10, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.02.2025 02:34, H.J. Lu wrote:
> > Since GOT slot size is 32 bits for i386 and 64 bits for x86-64, disallow
> > GOT memory access beyond its GOT slot.
> >
> > PR gas/32624
> > * config/tc-i386.c (check_GOT_memory): New function.
> > (output_disp): Call check_GOT_memory to check valid GOT memory
> > access.
> > * testsuite/gas/i386/got.s: Add vector instructions with
> > byte/word/dword GOT memory.
> > * testsuite/gas/i386/got-no-relax.d: Updated.
> > * testsuite/gas/i386/got.d: Likewise.
> > * testsuite/gas/i386/x86-64-gotpcrel-2.d: Likewise.
> > * testsuite/gas/i386/i386.exp: Run inval-got.
> > * testsuite/gas/i386/inval-got.l: New file.
> > * testsuite/gas/i386/inval-got.s: Likewise.
> > * testsuite/gas/i386/x86-64-inval-got.l: Likewise.
> > * testsuite/gas/i386/x86-64-inval-got.l: Likewise.
> > * testsuite/gas/i386/x86-64-gotpcrel-2.s: Add vector instructions
> > with byte/word/dword/qword GOT memory.
> > * testsuite/gas/i386/x86-64.exp: Run x86-64-inval-got.
>
> First: It's harder than necessary to reply to patches when those aren't
> (also) sent inline. I'll quote minimal context.
>
> > +  /* Disallow AMX TILE load and store instructions.  */
> > +  if (is_cpu (&i.tm, CpuAMX_TILE))
> > +    return false;
>
> How do you know what element and total (contiguous) size they access?
> What about the recently added load forms, which aren't AMX-TILE?

Other AMX load instructions don't allow RIP relative address.

> > +  /* Disallow instructions with 6-byte and 8-byte memory access.  */
> > +  if (i.tm.operand_types[n].bitfield.fword
> > +      || i.tm.operand_types[n].bitfield.tbyte)
> > +    return false;
>
> In the comment s/8/10/ ?

Fixed.

> > +       /* Allow memory access within GOT slot without broadcast.  */
> > +       gotmemshift = qword ? 3 : 2;
> > +       return i.tm.opcode_modifier.disp8memshift <= gotmemshift;
>
> The Disp8 shift doesn't always represent the full operand size.

Fixed on v4.

> Having reached the end of check_GOT_memory(), 8-byte accesses to GOT look
> to be permitted on 32-bit? What about MOVDIR64B and ENQCMD{,S}?
>
> While admittedly obscure, larger block accesses ({F,FX,X}RSTOR and alike)
> also continue to be permitted.

True.  We can add more checks later.

> Further, didn't you earlier on actually agree that .insn should remain
> unaffected? It's not entirely obvious that it indeed isn't, as s_insn()
> certainly plays with i.tm in various ways.

Correct.  They need to know what  they are doing.

> Just to repeat - even if all of the above were address in a v4, my general
> concern would remain: Why would we know better what people may or may not
> do? One further aspect occurred to me in the meantime: You check and reject
> direct (single-insn) accesses. An access involving two or more insns, otoh,
> is apparently fine? (Rhetorical question; yes, it is, as are direct ones.
> This is assembly language. People need to know what they're doing. We
> should only reject stuff that we are entirely certain is invalid, or that
> we can't make sense of.)
>
> It's also not clear to me what you hope to gain from this, besides making
> the assembler slightly slower. The linker, in particular, still can't
> assume it'll never get to see such accesses.
>
> IOW: No matter whether in the end the patch is technically perfect, I still
> object to it going in. Silence on my part on future versions of the patch
> will _not_ indicate silent agreement.
>
> Jan

GOT slot size is 8-byte.   Any memory beyond GOT slot size is undefined,
which can be anything, including unmapped or unreadable memory.

-- 
H.J.