x86: Disallow GOT memory as vector memory operand

Message ID CAMe9rOpi4s8x9octBcWAK4stRyusZBtjxFDRfwmgh6usMc7kXA@mail.gmail.com
State New
Headers
Series x86: Disallow GOT memory as vector memory operand |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

H.J. Lu Feb. 4, 2025, 10:19 p.m. UTC
  GOT memory is 4-byte for i386 and 8-byte for x86-64.  Disallow GOT memory
as vector memory operand.

PR gas/32624
* config/tc-i386.c (output_disp): Disallow GOT memory as vector
memory operand.
* 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.exp: Run x86-64-inval-got.
  

Comments

Jan Beulich Feb. 5, 2025, 8:08 a.m. UTC | #1
On 04.02.2025 23:19, H.J. Lu wrote:
> GOT memory is 4-byte for i386 and 8-byte for x86-64.  Disallow GOT memory
> as vector memory operand.

This is going too far and not far enough at the same time.

Too far: Assembly programmers ought to be permitted to do whatever
they like. There may be an advanced mode where we warn about things
we deem bogus.

Not far enough: Why would non-vector operands of the wrong size be
okay? (Correcting this can then easily go too far, though: If
one is after a specific property of a GOT entry, TEST or CMP may,
for example, be used on just the high part [of whatever size] of
it.)

Overall: This is yet another arbitrary heuristic you're introducing.
I did complain about you doing such already in the past. Please
don't. Either there are firm, psABI-mandated criteria, or there
aren't. As said above, a default-off optional check may be okay.

Furthermore, why would you check the template for dword/qword?
Shouldn't this be the actual operand that we're processing? While
doing so has (afaict, without actually trying it out) the advantage
of still permitting broadcasts of the right size, imo that needs
dealing with separately.

> PR gas/32624
> * config/tc-i386.c (output_disp): Disallow GOT memory as vector
> memory operand.
> * 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.exp: Run x86-64-inval-got.

All you test is VPERMB. Imo for a code change like this, testing
needs to cover quite a few more insns / variants. Since it was
just the other day that you demanded that I add testcases to
patches, here's my (general) request: Testcases ought to be
providing good coverage for what needs testing. It is often not
sufficient to merely test the one special case that was pointed
out somewhere.

Jan
  
H.J. Lu Feb. 6, 2025, 3:18 a.m. UTC | #2
On Wed, Feb 5, 2025 at 4:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.02.2025 23:19, H.J. Lu wrote:
> > GOT memory is 4-byte for i386 and 8-byte for x86-64.  Disallow GOT memory
> > as vector memory operand.
>
> This is going too far and not far enough at the same time.
>
> Too far: Assembly programmers ought to be permitted to do whatever
> they like. There may be an advanced mode where we warn about things
> we deem bogus.

Assembler doesn't allow a word memory operand when instruction
requires a 128-bit memory operand.

> Not far enough: Why would non-vector operands of the wrong size be
> okay? (Correcting this can then easily go too far, though: If
> one is after a specific property of a GOT entry, TEST or CMP may,
> for example, be used on just the high part [of whatever size] of
> it.)

Smaller than GOT slot size should be OK.

> Overall: This is yet another arbitrary heuristic you're introducing.
> I did complain about you doing such already in the past. Please
> don't. Either there are firm, psABI-mandated criteria, or there
> aren't. As said above, a default-off optional check may be okay.
>
> Furthermore, why would you check the template for dword/qword?

Some vector instructions take dword/qword and vector memory.

> Shouldn't this be the actual operand that we're processing? While

It sounds good.  But it is more complex than I like.

> doing so has (afaict, without actually trying it out) the advantage
> of still permitting broadcasts of the right size, imo that needs
> dealing with separately.
>
> > PR gas/32624
> > * config/tc-i386.c (output_disp): Disallow GOT memory as vector
> > memory operand.
> > * 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.exp: Run x86-64-inval-got.
>
> All you test is VPERMB. Imo for a code change like this, testing
> needs to cover quite a few more insns / variants. Since it was
> just the other day that you demanded that I add testcases to

Good point.  I will add some more tests.

FWIW, you provided some patches without ANY tests whatsoever.

> patches, here's my (general) request: Testcases ought to be
> providing good coverage for what needs testing. It is often not
> sufficient to merely test the one special case that was pointed
> out somewhere.
>
> Jan
  
Jan Beulich Feb. 6, 2025, 12:46 p.m. UTC | #3
On 06.02.2025 04:18, H.J. Lu wrote:
> On Wed, Feb 5, 2025 at 4:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.02.2025 23:19, H.J. Lu wrote:
>>> GOT memory is 4-byte for i386 and 8-byte for x86-64.  Disallow GOT memory
>>> as vector memory operand.
>>
>> This is going too far and not far enough at the same time.
>>
>> Too far: Assembly programmers ought to be permitted to do whatever
>> they like. There may be an advanced mode where we warn about things
>> we deem bogus.
> 
> Assembler doesn't allow a word memory operand when instruction
> requires a 128-bit memory operand.

I can't connect your reply to my comment.

>> Not far enough: Why would non-vector operands of the wrong size be
>> okay? (Correcting this can then easily go too far, though: If
>> one is after a specific property of a GOT entry, TEST or CMP may,
>> for example, be used on just the high part [of whatever size] of
>> it.)
> 
> Smaller than GOT slot size should be OK.

vpbroadcast{b,w,d,q}? cmpxchg{8,16}b? (Just to name two falling in
opposite categories in this regard. Not to speak of {F,FX,X}{SAVE,RSTOR}
and alike.)

>> Overall: This is yet another arbitrary heuristic you're introducing.
>> I did complain about you doing such already in the past. Please
>> don't. Either there are firm, psABI-mandated criteria, or there
>> aren't. As said above, a default-off optional check may be okay.
>>
>> Furthermore, why would you check the template for dword/qword?
> 
> Some vector instructions take dword/qword and vector memory.

Correct. And why would such an operand not be permitted to live in
GOT?

>> Shouldn't this be the actual operand that we're processing? While
> 
> It sounds good.  But it is more complex than I like.

IOW if already it's arbitrary, it can be arbitrarily arbitrary? At
the risk of sounding like a broken record: Assembler behavior should
be as predictable as possible for a programmer using it.

Jan
  

Patch

From 4b17304863a821b899597e85d44e82df189884e1 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 5 Feb 2025 06:13:37 +0800
Subject: [PATCH] x86: Disallow GOT memory as vector memory operand

GOT memory is 4-byte for i386 and 8-byte for x86-64.  Disallow GOT memory
as vector memory operand.

	PR gas/32624
	* config/tc-i386.c (output_disp): Disallow GOT memory as vector
	memory operand.
	* 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.exp: Run x86-64-inval-got.

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 gas/config/tc-i386.c                      | 12 ++++++++++++
 gas/testsuite/gas/i386/i386.exp           |  1 +
 gas/testsuite/gas/i386/inval-got.l        | 12 ++++++++++++
 gas/testsuite/gas/i386/inval-got.s        |  3 +++
 gas/testsuite/gas/i386/x86-64-inval-got.l | 12 ++++++++++++
 gas/testsuite/gas/i386/x86-64-inval-got.s |  3 +++
 gas/testsuite/gas/i386/x86-64.exp         |  1 +
 7 files changed, 44 insertions(+)
 create mode 100644 gas/testsuite/gas/i386/inval-got.l
 create mode 100644 gas/testsuite/gas/i386/inval-got.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-inval-got.l
 create mode 100644 gas/testsuite/gas/i386/x86-64-inval-got.s

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 2b5f07519f9..f276b63956d 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -12788,6 +12788,18 @@  output_disp (fragS *insn_start_frag, offsetT insn_start_off)
 
 	      p = frag_more (size);
 	      reloc_type = reloc (size, pcrel, sign, i.reloc[n]);
+
+	      if (GOT_symbol
+		  && ((reloc_type == BFD_RELOC_386_GOT32
+		       && !i.tm.operand_types[n].bitfield.dword)
+		   || (reloc_type == BFD_RELOC_32_PCREL
+		       && !i.tm.operand_types[n].bitfield.qword))
+		  && (i.tm.operand_types[n].bitfield.xmmword
+		      || i.tm.operand_types[n].bitfield.ymmword
+		      || i.tm.operand_types[n].bitfield.zmmword
+		      || i.tm.operand_types[n].bitfield.tmmword))
+		as_bad (_("invalid GOT memory operand"));
+
 	      if (GOT_symbol
 		  && GOT_symbol == i.op[n].disps->X_add_symbol
 		  && (((reloc_type == BFD_RELOC_32
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index b4d33cc7500..a22e96c77f6 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -53,6 +53,7 @@  if [gas_32_check] then {
     run_list_test "general" "-al --listing-lhs-width=2"
     run_list_test "inval" "-aln"
     run_list_test "inval-16" "-al"
+    run_list_test "inval-got" "-al"
     run_list_test "segment" "-al"
     run_list_test "inval-seg" "-al"
     run_list_test "inval-reg" "-al"
diff --git a/gas/testsuite/gas/i386/inval-got.l b/gas/testsuite/gas/i386/inval-got.l
new file mode 100644
index 00000000000..e6327d9699d
--- /dev/null
+++ b/gas/testsuite/gas/i386/inval-got.l
@@ -0,0 +1,12 @@ 
+.*: Assembler messages:
+.*:3: Error: .*
+GAS LISTING .*
+
+
+[ 	]*1[ 	]+\.text
+[ 	]*2[ 	]+# All the following should be illegal
+[ 	]*3[ 	]+\?\?\?\? 62F27548 		vpermb	foo@got, %zmm1, %zmm0
+.*  Error: invalid GOT memory operand
+[ 	]*3[ 	]+8D050000 +
+[ 	]*3[ 	]+0000
+#pass
diff --git a/gas/testsuite/gas/i386/inval-got.s b/gas/testsuite/gas/i386/inval-got.s
new file mode 100644
index 00000000000..66c7fcc3b26
--- /dev/null
+++ b/gas/testsuite/gas/i386/inval-got.s
@@ -0,0 +1,3 @@ 
+	.text
+# All the following should be illegal
+	vpermb	foo@got, %zmm1, %zmm0
diff --git a/gas/testsuite/gas/i386/x86-64-inval-got.l b/gas/testsuite/gas/i386/x86-64-inval-got.l
new file mode 100644
index 00000000000..0cd11edff97
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-inval-got.l
@@ -0,0 +1,12 @@ 
+.*: Assembler messages:
+.*:3: Error: .*
+GAS LISTING .*
+
+
+[ 	]*1[ 	]+\.text
+[ 	]*2[ 	]+# All the following should be illegal for x86-64
+[ 	]*3[ 	]+\?\?\?\? 62F27548 		vpermb	foo@GOTPCREL\(%rip\), %zmm1, %zmm0
+.*  Error: invalid GOT memory operand
+[ 	]*3[ 	]+8D050000 +
+[ 	]*3[ 	]+0000
+#pass
diff --git a/gas/testsuite/gas/i386/x86-64-inval-got.s b/gas/testsuite/gas/i386/x86-64-inval-got.s
new file mode 100644
index 00000000000..11afb783ed4
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-inval-got.s
@@ -0,0 +1,3 @@ 
+	.text
+# All the following should be illegal for x86-64
+	vpermb	foo@GOTPCREL(%rip), %zmm1, %zmm0
diff --git a/gas/testsuite/gas/i386/x86-64.exp b/gas/testsuite/gas/i386/x86-64.exp
index edacbaa0f20..f1462e36131 100644
--- a/gas/testsuite/gas/i386/x86-64.exp
+++ b/gas/testsuite/gas/i386/x86-64.exp
@@ -71,6 +71,7 @@  run_dump_test "x86-64-stack"
 run_dump_test "x86-64-stack-intel"
 run_dump_test "x86-64-stack-suffix"
 run_list_test "x86-64-inval" "-al"
+run_list_test "x86-64-inval-got" "-al"
 run_list_test "x86-64-segment" "-al"
 run_dump_test "x86-64-segovr"
 run_list_test "x86-64-inval-seg" "-al"
-- 
2.48.1