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

[Jan Beulich]

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?

> +  /* 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/ ?

> +	  /* 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.

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.

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.

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