diff mbox series

x86: Disallow APX instruction with length > 15 bytes

Message ID 20240201224749.214439-1-hjl.tools@gmail.com
State Unresolved
Headers
Series x86: Disallow APX instruction with length > 15 bytes |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

H.J. Lu Feb. 1, 2024, 10:47 p.m. UTC 
  It is a hard error when an APX instruction length exceeds the limit of
15 bytes:

[hjl@gnu-cfl-3 pr31323]$ cat z.s
	addq $0xe0, %fs:0, %rdx
[hjl@gnu-cfl-3 pr31323]$ as -o z.o z.s
z.s: Assembler messages:
z.s:1: Warning: instruction length of 16 bytes exceeds the limit of 15
[hjl@gnu-cfl-3 pr31323]$ objdump -dw z.o

z.o:     file format elf64-x86-64

Disassembly of section .text:

0000000000000000 <.text>:
   0:	64 62 f4 ec 18 81 04 25 00 00 00 00 e0 00 00 	(bad)
	...
[hjl@gnu-cfl-3 pr31323]$

We should issue an error when APX instruction length exceeds the limit of
15 bytes.

	PR gas/31323
	* config/tc-i386.c (output_insn): Issue an error when APX
	instruction length exceeds the limit of 15 bytes.
	* testsuite/gas/i386/x86-64-apx-inval.l: New file.
	* testsuite/gas/i386/x86-64-apx-inval.s: Likewise.
---
 gas/config/tc-i386.c                      | 10 ++++++++--
 gas/testsuite/gas/i386/x86-64-apx-inval.l |  3 +++
 gas/testsuite/gas/i386/x86-64-apx-inval.s |  4 ++++
 gas/testsuite/gas/i386/x86-64.exp         |  1 +
 4 files changed, 16 insertions(+), 2 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-inval.l
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-inval.s

Comments

Jan Beulich Feb. 2, 2024, 7:23 a.m. UTC | #1 
On 01.02.2024 23:47, H.J. Lu wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -11772,8 +11772,14 @@ output_insn (const struct last_insn *last_insn)
>  	{
>  	  j = encoding_length (insn_start_frag, insn_start_off, frag_more (0));
>  	  if (j > 15)
> -	    as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
> -		     j);
> +	    {
> +	      if (i.tm.cpu.bitfield.cpuapx_f)
> +		as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
> +			j);
> +	      else
> +		as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
> +			 j);
> +	    }

Why would APX insns be different from others? IOW I continue to think that
having a warning here is good enough, uniformly. And it's quite sad that
with introducing APX the limit isn't raised, to accommodate all valid insn
forms (not considering ones with redundant prefixes, of course). _That_
would then permit special casing APX here, in _not_ warning anymore.

Jan
H.J. Lu Feb. 2, 2024, 11:36 a.m. UTC | #2 
On Thu, Feb 1, 2024 at 11:23 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.02.2024 23:47, H.J. Lu wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -11772,8 +11772,14 @@ output_insn (const struct last_insn *last_insn)
> >       {
> >         j = encoding_length (insn_start_frag, insn_start_off, frag_more (0));
> >         if (j > 15)
> > -         as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
> > -                  j);
> > +         {
> > +           if (i.tm.cpu.bitfield.cpuapx_f)
> > +             as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
> > +                     j);
> > +           else
> > +             as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
> > +                      j);
> > +         }
>
> Why would APX insns be different from others? IOW I continue to think that

No.  It is just very easy to generate invalid instructions with APX.

> having a warning here is good enough, uniformly. And it's quite sad that

We ran into this with real codes and triggered run-time errors.

> with introducing APX the limit isn't raised, to accommodate all valid insn
> forms (not considering ones with redundant prefixes, of course). _That_
> would then permit special casing APX here, in _not_ warning anymore.
>

Here is the v2 patch:

https://sourceware.org/pipermail/binutils/2024-February/132285.html

to change warning to error for all instructions.
diff mbox series

Patch

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 3c64057fd67..d1e522d3637 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -11772,8 +11772,14 @@  output_insn (const struct last_insn *last_insn)
 	{
 	  j = encoding_length (insn_start_frag, insn_start_off, frag_more (0));
 	  if (j > 15)
-	    as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
-		     j);
+	    {
+	      if (i.tm.cpu.bitfield.cpuapx_f)
+		as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
+			j);
+	      else
+		as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
+			 j);
+	    }
 	  else if (fragP)
 	    {
 	      /* NB: Don't add prefix with GOTPC relocation since
diff --git a/gas/testsuite/gas/i386/x86-64-apx-inval.l b/gas/testsuite/gas/i386/x86-64-apx-inval.l
new file mode 100644
index 00000000000..6c1a346fcbf
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-inval.l
@@ -0,0 +1,3 @@ 
+.*: Assembler messages:
+.*:3: Error: instruction length of 16 bytes exceeds the limit of 15
+.*:4: Error: instruction length of 16 bytes exceeds the limit of 15
diff --git a/gas/testsuite/gas/i386/x86-64-apx-inval.s b/gas/testsuite/gas/i386/x86-64-apx-inval.s
new file mode 100644
index 00000000000..bb57817bc8a
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-inval.s
@@ -0,0 +1,4 @@ 
+# Check illegal 64bit APX_F instructions
+	.text
+	addq $0xe0, %fs:0, %rdx
+	xorq $0xe0, foo(%eax,%edx), %rdx
diff --git a/gas/testsuite/gas/i386/x86-64.exp b/gas/testsuite/gas/i386/x86-64.exp
index 6932ba97a4d..b77e8c10029 100644
--- a/gas/testsuite/gas/i386/x86-64.exp
+++ b/gas/testsuite/gas/i386/x86-64.exp
@@ -371,6 +371,7 @@  run_dump_test "x86-64-avx512f-rcigrne-intel"
 run_dump_test "x86-64-avx512f-rcigrne"
 run_dump_test "x86-64-avx512f-rcigru-intel"
 run_dump_test "x86-64-avx512f-rcigru"
+run_list_test "x86-64-apx-inval"
 run_list_test "x86-64-apx-egpr-inval"
 run_dump_test "x86-64-apx-evex-promoted-bad"
 run_list_test "x86-64-apx-egpr-promote-inval" "-al"