Home Original page

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

Jan Beulich jbeulich@suse.com
Tue Feb 11 09:45:18 GMT 2025 
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
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.

>> 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?

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

Jan
More information about the Binutils mailing list