[PATCH v2] x86: Disallow instructions with length > 15 bytes

[PATCH v2] x86: Disallow instructions with length > 15 bytes

[H.J. Lu]

It is a hard error when an instruction length exceeds the limit of 15
bytes:

[hjl@gnu-cfl-3 tmp]$ cat x.s
	.text
	xacquire lock addq $0x11223344, %fs:(,%eax)
[hjl@gnu-cfl-3 tmp]$ gcc -c x.s
x.s: Assembler messages:
x.s:2: Warning: instruction length of 16 bytes exceeds the limit of 15
[hjl@gnu-cfl-3 tmp]$ objdump -dw x.o

x.o:     file format elf64-x86-64

Disassembly of section .text:

0000000000000000 <.text>:
   0:	64 67 f2 f0 48 81 04 05 00 00 00 00 44 33 22 	xacquire lock (bad)
   f:	11                   	.byte 0x11
[hjl@gnu-cfl-3 tmp]$

and

[hjl@gnu-cfl-3 tmp]$ cat z.s
	addq $0xe0, %fs:0, %rdx
[hjl@gnu-cfl-3 tmp]$ 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 tmp]$ 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]$

Instructions with length > 15 bytes are always invalid.  It is quite easy
to generate invalid instructions with AVX now.  We should issue an error
when instruction length exceeds the limit of 15 bytes.

	PR gas/31323
	* config/tc-i386.c (output_insn): Issue an error when instruction
	length exceeds the limit of 15 bytes.
	* testsuite/gas/i386/oversized16.l: Updated.
	* testsuite/gas/i386/oversized64.l: Likewise.
	* testsuite/gas/i386/x86-64-apx-inval.l: New file.
	* testsuite/gas/i386/x86-64-apx-inval.s: Likewise.
---
 gas/config/tc-i386.c                      |  4 +--
 gas/testsuite/gas/i386/oversized16.l      | 30 +++++++++++------------
 gas/testsuite/gas/i386/oversized64.l      | 30 +++++++++++------------
 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 +
 6 files changed, 40 insertions(+), 32 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

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 3e1811a092d..8f3a1b6f686 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -11780,8 +11780,8 @@ 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);
+	    as_bad (_("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/oversized16.l b/gas/testsuite/gas/i386/oversized16.l
index 3bf7e1d8095..e0fc7619bdc 100644
--- a/gas/testsuite/gas/i386/oversized16.l
+++ b/gas/testsuite/gas/i386/oversized16.l
@@ -1,9 +1,9 @@
 .*: Assembler messages:
-.*:6: Warning: instruction length.*
-.*:7: Warning: instruction length.*
-.*:8: Warning: instruction length.*
-.*:10: Warning: instruction length.*
-.*:11: Warning: instruction length.*
+.*:6: Error: instruction length.*
+.*:7: Error: instruction length.*
+.*:8: Error: instruction length.*
+.*:10: Error: instruction length.*
+.*:11: Error: instruction length.*
 GAS LISTING .*
 
 
@@ -12,29 +12,29 @@ GAS LISTING .*
 [ 	]*3[ 	]+\.code16
 [ 	]*4[ 	]+
 [ 	]*5[ 	]+long16:
-[ 	]*6[ 	]+0000 26678FEA[ 	]+lwpins	eax, es:\[eax\*8\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*6[ 	]+\?\?\?\? 26678FEA[ 	]+lwpins	eax, es:\[eax\*8\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*6[ 	]+781204C5[ 	]*
 [ 	]*6[ 	]+00000000[ 	]*
 [ 	]*6[ 	]+44332211[ 	]*
-[ 	]*7[ 	]+0010 26678FEA[ 	]+lwpval	eax, es:\[eax\*4\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*7[ 	]+\?\?\?\? 26678FEA[ 	]+lwpval	eax, es:\[eax\*4\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*7[ 	]+78120C85[ 	]*
 [ 	]*7[ 	]+00000000[ 	]*
 [ 	]*7[ 	]+44332211[ 	]*
-[ 	]*8[ 	]+0020 26678FEA[ 	]+bextr	eax, es:\[eax\*2\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*8[ 	]+\?\?\?\? 26678FEA[ 	]+bextr	eax, es:\[eax\*2\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*8[ 	]+78100445[ 	]*
 [ 	]*8[ 	]+00000000[ 	]*
 [ 	]*8[ 	]+44332211[ 	]*
 [ 	]*9[ 	]+
-[ 	]*10[ 	]+0030 266766F2[ 	]+xacquire lock add dword ptr es:\[eax\*2\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*10[ 	]+\?\?\?\? 266766F2[ 	]+xacquire lock add dword ptr es:\[eax\*2\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*10[ 	]+F0810445[ 	]*
 [ 	]*10[ 	]+00000000[ 	]*
 [ 	]*10[ 	]+44332211[ 	]*
-[ 	]*11[ 	]+0040 266766F3[ 	]+xrelease lock sub dword ptr es:\[eax\*2\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*11[ 	]+\?\?\?\? 266766F3[ 	]+xrelease lock sub dword ptr es:\[eax\*2\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*11[ 	]+F0812C45[ 	]*
 [ 	]*11[ 	]+00000000[ 	]*
 [ 	]*11[ 	]+44332211[ 	]*
diff --git a/gas/testsuite/gas/i386/oversized64.l b/gas/testsuite/gas/i386/oversized64.l
index dc63fb5ae77..ac32c4d8139 100644
--- a/gas/testsuite/gas/i386/oversized64.l
+++ b/gas/testsuite/gas/i386/oversized64.l
@@ -1,9 +1,9 @@
 .*: Assembler messages:
-.*:5: Warning: instruction length.*
-.*:6: Warning: instruction length.*
-.*:7: Warning: instruction length.*
-.*:9: Warning: instruction length.*
-.*:10: Warning: instruction length.*
+.*:5: Error: instruction length.*
+.*:6: Error: instruction length.*
+.*:7: Error: instruction length.*
+.*:9: Error: instruction length.*
+.*:10: Error: instruction length.*
 GAS LISTING .*
 
 
@@ -11,29 +11,29 @@ GAS LISTING .*
 [ 	]*2[ 	]+\.intel_syntax noprefix
 [ 	]*3[ 	]+
 [ 	]*4[ 	]+long64:
-[ 	]*5[ 	]+0000 64678FEA[ 	]+lwpins	rax, fs:\[eax\*8\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*5[ 	]+\?\?\?\? 64678FEA[ 	]+lwpins	rax, fs:\[eax\*8\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*5[ 	]+F81204C5[ 	]*
 [ 	]*5[ 	]+00000000[ 	]*
 [ 	]*5[ 	]+44332211[ 	]*
-[ 	]*6[ 	]+0010 64678FEA[ 	]+lwpval	eax, fs:\[eax\*4\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*6[ 	]+\?\?\?\? 64678FEA[ 	]+lwpval	eax, fs:\[eax\*4\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*6[ 	]+78120C85[ 	]*
 [ 	]*6[ 	]+00000000[ 	]*
 [ 	]*6[ 	]+44332211[ 	]*
-[ 	]*7[ 	]+0020 64678FEA[ 	]+bextr	rax, fs:\[eax\*2\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*7[ 	]+\?\?\?\? 64678FEA[ 	]+bextr	rax, fs:\[eax\*2\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*7[ 	]+F8100445[ 	]*
 [ 	]*7[ 	]+00000000[ 	]*
 [ 	]*7[ 	]+44332211[ 	]*
 [ 	]*8[ 	]+
-[ 	]*9[ 	]+0030 6567F2F0[ 	]+xacquire lock add qword ptr gs:\[eax\*8\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*9[ 	]+\?\?\?\? 6567F2F0[ 	]+xacquire lock add qword ptr gs:\[eax\*8\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*9[ 	]+488104C5[ 	]*
 [ 	]*9[ 	]+00000000[ 	]*
 [ 	]*9[ 	]+44332211[ 	]*
-[ 	]*10[ 	]+0040 6567F3F0[ 	]+xrelease lock sub qword ptr gs:\[eax\*8\], 0x11223344
-\*\*\*\*  Warning: instruction length of 16 bytes exceeds the limit of 15
+[ 	]*10[ 	]+\?\?\?\? 6567F3F0[ 	]+xrelease lock sub qword ptr gs:\[eax\*8\], 0x11223344
+\*\*\*\*  Error: instruction length of 16 bytes exceeds the limit of 15
 [ 	]*10[ 	]+48812CC5[ 	]*
 [ 	]*10[ 	]+00000000[ 	]*
 [ 	]*10[ 	]+44332211[ 	]*
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"
-- 
2.43.0

Re: [PATCH v2] x86: Disallow instructions with length > 15 bytes

[Jan Beulich]

On 02.02.2024 12:33, H.J. Lu wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -11780,8 +11780,8 @@ 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);
> +	    as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
> +		    j);

When taking purely a gas perspective, this may be okay. But I'd like
to retain the ability to generate overlong insns (without resorting
to .byte), so there wants to be a way to downgrade the error to a
warning. This is useful in particular when trying to make testcases
for disassemblers or other software decoding insns.

Jan

Re: [PATCH v2] x86: Disallow instructions with length > 15 bytes

[H.J. Lu]

On Fri, Feb 2, 2024 at 3:41 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.02.2024 12:33, H.J. Lu wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -11780,8 +11780,8 @@ 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);
> > +         as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
> > +                 j);
>
> When taking purely a gas perspective, this may be okay. But I'd like
> to retain the ability to generate overlong insns (without resorting
> to .byte), so there wants to be a way to downgrade the error to a
> warning. This is useful in particular when trying to make testcases
> for disassemblers or other software decoding insns.
>
> Jan

I am checking it in now and backport it to 2.42 branch.   We can add
a switch or a directive later to downgrade error to warning.

-- 
H.J.

Re: [PATCH v2] x86: Disallow instructions with length > 15 bytes

[Jan Beulich]

On 02.02.2024 12:49, H.J. Lu wrote:
> On Fri, Feb 2, 2024 at 3:41 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.02.2024 12:33, H.J. Lu wrote:
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -11780,8 +11780,8 @@ 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);
>>> +         as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
>>> +                 j);
>>
>> When taking purely a gas perspective, this may be okay. But I'd like
>> to retain the ability to generate overlong insns (without resorting
>> to .byte), so there wants to be a way to downgrade the error to a
>> warning. This is useful in particular when trying to make testcases
>> for disassemblers or other software decoding insns.
> 
> I am checking it in now and backport it to 2.42 branch.   We can add
> a switch or a directive later to downgrade error to warning.

Putting on the branch should be (have been?) deferred until the complete
solution is in place. 2.42.1 could be cut from it at basically any time,
at which point your change would regress the particular case I've been
mentioning. Furthermore I have to raise two more points:
- The 15-byte limit - while long-standing - being rather arbitrary, it
  could in principle be changed (bumped) at any time (as indicated, I
  think the latest with APX it should be bumped). Whenever such a bump
  occurs, the assembler outright refusing to assemble respective insns
  will be a problem.
- .insn is particularly intended to permit encoding things the assembler
  may not otherwise be able to encode. Therefore the diagnostic there
  ought to be more relaxed than for "ordinary" insns.

Jan

Re: [PATCH v2] x86: Disallow instructions with length > 15 bytes

[H.J. Lu]

On Sun, Feb 4, 2024 at 11:41 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.02.2024 12:49, H.J. Lu wrote:
> > On Fri, Feb 2, 2024 at 3:41 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 02.02.2024 12:33, H.J. Lu wrote:
> >>> --- a/gas/config/tc-i386.c
> >>> +++ b/gas/config/tc-i386.c
> >>> @@ -11780,8 +11780,8 @@ 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);
> >>> +         as_bad (_("instruction length of %u bytes exceeds the limit of 15"),
> >>> +                 j);
> >>
> >> When taking purely a gas perspective, this may be okay. But I'd like
> >> to retain the ability to generate overlong insns (without resorting
> >> to .byte), so there wants to be a way to downgrade the error to a
> >> warning. This is useful in particular when trying to make testcases
> >> for disassemblers or other software decoding insns.
> >
> > I am checking it in now and backport it to 2.42 branch.   We can add
> > a switch or a directive later to downgrade error to warning.
>
> Putting on the branch should be (have been?) deferred until the complete
> solution is in place. 2.42.1 could be cut from it at basically any time,
> at which point your change would regress the particular case I've been
> mentioning. Furthermore I have to raise two more points:
> - The 15-byte limit - while long-standing - being rather arbitrary, it
>   could in principle be changed (bumped) at any time (as indicated, I
>   think the latest with APX it should be bumped). Whenever such a bump
>   occurs, the assembler outright refusing to assemble respective insns
>   will be a problem.

15 bytes is a hard limit, which also applies to APX.  When the size
limit is increased for a new ISA in the future, we will allow the bigger
size only for the specific ISA.

> - .insn is particularly intended to permit encoding things the assembler
>   may not otherwise be able to encode. Therefore the diagnostic there
>   ought to be more relaxed than for "ordinary" insns.
>

I will see what I can do about it.

-- 
H.J.