Update: [PATCH 5/X] libsanitizer: mid-end: Introduce stack variable handling for HWASAN

[Richard Sandiford]

Matthew Malcomson <matthew.malcomson@arm.com> writes:
> […]
> +/* hwasan_frame_base_init_seq is the sequence of RTL insns that will initialize
> +   the hwasan_frame_base_ptr.  When the hwasan_frame_base_ptr is requested, we
> +   generate this sequence but do not emit it.  If the sequence was created it
> +   is emitted once the function body has been expanded.
> +
> +   This delay is because the frame base pointer may be needed anywhere in the
> +   function body, or needed by the expand_used_vars function.  Emitting once in
> +   a known place is simpler than requiring the emition of the instructions to

s/emition/emission/

> +   be know where it should go depending on the first place the hwasan frame
> +   base is needed.  */
> +static GTY(()) rtx_insn *hwasan_frame_base_init_seq = NULL;
> […]
> +/* For stack tagging:
> +
> +   Return the 'base pointer' for this function.  If that base pointer has not
> +   yet been created then we create a register to hold it and record the insns
> +   to initialize the register in `hwasan_frame_base_init_seq` for later
> +   emission.  */
> +rtx
> +hwasan_frame_base ()
> +{
> +  if (! hwasan_frame_base_ptr)
> +    {
> +      start_sequence ();
> +      hwasan_frame_base_ptr =
> +	force_reg (Pmode,
> +		   targetm.memtag.insert_random_tag (virtual_stack_vars_rtx,
> +						     NULL_RTX));

Nit: should be formatted as:

      hwasan_frame_base_ptr
	= force_reg (Pmode,
		     targetm.memtag.insert_random_tag (virtual_stack_vars_rtx,
						       NULL_RTX));

> […]
> +  size_t length = hwasan_tagged_stack_vars.length ();
> +  hwasan_stack_var *vars = hwasan_tagged_stack_vars.address ();
> +
> +  poly_int64 bot = 0, top = 0;
> +  size_t i = 0;
> +  for (i = 0; i < length; i++)
> +    {
> +      hwasan_stack_var& cur = vars[i];

Simpler as:

  poly_int64 bot = 0, top = 0;
  for (hwasan_stack_var &cur : hwasan_tagged_stack_vars)

(GCC style is to add a space before “&”, as for “*”)

> +      poly_int64 nearest = cur.nearest_offset;
> +      poly_int64 farthest = cur.farthest_offset;
> +
> +      if (known_ge (nearest, farthest))
> +	{
> +	  top = nearest;
> +	  bot = farthest;
> +	}
> +      else
> +	{
> +	  /* Given how these values are calculated, one must be known greater
> +	     than the other.  */
> +	  gcc_assert (known_le (nearest, farthest));
> +	  top = farthest;
> +	  bot = nearest;
> +	}
> +      poly_int64 size = (top - bot);
> +
> +      /* Assert the edge of each variable is aligned to the HWASAN tag granule
> +	 size.  */
> +      gcc_assert (multiple_p (top, HWASAN_TAG_GRANULE_SIZE));
> +      gcc_assert (multiple_p (bot, HWASAN_TAG_GRANULE_SIZE));
> +      gcc_assert (multiple_p (size, HWASAN_TAG_GRANULE_SIZE));
> +
> +      rtx ret = init_one_libfunc ("__hwasan_tag_memory");
> +      rtx base_tag = targetm.memtag.extract_tag (cur.tagged_base, NULL_RTX);
> +      rtx tag = plus_constant (QImode, base_tag, cur.tag_offset);
> +      tag = hwasan_truncate_to_tag_size (tag, NULL_RTX);
> +
> +      rtx bottom = convert_memory_address (ptr_mode,
> +					   plus_constant (Pmode,
> +							  cur.untagged_base,
> +							  bot));
> +      emit_library_call (ret, LCT_NORMAL, VOIDmode,
> +			 bottom, ptr_mode,
> +			 tag, QImode,
> +			 gen_int_mode (size, ptr_mode), ptr_mode);
> +    }
> +  /* Clear the stack vars, we've emitted the prologue for them all now.  */
> +  hwasan_tagged_stack_vars.truncate (0);
> +}
> +
> +/* For stack tagging:
> +
> +   Return RTL insns to clear the tags between DYNAMIC and VARS pointers
> +   into the stack.  These instructions should be emitted at the end of
> +   every function.
> +
> +   If `dynamic` is NULL_RTX then no insns are returned.  */
> +rtx_insn *
> +hwasan_emit_untag_frame (rtx dynamic, rtx vars)
> +{
> +  if (! dynamic)
> +    return NULL;
> +
> +  start_sequence ();
> +
> +  dynamic = convert_memory_address (ptr_mode, dynamic);
> +  vars = convert_memory_address (ptr_mode, vars);
> +
> +  rtx top_rtx;
> +  rtx bot_rtx;
> +  if (FRAME_GROWS_DOWNWARD)
> +    {
> +      top_rtx = vars;
> +      bot_rtx = dynamic;
> +    }
> +  else
> +    {
> +      top_rtx = dynamic;
> +      bot_rtx = vars;
> +    }
> +
> +  rtx size_rtx = expand_simple_binop (ptr_mode, MINUS, top_rtx, bot_rtx,
> +				      NULL_RTX, /* unsignedp = */0,
> +				      OPTAB_DIRECT);
> +
> +  rtx ret = init_one_libfunc ("__hwasan_tag_memory");
> +  emit_library_call (ret, LCT_NORMAL, VOIDmode,
> +		     bot_rtx, ptr_mode,
> +		     HWASAN_STACK_BACKGROUND, QImode,
> +		     size_rtx, ptr_mode);

Nit: “ret” seems like a strange name for this variable, since it implies
that it's a return value of the library call.  Maybe “fn” or something
would be better.

> […]
> @@ -1216,6 +1255,24 @@ expand_stack_vars (bool (*pred) (size_t), class stack_vars_data *data)
>  	    {
>  	      offset = alloc_stack_frame_space (stack_vars[i].size, alignb);
>  	      base_align = crtl->max_used_stack_slot_alignment;
> +
> +	      if (hwasan_sanitize_stack_p ())
> +		{
> +		  /* Align again since the point of this alignment is to handle
> +		     the "end" of the object (i.e. smallest address after the
> +		     stack object).  For FRAME_GROWS_DOWNWARD that requires
> +		     aligning the stack before allocating, but for a frame that
> +		     grows upwards that requires aligning the stack after
> +		     allocation.
> +
> +		     Use `frame_offset` to record the offset value rather than
> +		     offset since the frame_offset describes the extent

Would be easier to parse if the second “offset” was in quotes too.

> +		     allocated for this particular variable while `offset`
> +		     describes the address that this variable starts at.  */
> +		  align_frame_offset (HWASAN_TAG_GRANULE_SIZE);
> +		  hwasan_record_stack_var (virtual_stack_vars_rtx, base,
> +					   hwasan_orig_offset, frame_offset);
> +		}
>  	    }
>  	}
>        else
> […]
> +/* The default implementation of TARGET_MEMTAG_SET_TAG.  */
> +rtx
> +default_memtag_set_tag (rtx untagged, rtx tag, rtx target)
> +{
> +  gcc_assert (GET_MODE (untagged) == Pmode);
> +  gcc_assert (GET_MODE (tag) == QImode);

Nit: I think the general preference is to have a single gcc_assert that
combines both conditions, for code size reasons.

> +  tag = expand_simple_binop (Pmode, ASHIFT, tag, HWASAN_SHIFT_RTX, tag,
> +			     /* unsignedp = */1, OPTAB_WIDEN);

This should pass NULL_RTX as the target instead of “tag”, since we can't
guarantee that “tag” is overwritable.  (In practice “tag” would probably
never get chosen as the target anyway since it has the wrong mode.)

OK with those changes, thanks.

Richard

> +  rtx ret = expand_simple_binop (Pmode, IOR, untagged, tag, target,
> +				 /* unsignedp = */1, OPTAB_DIRECT);
> +  gcc_assert (ret);
> +  return ret;
> +}
> […]