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

[H.J. Lu]

On Tue, Feb 11, 2025 at 5:45 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 11.02.2025 03:05, H.J. Lu wrote:
> > 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.
>
> Oh, right, I was mislead by the comment. It really appears to mean tile
> configuration loads and stores. Yet I find it inconsistent that you check

Will add configuration to comments.

> for those, but not ...
>
> >>> +  /* 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}?
>
> ... for any of these.

Some are covered:

[hjl@gnu-tgl-3 gcc]$ cat x.s
cvtps2pd foo@GOT, %xmm0
[hjl@gnu-tgl-3 gcc]$ as --32 -o x.o x.s
x.s: Assembler messages:
x.s:1: Error: invalid GOT memory operand
[hjl@gnu-tgl-3 gcc]$

More checks can be added later.

> >> 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.
>
> And you've convinced yourself that .insn is unaffected by your change?

Correct.

> >> 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.
> >
> > GOT slot size is 8-byte.   Any memory beyond GOT slot size is undefined,
> > which can be anything, including unmapped or unreadable memory.
>
> No question about this. Yet it doesn't mean people can't do fancy things.
>

They need to find another way to shoot themselves.

-- 
H.J.