* [PATCH] x86: Fix SYSRETQ disassembly
@ 2020-10-04 21:00 Borislav Petkov
2020-10-04 21:05 ` H.J. Lu
0 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2020-10-04 21:00 UTC (permalink / raw)
To: Binutils
Disassembling this:
int main(void)
{
asm volatile("sysretq");
return 0;
}
gives:
1129: 48 0f 07 rex.W sysret
because the break condition in putop() which handles a %LQ (the template
is "sysret%LQ") looks at ModRM.mod before verifying first that the
instruction actually even has a ModRM byte and SYSRET doesn't, for
example.
Check need_modrm first before accessing a stale modrm value.
---
opcodes/ChangeLog | 4 ++++
opcodes/i386-dis.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog
index c57c0b1e209f..bc43113cf771 100644
--- a/opcodes/ChangeLog
+++ b/opcodes/ChangeLog
@@ -1,3 +1,7 @@
+2020-10-04 Borislav Petkov <bp@suse.de>
+
+ * i386-dis.c: Fix SYSRETQ disassembly.
+
2020-09-28 Przemyslaw Wirkus <przemyslaw.wirkus@arm.com>
* aarch64-opc.c: Added ETMv4 system registers TRCACATRn, TRCACVRn,
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 67d5b6aaefc6..53cadb6a4401 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -10858,7 +10858,7 @@ putop (const char *in_template, int sizeflag)
*obufp++ = vex.w ? 'q' : 'd';
else if (l == 1 && last[0] == 'L')
{
- if (cond ? modrm.mod == 3 && !(sizeflag & SUFFIX_ALWAYS)
+ if (cond ? (need_modrm && modrm.mod == 3) && !(sizeflag & SUFFIX_ALWAYS)
: address_mode != mode_64bit)
break;
if ((rex & REX_W))
--
2.21.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: Fix SYSRETQ disassembly
2020-10-04 21:00 [PATCH] x86: Fix SYSRETQ disassembly Borislav Petkov
@ 2020-10-04 21:05 ` H.J. Lu
2020-10-04 21:49 ` Borislav Petkov
0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu @ 2020-10-04 21:05 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Binutils
On Sun, Oct 4, 2020 at 2:01 PM Borislav Petkov <bp@suse.de> wrote:
>
> Disassembling this:
>
> int main(void)
> {
>
> asm volatile("sysretq");
> return 0;
> }
>
> gives:
>
> 1129: 48 0f 07 rex.W sysret
>
> because the break condition in putop() which handles a %LQ (the template
> is "sysret%LQ") looks at ModRM.mod before verifying first that the
> instruction actually even has a ModRM byte and SYSRET doesn't, for
> example.
>
> Check need_modrm first before accessing a stale modrm value.
Which version of binutils are you using? Binutils master branch gave:
Disassembly of section .text.startup:
0000000000000000 <main>:
0: 48 0f 07 sysretq
3: 31 c0 xor %eax,%eax
5: c3 ret
--
H.J.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: Fix SYSRETQ disassembly
2020-10-04 21:05 ` H.J. Lu
@ 2020-10-04 21:49 ` Borislav Petkov
2020-10-04 21:52 ` H.J. Lu
0 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2020-10-04 21:49 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils
On Sun, Oct 04, 2020 at 02:05:53PM -0700, H.J. Lu wrote:
> Which version of binutils are you using? Binutils master branch gave:
I have at the tip of the master branch here:
commit 37df08e2fe25511182f25108384986903ce3aec1 (refs/remotes/origin/master, refs/remotes/origin/HEAD, refs/heads/master)
Author: GDB Administrator <gdbadmin@sourceware.org>
Date: Sun Oct 4 00:00:07 2020 +0000
Automatic date update in version.in
and I pull from:
git://sourceware.org/git/binutils-gdb.git
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: Fix SYSRETQ disassembly
2020-10-04 21:49 ` Borislav Petkov
@ 2020-10-04 21:52 ` H.J. Lu
2020-10-04 21:57 ` Borislav Petkov
0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu @ 2020-10-04 21:52 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Binutils
On Sun, Oct 4, 2020 at 2:50 PM Borislav Petkov <bp@suse.de> wrote:
>
> On Sun, Oct 04, 2020 at 02:05:53PM -0700, H.J. Lu wrote:
> > Which version of binutils are you using? Binutils master branch gave:
>
> I have at the tip of the master branch here:
>
> commit 37df08e2fe25511182f25108384986903ce3aec1 (refs/remotes/origin/master, refs/remotes/origin/HEAD, refs/heads/master)
> Author: GDB Administrator <gdbadmin@sourceware.org>
> Date: Sun Oct 4 00:00:07 2020 +0000
>
> Automatic date update in version.in
>
> and I pull from:
>
> git://sourceware.org/git/binutils-gdb.git
I can't reproduce it with your testcase. Please show me how to reproduce it.
--
H.J.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: Fix SYSRETQ disassembly
2020-10-04 21:52 ` H.J. Lu
@ 2020-10-04 21:57 ` Borislav Petkov
2020-10-04 22:34 ` H.J. Lu
0 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2020-10-04 21:57 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils
On Sun, Oct 04, 2020 at 02:52:39PM -0700, H.J. Lu wrote:
> I can't reproduce it with your testcase. Please show me how to reproduce it.
[boris@zn: /home/share/src/binutils/build> cat /tmp/sysretq.c
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main(void)
{
asm volatile("sysretq");
return 0;
}
[boris@zn: /home/share/src/binutils/build> gcc -Wall -o /tmp/sysretq{,.c}
[boris@zn: /home/share/src/binutils/build> ./binutils/objdump -d /tmp/sysretq | grep -A 10 "<main>:"
0000000000001125 <main>:
1125: 55 push %rbp
1126: 48 89 e5 mov %rsp,%rbp
1129: 48 0f 07 rex.W sysret
112c: b8 00 00 00 00 mov $0x0,%eax
1131: 5d pop %rbp
1132: c3 ret
1133: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
113a: 00 00 00
113d: 0f 1f 00 nopl (%rax)
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: Fix SYSRETQ disassembly
2020-10-04 21:57 ` Borislav Petkov
@ 2020-10-04 22:34 ` H.J. Lu
2020-10-04 23:18 ` [PATCH] x86: Clear modrm if not needed H.J. Lu
0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu @ 2020-10-04 22:34 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Binutils
On Sun, Oct 4, 2020 at 2:57 PM Borislav Petkov <bp@suse.de> wrote:
>
> On Sun, Oct 04, 2020 at 02:52:39PM -0700, H.J. Lu wrote:
> > I can't reproduce it with your testcase. Please show me how to reproduce it.
>
> [boris@zn: /home/share/src/binutils/build> cat /tmp/sysretq.c
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> int main(void)
> {
>
> asm volatile("sysretq");
>
> return 0;
> }
>
> [boris@zn: /home/share/src/binutils/build> gcc -Wall -o /tmp/sysretq{,.c}
> [boris@zn: /home/share/src/binutils/build> ./binutils/objdump -d /tmp/sysretq | grep -A 10 "<main>:"
> 0000000000001125 <main>:
> 1125: 55 push %rbp
> 1126: 48 89 e5 mov %rsp,%rbp
> 1129: 48 0f 07 rex.W sysret
> 112c: b8 00 00 00 00 mov $0x0,%eax
> 1131: 5d pop %rbp
> 1132: c3 ret
> 1133: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
> 113a: 00 00 00
> 113d: 0f 1f 00 nopl (%rax)
>
I opened:
https://sourceware.org/bugzilla/show_bug.cgi?id=26705
--
H.J.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] x86: Clear modrm if not needed
2020-10-04 22:34 ` H.J. Lu
@ 2020-10-04 23:18 ` H.J. Lu
2020-10-05 8:29 ` Borislav Petkov
0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu @ 2020-10-04 23:18 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Binutils
[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]
On Sun, Oct 4, 2020 at 3:34 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Sun, Oct 4, 2020 at 2:57 PM Borislav Petkov <bp@suse.de> wrote:
> >
> > On Sun, Oct 04, 2020 at 02:52:39PM -0700, H.J. Lu wrote:
> > > I can't reproduce it with your testcase. Please show me how to reproduce it.
> >
> > [boris@zn: /home/share/src/binutils/build> cat /tmp/sysretq.c
> > #include <errno.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> >
> > int main(void)
> > {
> >
> > asm volatile("sysretq");
> >
> > return 0;
> > }
> >
> > [boris@zn: /home/share/src/binutils/build> gcc -Wall -o /tmp/sysretq{,.c}
> > [boris@zn: /home/share/src/binutils/build> ./binutils/objdump -d /tmp/sysretq | grep -A 10 "<main>:"
> > 0000000000001125 <main>:
> > 1125: 55 push %rbp
> > 1126: 48 89 e5 mov %rsp,%rbp
> > 1129: 48 0f 07 rex.W sysret
> > 112c: b8 00 00 00 00 mov $0x0,%eax
> > 1131: 5d pop %rbp
> > 1132: c3 ret
> > 1133: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
> > 113a: 00 00 00
> > 113d: 0f 1f 00 nopl (%rax)
> >
>
> I opened:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26705
>
I am testing this patch.
--
H.J.
[-- Attachment #2: 0001-x86-Clear-modrm-if-not-needed.patch --]
[-- Type: text/x-patch, Size: 4844 bytes --]
From 9c85297a6bf95ce555f9a607b39bfcdf4c21f380 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 4 Oct 2020 16:06:26 -0700
Subject: [PATCH] x86: Clear modrm if not needed
The MODRM byte can be checked to display the instruction name only if the
MODRM byte needed. Clear modrm if the MODRM byte isn't needed so that
modrm field checks in putop like, modrm.mod == N with N != 0, can be done
without checking need_modrm.
gas/
PR binutils/26705
* testsuite/gas/i386/x86-64-suffix.s: Add "mov %rsp,%rbp" before
sysretq.
* testsuite/gas/i386/x86-64-suffix-intel.d: Updated.
* testsuite/gas/i386/x86-64-suffix.d: Likewise.
opcodes/
PR binutils/26705
* i386-dis.c (print_insn): Clear modrm if not needed.
(putop): Check need_modrm for modrm.mod != 3. Don't check
need_modrm for modrm.mod == 3.
---
gas/testsuite/gas/i386/x86-64-suffix-intel.d | 2 ++
gas/testsuite/gas/i386/x86-64-suffix.d | 2 ++
gas/testsuite/gas/i386/x86-64-suffix.s | 2 ++
opcodes/i386-dis.c | 12 ++++++++----
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/gas/testsuite/gas/i386/x86-64-suffix-intel.d b/gas/testsuite/gas/i386/x86-64-suffix-intel.d
index 74c8eaa4747..55d4a8dd1b6 100644
--- a/gas/testsuite/gas/i386/x86-64-suffix-intel.d
+++ b/gas/testsuite/gas/i386/x86-64-suffix-intel.d
@@ -18,11 +18,13 @@ Disassembly of section .text:
[ ]*[a-f0-9]+: cf iretd
[ ]*[a-f0-9]+: 48 cf iretq
[ ]*[a-f0-9]+: 0f 07 sysretd
+[ ]*[a-f0-9]+: 48 89 e5 mov rbp,rsp
[ ]*[a-f0-9]+: 48 0f 07 sysretq
[ ]*[a-f0-9]+: 66 cf iretw
[ ]*[a-f0-9]+: cf iretd
[ ]*[a-f0-9]+: cf iretd
[ ]*[a-f0-9]+: 48 cf iretq
[ ]*[a-f0-9]+: 0f 07 sysretd
+[ ]*[a-f0-9]+: 48 89 e5 mov rbp,rsp
[ ]*[a-f0-9]+: 48 0f 07 sysretq
#pass
diff --git a/gas/testsuite/gas/i386/x86-64-suffix.d b/gas/testsuite/gas/i386/x86-64-suffix.d
index d81ac4526bf..5ae11730b63 100644
--- a/gas/testsuite/gas/i386/x86-64-suffix.d
+++ b/gas/testsuite/gas/i386/x86-64-suffix.d
@@ -17,11 +17,13 @@ Disassembly of section .text:
[ ]*[a-f0-9]+: cf iretl
[ ]*[a-f0-9]+: 48 cf iretq
[ ]*[a-f0-9]+: 0f 07 sysretl
+[ ]*[a-f0-9]+: 48 89 e5 movq %rsp,%rbp
[ ]*[a-f0-9]+: 48 0f 07 sysretq
[ ]*[a-f0-9]+: 66 cf iretw
[ ]*[a-f0-9]+: cf iretl
[ ]*[a-f0-9]+: cf iretl
[ ]*[a-f0-9]+: 48 cf iretq
[ ]*[a-f0-9]+: 0f 07 sysretl
+[ ]*[a-f0-9]+: 48 89 e5 movq %rsp,%rbp
[ ]*[a-f0-9]+: 48 0f 07 sysretq
#pass
diff --git a/gas/testsuite/gas/i386/x86-64-suffix.s b/gas/testsuite/gas/i386/x86-64-suffix.s
index 2261c2c1aee..a226836215c 100644
--- a/gas/testsuite/gas/i386/x86-64-suffix.s
+++ b/gas/testsuite/gas/i386/x86-64-suffix.s
@@ -14,6 +14,7 @@ foo:
iretl
iretq
sysretl
+ mov %rsp,%rbp
sysretq
.intel_syntax noprefix
@@ -22,4 +23,5 @@ foo:
iret
iretq
sysretd
+ mov rbp,rsp
sysretq
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 67d5b6aaefc..f4f35bc8f1a 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -9927,7 +9927,7 @@ print_insn (bfd_vma pc, disassemble_info *info)
FETCH_DATA (info, codep + 1);
threebyte = *codep;
dp = &dis386_twobyte[threebyte];
- need_modrm = twobyte_has_modrm[*codep];
+ need_modrm = twobyte_has_modrm[threebyte];
codep++;
}
else
@@ -9954,6 +9954,8 @@ print_insn (bfd_vma pc, disassemble_info *info)
modrm.reg = (*codep >> 3) & 7;
modrm.rm = *codep & 7;
}
+ else
+ memset (&modrm, 0, sizeof (modrm));
need_vex = 0;
memset (&vex, 0, sizeof (vex));
@@ -10644,7 +10646,8 @@ putop (const char *in_template, int sizeflag)
case 'A':
if (intel_syntax)
break;
- if (modrm.mod != 3 || (sizeflag & SUFFIX_ALWAYS))
+ if ((need_modrm && modrm.mod != 3)
+ || (sizeflag & SUFFIX_ALWAYS))
*obufp++ = 'b';
break;
case 'B':
@@ -10796,7 +10799,7 @@ putop (const char *in_template, int sizeflag)
case 'P':
if (l == 0)
{
- if (((need_modrm && modrm.mod == 3) || !cond)
+ if ((modrm.mod == 3 || !cond)
&& !(sizeflag & SUFFIX_ALWAYS))
break;
/* Fall through. */
@@ -10840,7 +10843,8 @@ putop (const char *in_template, int sizeflag)
if (intel_syntax && !alt)
break;
USED_REX (REX_W);
- if (modrm.mod != 3 || (sizeflag & SUFFIX_ALWAYS))
+ if ((need_modrm && modrm.mod != 3)
+ || (sizeflag & SUFFIX_ALWAYS))
{
if (rex & REX_W)
*obufp++ = 'q';
--
2.26.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: Clear modrm if not needed
2020-10-04 23:18 ` [PATCH] x86: Clear modrm if not needed H.J. Lu
@ 2020-10-05 8:29 ` Borislav Petkov
0 siblings, 0 replies; 8+ messages in thread
From: Borislav Petkov @ 2020-10-05 8:29 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils
On Sun, Oct 04, 2020 at 04:18:24PM -0700, H.J. Lu wrote:
> I am testing this patch.
>
> --
> H.J.
> From 9c85297a6bf95ce555f9a607b39bfcdf4c21f380 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Sun, 4 Oct 2020 16:06:26 -0700
> Subject: [PATCH] x86: Clear modrm if not needed
>
> The MODRM byte can be checked to display the instruction name only if the
> MODRM byte needed. Clear modrm if the MODRM byte isn't needed so that
> modrm field checks in putop like, modrm.mod == N with N != 0, can be done
> without checking need_modrm.
This looks like the better solution.
Thx.
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-10-05 8:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-04 21:00 [PATCH] x86: Fix SYSRETQ disassembly Borislav Petkov
2020-10-04 21:05 ` H.J. Lu
2020-10-04 21:49 ` Borislav Petkov
2020-10-04 21:52 ` H.J. Lu
2020-10-04 21:57 ` Borislav Petkov
2020-10-04 22:34 ` H.J. Lu
2020-10-04 23:18 ` [PATCH] x86: Clear modrm if not needed H.J. Lu
2020-10-05 8:29 ` Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).