Update [PATCH 6/X] libsanitizer: Add hwasan pass and associated gimple changes

Richard Sandiford richard.sandiford@arm.com
Thu Nov 19 16:28:45 GMT 2020


Matthew Malcomson <matthew.malcomson@arm.com> writes:
> +/* Emit gimple statements into &stmts that take the size given in `len` and
> +   generate a size that is guaranteed to be rounded upwards to `align`.
> +
> +   This is a helper function for both handle_builtin_alloca and
> +   asan_expand_mark_ifn when using HWASAN.
> +
> +   Return the tree node representing this size, it is of TREE_TYPE
> +   size_type_node.  */
> +
> +static tree
> +hwasan_emit_round_up (gimple_seq *seq, location_t loc, tree old_size,
> +		      uint8_t align)
> +{
> +  uint8_t tg_mask = align - 1;
> +  /* tree new_size = (old_size + tg_mask) & ~tg_mask;  */
> +  tree tree_mask = build_int_cst (size_type_node, tg_mask);
> +  tree oversize = gimple_build (seq, loc, PLUS_EXPR, size_type_node, old_size,
> +				tree_mask);
> +
> +  tree mask = build_int_cst (size_type_node, -align);
> +  return gimple_build (seq, loc, BIT_AND_EXPR, size_type_node, oversize, mask);
> +}
> +

There's nothing really hwasan-specific about this, apart from the choice
“uint8_t” for the alignment and mask.  So I think we should:

- chnage “align” and “tg_mask” to “unsigned HOST_WIDE_INT”
- change the name to “gimple_build_round_up”
- take the type as a parameter, in the same position as other
  gimple_build_* type parameters
- move the function to gimple-fold.c, exported via gimple-fold.h
- drop:

   This is a helper function for both handle_builtin_alloca and
   asan_expand_mark_ifn when using HWASAN.

> […]
> @@ -690,6 +757,71 @@ handle_builtin_alloca (gcall *call, gimple_stmt_iterator *iter)
>      = DECL_FUNCTION_CODE (callee) == BUILT_IN_ALLOCA
>        ? 0 : tree_to_uhwi (gimple_call_arg (call, 1));
>  
> +  if (hwasan_sanitize_allocas_p ())
> +    {
> +      gimple_seq stmts = NULL;
> +      location_t loc = gimple_location (gsi_stmt (*iter));
> +      /*
> +	 HWASAN needs a different expansion.
> +
> +	 addr = __builtin_alloca (size, align);
> +
> +	 should be replaced by
> +
> +	 new_size = size rounded up to HWASAN_TAG_GRANULE_SIZE byte alignment;
> +	 untagged_addr = __builtin_alloca (new_size, align);
> +	 tag = __hwasan_choose_alloca_tag ();
> +	 addr = ifn_HWASAN_SET_TAG (untagged_addr, tag);
> +	 __hwasan_tag_memory (untagged_addr, tag, new_size);
> +	*/
> +      /* Ensure alignment at least HWASAN_TAG_GRANULE_SIZE bytes so we start on
> +	 a tag granule.  */
> +      align = align > HWASAN_TAG_GRANULE_SIZE ? align : HWASAN_TAG_GRANULE_SIZE;
> +
> +      tree old_size = gimple_call_arg (call, 0);
> +      tree new_size = hwasan_emit_round_up (&stmts, loc, old_size,
> +					    HWASAN_TAG_GRANULE_SIZE);
> +
> +      /* Make the alloca call */
> +      tree untagged_addr
> +	= gimple_build (&stmts, loc,
> +			as_combined_fn (BUILT_IN_ALLOCA_WITH_ALIGN), ptr_type,
> +			new_size, build_int_cst (size_type_node, align));
> +
> +      /* Choose the tag.
> +	 Here we use an internal function so we can choose the tag at expand
> +	 time.  We need the decision to be made after stack variables have been
> +	 assigned their tag (i.e. once the hwasan_frame_tag_offset variable has
> +	 been set to one after the last stack variables tag).  */
> +      gcall *stmt = gimple_build_call_internal (IFN_HWASAN_CHOOSE_TAG, 0);
> +      tree tag = make_ssa_name (unsigned_char_type_node);
> +      gimple_call_set_lhs (stmt, tag);
> +      gimple_set_location (stmt, loc);
> +      gimple_seq_add_stmt_without_update (&stmts, stmt);

Even though there are currently no folds defined for argumentless
functions, I think it would be worth adding a gimple_build overload
for this instead of writing it out by hand.  I.e. have a zero-argument
version of:

tree
gimple_build (gimple_seq *seq, location_t loc, combined_fn fn,
	      tree type, tree arg0)
{
  tree res = gimple_simplify (fn, type, arg0, seq, gimple_build_valueize);
  if (!res)
    {
      gcall *stmt;
      if (internal_fn_p (fn))
	stmt = gimple_build_call_internal (as_internal_fn (fn), 1, arg0);
      else
	{
	  tree decl = builtin_decl_implicit (as_builtin_fn (fn));
	  stmt = gimple_build_call (decl, 1, arg0);
	}
      if (!VOID_TYPE_P (type))
	{
	  res = create_tmp_reg_or_ssa_name (type);
	  gimple_call_set_lhs (stmt, res);
	}
      gimple_set_location (stmt, loc);
      gimple_seq_add_stmt_without_update (seq, stmt);
    }
  return res;
}

without the gimple_simplify call.

> +
> +      /* Add tag to pointer.  */
> +      tree addr
> +	= gimple_build (&stmts, loc, as_combined_fn (IFN_HWASAN_SET_TAG),

This is CFN_HWASAN_SET_TAG.

> +			ptr_type, untagged_addr, tag);
> +
> +      /* Tag shadow memory.
> +	 NOTE: require using `untagged_addr` here for libhwasan API.  */
> +      gimple_build (&stmts, loc, as_combined_fn (BUILT_IN_HWASAN_TAG_MEM),
> +		    void_type_node, untagged_addr, tag, new_size);
> +
> +      /* Insert the built up code sequence into the original instruction stream
> +	 the iterator points to.  */
> +      gsi_insert_seq_before (iter, stmts, GSI_SAME_STMT);
> +
> +      /* Finally, replace old alloca ptr with NEW_ALLOCA.  */
> +      replace_call_with_value (iter, addr);
> +      return;
> +    }
> +
> +  tree last_alloca = get_last_alloca_addr ();
> +  const HOST_WIDE_INT redzone_mask = ASAN_RED_ZONE_SIZE - 1;
> +
> +

Nit: excess blank line.

>    /* If ALIGN > ASAN_RED_ZONE_SIZE, we embed left redzone into first ALIGN
>       bytes of allocated space.  Otherwise, align alloca to ASAN_RED_ZONE_SIZE
>       manually.  */
> […]
> @@ -2351,7 +2686,8 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t,
>      {
>        if (DECL_THREAD_LOCAL_P (inner))
>  	return;
> -      if (!param_asan_globals && is_global_var (inner))
> +      if ((hwasan_sanitize_p () || !param_asan_globals)
> +	  && is_global_var (inner))
>          return;
>        if (!TREE_STATIC (inner))
>  	{

As mentioned in the previous round, I think it would be good to have
some commentary explaining this.

> […]
> +      /* If the length is a simple SHWI, then we can calculate the rounded up
> +	 length directly.  Otherwise we need to emit gimple to do this
> +	 rounding at runtime.  */
> +      if (tree_fits_shwi_p (len))
> +	{
> +	  unsigned HOST_WIDE_INT size_in_bytes = tree_to_shwi (len);
> +	  uint8_t tg_mask = HWASAN_TAG_GRANULE_SIZE - 1;
> +	  size_in_bytes = (size_in_bytes + tg_mask) & ~tg_mask;
> +	  new_len = build_int_cst (pointer_sized_int_node, size_in_bytes);
> +	}
> +      else
> +	new_len = hwasan_emit_round_up (&stmts, loc, len,
> +					HWASAN_TAG_GRANULE_SIZE);

I'm not sure the special case is worth it.  We might as well call
(hwasan_emit/gimple_build)_round_up unconditionally, which would also
have the nice effect of testing it more.

> +      gimple_build (&stmts, loc, as_combined_fn (IFN_HWASAN_MARK),

CFN_HWASAN_MARK

> +		    void_type_node, gimple_call_arg (g, 0),
> +		    base, new_len);
> +      gsi_replace_with_seq (iter, stmts, true);
> +      return false;
> +    }
> +
>    if (is_poison)
>      {
>        if (asan_handled_variables == NULL)
> […]
> +/* For hwasan stack tagging:
> +   Tag a region of space in the shadow stack according to the base pointer of
> +   an object on the stack.  N.b. the length provided in the internal call is
> +   required to be aligned to HWASAN_TAG_GRANULE_SIZE.  */
> +static void
> +expand_HWASAN_MARK (internal_fn, gcall *gc)
> +{
> +  HOST_WIDE_INT flag = tree_to_shwi (gimple_call_arg (gc, 0));
> +  bool is_poison = ((asan_mark_flags)flag) == ASAN_MARK_POISON;
> +
> +  tree base = gimple_call_arg (gc, 1);
> +  gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
> +  /* base is a pointer argument, hence in ptr_mode.
> +     We convert to Pmode for use in the targetm.memtag.extract_tag and
> +     targetm.memtag.untagged_pointer hooks.
> +     We then convert the result to ptr_mode for the emit_library_call.
> +
> +     This conversion is not for functionality so much as for the code to be
> +     correctly formed.  If ptr_mode is smaller than Pmode then the top byte of
> +     a Pmode value will be truncated in C code losing the tag values, and
> +     HWASAN will not work.  */
> +  rtx base_rtx = convert_memory_address (Pmode, expand_normal (base));
> +
> +  rtx tag = is_poison ? HWASAN_STACK_BACKGROUND
> +    : targetm.memtag.extract_tag (base_rtx, NULL_RTX);
> +  rtx address = targetm.memtag.untagged_pointer (base_rtx, NULL_RTX);
> +  address = convert_memory_address (ptr_mode, address);
> +
> +  tree len = gimple_call_arg (gc, 2);
> +  rtx r_len = expand_normal (len);
> +
> +  rtx func = init_one_libfunc ("__hwasan_tag_memory");
> +  emit_library_call (func, LCT_NORMAL, VOIDmode, address, ptr_mode,
> +		     tag, QImode, r_len, ptr_mode);
> +}
> +
> +/* For hwasan stack tagging:
> +   Store a tag into a pointer.  */
> +static void
> +expand_HWASAN_SET_TAG (internal_fn, gcall *gc)
> +{
> +  tree g_target = gimple_call_lhs (gc);
> +  tree g_ptr = gimple_call_arg (gc, 0);
> +  tree g_tag = gimple_call_arg (gc, 1);
> +
> +  rtx ptr = convert_memory_address (Pmode, expand_normal (g_ptr));
> +  rtx tag = expand_expr (g_tag, NULL_RTX, QImode, EXPAND_NORMAL);
> +  rtx target = expand_normal (g_target);
> +  machine_mode mode = GET_MODE (target);
> +
> +  rtx untagged = targetm.memtag.untagged_pointer (ptr, target);
> +  rtx tagged_value = targetm.memtag.set_tag (untagged, tag, target);
> +  /* Really need to put the return value into the `target` RTX, since that's
> +     the return value of the function.
> +     `target` will be in ptr_mode, while `tagged_value` will be in Pmode.
> +     These can be different.  When they are different we try to truncate the
> +     Pmode value into ptr_mode.  This will usually lose the tag, but since such
> +     a difference between ptr_mode and Pmode will already cause problems
> +     wherever the HWASAN library returns a pointer losing the tag here does not
> +     introduce new incompatibilities.
> +
> +     We do this mainly so that compiling for such a target with ptr_mode and
> +     Pmode sizes being different doesn't ICE, even if the resulting binary is
> +     not usable.  */

I'm not sure that's the right trade-off.  Producing wrong code is a more
serious class of bug than an ICE.

Could the code in principle be reached on ILP32 targets?  If so,
that seems like something we should fix.  If not, then I think we
should just do everything in Pmode, perhaps with an assertion that
Pmode == ptr_mode to make it clear that this is a deliberate choice.

I see what you mean about trying to make sure that types “agree”
at the appropriate interface.  But in a sense we're already failing
to do that because we assume Pmode is correct for all addresses,
without taking address spaces into account.  Assuming Pmode == ptr_mode
doesn't seem worse than assuming a unified address space.

LGTM otherwise though.

Thanks,
Richard


More information about the Gcc-patches mailing list