Re: Fix use after free in cgen instruction lookup

[Stafford Horne <shorne at gmail dot com>]

On Sat, Feb 11, 2017 at 05:49:01PM +1030, Alan Modra wrote:
> On Thu, Feb 09, 2017 at 06:54:13AM +0900, Stafford Horne wrote:
> > The buf variable is used after it is free'd.  This causes the lookups to
> > fail and also causes memory corruption.
> > 
> > Re-arrange the code a bit to make sure we always free memory before
> > returning. This was caught in openrisc testing, one of the only user of
> > this method.
> > 
> > opcodes/ChangeLog:
> > 
> > 2017-02-09  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* cgen-opc.c (cgen_lookup_insn): Fix memory corruption issue.
> 
> Thanks for fixing this.  I'm going to apply a slightly different
> patch to remove the unnecessary temporaries as well.

Thanks,
I reviewed and tested the below and it looks good.

 $ or1k-linux-musl-gcc -static -g main.c
 $ qemu-or32 -g 10001 ./a.out

 $ or1k-linux-musl-gdb ./a.out \
    --eval-command='target remote localhost:10001'

 Remote debugging using localhost:10001
 0x000020e4 in _start ()
 (gdb) si
 0x000020e8 in _start ()
 (gdb) si                   << Used to SEGV here
 0x000020f0 in _start ()
 (gdb) si
 0x000020f4 in _start ()

-Stafford

> diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog
> index 8366d1b..fc6637b 100644
> --- a/opcodes/ChangeLog
> +++ b/opcodes/ChangeLog
> @@ -1,3 +1,10 @@
> +2017-02-11  Stafford Horne  <shorne@gmail.com>
> +	    Alan Modra  <amodra@gmail.com>
> +
> +	* cgen-opc.c (cgen_lookup_insn): Delete buf and base_insn temps.
> +	Use insn_bytes_value and insn_int_value directly instead.  Don't
> +	free allocated memory until function exit.
> +
>  2017-02-10  Nicholas Piggin  <npiggin@gmail.com>
>  
>  	* ppc-opc.c (powerpc_opcodes) <scv, rfscv>: New mnemonics.
> diff --git a/opcodes/cgen-opc.c b/opcodes/cgen-opc.c
> index 72b4f05..4299db3 100644
> --- a/opcodes/cgen-opc.c
> +++ b/opcodes/cgen-opc.c
> @@ -452,18 +452,14 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
>  		  CGEN_FIELDS *fields,
>  		  int alias_p)
>  {
> -  unsigned char *buf;
> -  CGEN_INSN_INT base_insn;
>    CGEN_EXTRACT_INFO ex_info;
>    CGEN_EXTRACT_INFO *info;
>  
>    if (cd->int_insn_p)
>      {
>        info = NULL;
> -      buf = (unsigned char *) xmalloc (cd->max_insn_bitsize / 8);
> -      cgen_put_insn_value (cd, buf, length, insn_int_value);
> -      base_insn = insn_int_value;
> -      free (buf);
> +      insn_bytes_value = (unsigned char *) xmalloc (cd->max_insn_bitsize / 8);
> +      cgen_put_insn_value (cd, insn_bytes_value, length, insn_int_value);
>      }
>    else
>      {
> @@ -471,8 +467,7 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
>        ex_info.dis_info = NULL;
>        ex_info.insn_bytes = insn_bytes_value;
>        ex_info.valid = -1;
> -      buf = insn_bytes_value;
> -      base_insn = cgen_get_insn_value (cd, buf, length);
> +      insn_int_value = cgen_get_insn_value (cd, insn_bytes_value, length);
>      }
>  
>    if (!insn)
> @@ -482,7 +477,8 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
>        /* The instructions are stored in hash lists.
>  	 Pick the first one and keep trying until we find the right one.  */
>  
> -      insn_list = cgen_dis_lookup_insn (cd, (char *) buf, base_insn);
> +      insn_list = cgen_dis_lookup_insn (cd, (char *) insn_bytes_value,
> +					insn_int_value);
>        while (insn_list != NULL)
>  	{
>  	  insn = insn_list->insn;
> @@ -494,18 +490,18 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
>  	      /* Basic bit mask must be correct.  */
>  	      /* ??? May wish to allow target to defer this check until the
>  		 extract handler.  */
> -	      if ((base_insn & CGEN_INSN_BASE_MASK (insn))
> +	      if ((insn_int_value & CGEN_INSN_BASE_MASK (insn))
>  		  == CGEN_INSN_BASE_VALUE (insn))
>  		{
>  		  /* ??? 0 is passed for `pc' */
>  		  int elength = CGEN_EXTRACT_FN (cd, insn)
> -		    (cd, insn, info, base_insn, fields, (bfd_vma) 0);
> +		    (cd, insn, info, insn_int_value, fields, (bfd_vma) 0);
>  		  if (elength > 0)
>  		    {
>  		      /* sanity check */
>  		      if (length != 0 && length != elength)
>  			abort ();
> -		      return insn;
> +		      break;
>  		    }
>  		}
>  	    }
> @@ -525,15 +521,17 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
>  
>        /* ??? 0 is passed for `pc' */
>        length = CGEN_EXTRACT_FN (cd, insn)
> -	(cd, insn, info, base_insn, fields, (bfd_vma) 0);
> +	(cd, insn, info, insn_int_value, fields, (bfd_vma) 0);
>        /* Sanity check: must succeed.
>  	 Could relax this later if it ever proves useful.  */
>        if (length == 0)
>  	abort ();
> -      return insn;
>      }
>  
> -  return NULL;
> +  if (cd->int_insn_p)
> +    free (insn_bytes_value);
> +
> +  return insn;
>  }
>  
>  /* Fill in the operand instances used by INSN whose operands are FIELDS.
> 
> 
> -- 
> Alan Modra
> Australia Development Lab, IBM