strange segfault i386-dis.c:9815:28

strange segfault i386-dis.c:9815:28

[Alan Modra]

Hi Jan, H.J.,
I'm seeing some really weird oss-fuzz reports of segfaults in
i386-dis.c.
https://oss-fuzz.com/testcase-detail/5870018505342976
https://oss-fuzz.com/testcase-detail/4651718416924672
https://oss-fuzz.com/testcase-detail/5452642448179200

I can't reproduce them locally, and don't have access to the binaries
to see exactly what is going on.  The problem may well turn out to be
a clang bug, but then there are these notes from the setjmp man page:

"Consequently, the values of automatic variables are unspecified after
a call to longjmp() if they meet all the following criteria:
  • they are local to the function that made the corresponding setjmp() call;
  • their values are changed between the calls to setjmp() and longjmp(); and
  • they are not declared as volatile."

Jan's commit 384e201e5aec made "ins" an auto var.  "priv" was already
an auto var.  It might be possible that one or more of the "ins" or
"priv" fields are living in non-volatile regs and thus have stale
values after the longjmp.  To cover that possibility, how about the
following patch?

	* i386-dis.c (print_insn): Access "ins" and "priv" via volatile
	pointers after second sigsetjmp return.

diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index a414e8c9b1e..9684dcda746 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -9808,12 +9808,17 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       /* Getting here means we tried for data but didn't get it.  That
 	 means we have an incomplete instruction of some sort.  Just
 	 print the first byte as a prefix or a .byte pseudo-op.  */
-      if (ins.codep > priv.the_buffer)
+      volatile struct dis_private *ppriv = &priv;
+      volatile instr_info *pins = &ins;
+      if (pins->codep > ppriv->the_buffer)
 	{
 	  const char *name = NULL;
 
-	  if (ins.prefixes || ins.fwait_prefix >= 0 || (ins.rex & REX_OPCODE))
-	    name = prefix_name (&ins, priv.the_buffer[0], priv.orig_sizeflag);
+	  if (pins->prefixes
+	      || pins->fwait_prefix >= 0
+	      || (pins->rex & REX_OPCODE))
+	    name = prefix_name (&ins, ppriv->the_buffer[0],
+				ppriv->orig_sizeflag);
 	  if (name != NULL)
 	    i386_dis_printf (&ins, dis_style_mnemonic, "%s", name);
 	  else
@@ -9822,7 +9827,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	      i386_dis_printf (&ins, dis_style_assembler_directive,
 			       ".byte ");
 	      i386_dis_printf (&ins, dis_style_immediate, "0x%x",
-			       (unsigned int) priv.the_buffer[0]);
+			       (unsigned int) ppriv->the_buffer[0]);
 	    }
 
 	  return 1;

-- 
Alan Modra
Australia Development Lab, IBM

Re: strange segfault i386-dis.c:9815:28

[H.J. Lu]

On Fri, Mar 17, 2023 at 3:41 AM Alan Modra <amodra@gmail.com> wrote:
>
> Hi Jan, H.J.,
> I'm seeing some really weird oss-fuzz reports of segfaults in
> i386-dis.c.
> https://oss-fuzz.com/testcase-detail/5870018505342976
> https://oss-fuzz.com/testcase-detail/4651718416924672
> https://oss-fuzz.com/testcase-detail/5452642448179200
>
> I can't reproduce them locally, and don't have access to the binaries
> to see exactly what is going on.  The problem may well turn out to be
> a clang bug, but then there are these notes from the setjmp man page:
>
> "Consequently, the values of automatic variables are unspecified after
> a call to longjmp() if they meet all the following criteria:
>   • they are local to the function that made the corresponding setjmp() call;
>   • their values are changed between the calls to setjmp() and longjmp(); and
>   • they are not declared as volatile."
>
> Jan's commit 384e201e5aec made "ins" an auto var.  "priv" was already
> an auto var.  It might be possible that one or more of the "ins" or
> "priv" fields are living in non-volatile regs and thus have stale
> values after the longjmp.  To cover that possibility, how about the
> following patch?
>
>         * i386-dis.c (print_insn): Access "ins" and "priv" via volatile
>         pointers after second sigsetjmp return.
>
> diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
> index a414e8c9b1e..9684dcda746 100644
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -9808,12 +9808,17 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>        /* Getting here means we tried for data but didn't get it.  That
>          means we have an incomplete instruction of some sort.  Just
>          print the first byte as a prefix or a .byte pseudo-op.  */
> -      if (ins.codep > priv.the_buffer)
> +      volatile struct dis_private *ppriv = &priv;
> +      volatile instr_info *pins = &ins;
> +      if (pins->codep > ppriv->the_buffer)
>         {
>           const char *name = NULL;
>
> -         if (ins.prefixes || ins.fwait_prefix >= 0 || (ins.rex & REX_OPCODE))
> -           name = prefix_name (&ins, priv.the_buffer[0], priv.orig_sizeflag);
> +         if (pins->prefixes
> +             || pins->fwait_prefix >= 0
> +             || (pins->rex & REX_OPCODE))
> +           name = prefix_name (&ins, ppriv->the_buffer[0],
> +                               ppriv->orig_sizeflag);
>           if (name != NULL)
>             i386_dis_printf (&ins, dis_style_mnemonic, "%s", name);
>           else
> @@ -9822,7 +9827,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>               i386_dis_printf (&ins, dis_style_assembler_directive,
>                                ".byte ");
>               i386_dis_printf (&ins, dis_style_immediate, "0x%x",
> -                              (unsigned int) priv.the_buffer[0]);
> +                              (unsigned int) ppriv->the_buffer[0]);
>             }
>
>           return 1;
>
> --
> Alan Modra
> Australia Development Lab, IBM

Compilers should handle local variables with setjmp properly.
Without a testcase, it is hard to tell what really went wrong.

-- 
H.J.

Re: strange segfault i386-dis.c:9815:28

[Andreas Schwab]

On Mär 17 2023, H.J. Lu via Binutils wrote:

> Compilers should handle local variables with setjmp properly.

The local variable ins is modified between setjmp and longjmp which can
only work properly if it is declared volatile.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: strange segfault i386-dis.c:9815:28

[Alan Modra]

On Fri, Mar 17, 2023 at 08:56:25AM -0700, H.J. Lu wrote:
> Compilers should handle local variables with setjmp properly.
> Without a testcase, it is hard to tell what really went wrong.

gcc might do so, because it recognises setjmp as special.  Does clang?
What about other compilers?  I'm going to apply the patch I posted.
Making accesses to "ins" and "priv" go via volatile pointer in the
longjmp return path has minimal effect on performance.

-- 
Alan Modra
Australia Development Lab, IBM

Re: strange segfault i386-dis.c:9815:28

[Jan Beulich]

On 17.03.2023 11:41, Alan Modra wrote:
> Hi Jan, H.J.,
> I'm seeing some really weird oss-fuzz reports of segfaults in
> i386-dis.c.
> https://oss-fuzz.com/testcase-detail/5870018505342976
> https://oss-fuzz.com/testcase-detail/4651718416924672
> https://oss-fuzz.com/testcase-detail/5452642448179200
> 
> I can't reproduce them locally, and don't have access to the binaries
> to see exactly what is going on.  The problem may well turn out to be
> a clang bug, but then there are these notes from the setjmp man page:
> 
> "Consequently, the values of automatic variables are unspecified after
> a call to longjmp() if they meet all the following criteria:
>   • they are local to the function that made the corresponding setjmp() call;
>   • their values are changed between the calls to setjmp() and longjmp(); and
>   • they are not declared as volatile."
> 
> Jan's commit 384e201e5aec made "ins" an auto var.

I don't think this particularly matters - it was an auto var in each of
the callers before that, and by (partial) inlining (full inlining would
have been very odd for a compiler to actually do) the same issue could
surface. I'm not sure whether even without inlining a compiler couldn't
derive enough information to decide to hold certain fields in call-
clobbered registers, as enough of the invoked functions are static.

>  "priv" was already
> an auto var.  It might be possible that one or more of the "ins" or
> "priv" fields are living in non-volatile regs and thus have stale
> values after the longjmp.  To cover that possibility, how about the
> following patch?
> 
> 	* i386-dis.c (print_insn): Access "ins" and "priv" via volatile
> 	pointers after second sigsetjmp return.

Does this actually go far enough? Functions called may be inlined, and
hence further accesses of struct fields held in registers may occur.

> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -9808,12 +9808,17 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>        /* Getting here means we tried for data but didn't get it.  That
>  	 means we have an incomplete instruction of some sort.  Just
>  	 print the first byte as a prefix or a .byte pseudo-op.  */
> -      if (ins.codep > priv.the_buffer)
> +      volatile struct dis_private *ppriv = &priv;
> +      volatile instr_info *pins = &ins;
> +      if (pins->codep > ppriv->the_buffer)
>  	{
>  	  const char *name = NULL;
>  
> -	  if (ins.prefixes || ins.fwait_prefix >= 0 || (ins.rex & REX_OPCODE))
> -	    name = prefix_name (&ins, priv.the_buffer[0], priv.orig_sizeflag);
> +	  if (pins->prefixes
> +	      || pins->fwait_prefix >= 0
> +	      || (pins->rex & REX_OPCODE))
> +	    name = prefix_name (&ins, ppriv->the_buffer[0],
> +				ppriv->orig_sizeflag);

I guess passing &ins here can be easily avoided, as the function only
looks at address_mode, so we could pass that instead. But for ...

>  	  if (name != NULL)
>  	    i386_dis_printf (&ins, dis_style_mnemonic, "%s", name);
>  	  else
> @@ -9822,7 +9827,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  	      i386_dis_printf (&ins, dis_style_assembler_directive,
>  			       ".byte ");
>  	      i386_dis_printf (&ins, dis_style_immediate, "0x%x",
> -			       (unsigned int) priv.the_buffer[0]);
> +			       (unsigned int) ppriv->the_buffer[0]);
>  	    }

... all the uses of i386_dis_printf() I think its first parameter
would now also need to become pointer-to-volatile (and pins be passed).

Overall I think though that the use of setjmp/longjmp here would better
be avoided. But that'll be something for later.

Jan

Re: strange segfault i386-dis.c:9815:28

[Alan Modra]

On Mon, Mar 20, 2023 at 08:29:44AM +0100, Jan Beulich wrote:
> Does this actually go far enough? Functions called may be inlined, and
> hence further accesses of struct fields held in registers may occur.

I think we are OK.  The fields accessed are ins.address_mode and
ins.info, which are set up early in print_insn and read-only past the
setjmp.

-- 
Alan Modra
Australia Development Lab, IBM

Re: strange segfault i386-dis.c:9815:28

[Alan Modra]

On Mon, Mar 20, 2023 at 06:27:20PM +1030, Alan Modra wrote:
> On Mon, Mar 20, 2023 at 08:29:44AM +0100, Jan Beulich wrote:
> > Does this actually go far enough? Functions called may be inlined, and
> > hence further accesses of struct fields held in registers may occur.
> 
> I think we are OK.  The fields accessed are ins.address_mode and
> ins.info, which are set up early in print_insn and read-only past the
> setjmp.

Perhaps you are correct that my change doesn't go far enough for a
different reason.  Accessing these local var structs using a volatile
qualified pointer may indeed read the object, but I don't think
changed values are guaranteed to be written back to the object unless
the actual object is declared volatile.

I'm going to revert my change.  It didn't cure the oss-fuzz testcase
fails.

-- 
Alan Modra
Australia Development Lab, IBM

Re: strange segfault i386-dis.c:9815:28

[Jan Beulich]

On 20.03.2023 11:26, Alan Modra wrote:
> On Mon, Mar 20, 2023 at 06:27:20PM +1030, Alan Modra wrote:
>> On Mon, Mar 20, 2023 at 08:29:44AM +0100, Jan Beulich wrote:
>>> Does this actually go far enough? Functions called may be inlined, and
>>> hence further accesses of struct fields held in registers may occur.
>>
>> I think we are OK.  The fields accessed are ins.address_mode and
>> ins.info, which are set up early in print_insn and read-only past the
>> setjmp.
> 
> Perhaps you are correct that my change doesn't go far enough for a
> different reason.  Accessing these local var structs using a volatile
> qualified pointer may indeed read the object, but I don't think
> changed values are guaranteed to be written back to the object unless
> the actual object is declared volatile.

I was indeed wondering about that as well, but ...

> I'm going to revert my change.  It didn't cure the oss-fuzz testcase
> fails.

... had assumed you already knew it helped. Now that we know it doesn't,
maybe we indeed want to look into getting rid of this setjmp/longjmp
use.

Jan

Re: strange segfault i386-dis.c:9815:28

[Alan Modra]

On Mon, Mar 20, 2023 at 11:41:08AM +0100, Jan Beulich wrote:
> On 20.03.2023 11:26, Alan Modra wrote:
> > On Mon, Mar 20, 2023 at 06:27:20PM +1030, Alan Modra wrote:
> >> On Mon, Mar 20, 2023 at 08:29:44AM +0100, Jan Beulich wrote:
> >>> Does this actually go far enough? Functions called may be inlined, and
> >>> hence further accesses of struct fields held in registers may occur.
> >>
> >> I think we are OK.  The fields accessed are ins.address_mode and
> >> ins.info, which are set up early in print_insn and read-only past the
> >> setjmp.
> > 
> > Perhaps you are correct that my change doesn't go far enough for a
> > different reason.  Accessing these local var structs using a volatile
> > qualified pointer may indeed read the object, but I don't think
> > changed values are guaranteed to be written back to the object unless
> > the actual object is declared volatile.
> 
> I was indeed wondering about that as well, but ...
> 
> > I'm going to revert my change.  It didn't cure the oss-fuzz testcase
> > fails.
> 
> ... had assumed you already knew it helped. Now that we know it doesn't,
> maybe we indeed want to look into getting rid of this setjmp/longjmp
> use.

I finally managed to reproduce one of the oss-fuzz failures locally,
by following the recipe at
https://google.github.io/oss-fuzz/advanced-topics/reproducing/

It looks to be a bug in clang-15 -fsanitize-coverage.  The following
code produced for "ins.fwait_prefix >= 0" segfaults at 6666fc due to
loading a bogus address from 0x78(%rbx).

  6666f8:       48 8b 43 78             mov    0x78(%rbx),%rax
  6666fc:       44 0f b6 30             movzbl (%rax),%r14d
  666700:       bf ff 00 00 00          mov    $0xff,%edi
  666705:       44 89 f6                mov    %r14d,%esi
  666708:       e8 d3 9e e7 ff          call   4e05e0 <__sanitizer_cov_trace_const_cmp1>
  66670d:       45 84 f6                test   %r14b,%r14b
  666710:       0f 88 10 05 00 00       js     666c26 <print_insn+0x28e6>

I don't know enough about clang to say what is going on here, but
presumably the code expects 0x78(%rbx) to contain the address of
ins.fwait_prefix.  Making ins.fwait_prefix volatile doesn't help.
So for now I'm just going to ignore this issue (and what I've found so
far wouldn't make a very good clang bug report).

-- 
Alan Modra
Australia Development Lab, IBM

Re: strange segfault i386-dis.c:9815:28

[Jan Beulich]

On 23.03.2023 13:59, Alan Modra wrote:
> On Mon, Mar 20, 2023 at 11:41:08AM +0100, Jan Beulich wrote:
>> On 20.03.2023 11:26, Alan Modra wrote:
>>> On Mon, Mar 20, 2023 at 06:27:20PM +1030, Alan Modra wrote:
>>>> On Mon, Mar 20, 2023 at 08:29:44AM +0100, Jan Beulich wrote:
>>>>> Does this actually go far enough? Functions called may be inlined, and
>>>>> hence further accesses of struct fields held in registers may occur.
>>>>
>>>> I think we are OK.  The fields accessed are ins.address_mode and
>>>> ins.info, which are set up early in print_insn and read-only past the
>>>> setjmp.
>>>
>>> Perhaps you are correct that my change doesn't go far enough for a
>>> different reason.  Accessing these local var structs using a volatile
>>> qualified pointer may indeed read the object, but I don't think
>>> changed values are guaranteed to be written back to the object unless
>>> the actual object is declared volatile.
>>
>> I was indeed wondering about that as well, but ...
>>
>>> I'm going to revert my change.  It didn't cure the oss-fuzz testcase
>>> fails.
>>
>> ... had assumed you already knew it helped. Now that we know it doesn't,
>> maybe we indeed want to look into getting rid of this setjmp/longjmp
>> use.
> 
> I finally managed to reproduce one of the oss-fuzz failures locally,
> by following the recipe at
> https://google.github.io/oss-fuzz/advanced-topics/reproducing/
> 
> It looks to be a bug in clang-15 -fsanitize-coverage.  The following
> code produced for "ins.fwait_prefix >= 0" segfaults at 6666fc due to
> loading a bogus address from 0x78(%rbx).
> 
>   6666f8:       48 8b 43 78             mov    0x78(%rbx),%rax
>   6666fc:       44 0f b6 30             movzbl (%rax),%r14d
>   666700:       bf ff 00 00 00          mov    $0xff,%edi
>   666705:       44 89 f6                mov    %r14d,%esi
>   666708:       e8 d3 9e e7 ff          call   4e05e0 <__sanitizer_cov_trace_const_cmp1>
>   66670d:       45 84 f6                test   %r14b,%r14b
>   666710:       0f 88 10 05 00 00       js     666c26 <print_insn+0x28e6>
> 
> I don't know enough about clang to say what is going on here, but
> presumably the code expects 0x78(%rbx) to contain the address of
> ins.fwait_prefix.  Making ins.fwait_prefix volatile doesn't help.
> So for now I'm just going to ignore this issue (and what I've found so
> far wouldn't make a very good clang bug report).

I hope this will all be taken care of by the first so many patches of
https://sourceware.org/pipermail/binutils/2023-April/126962.html.

Jan