[PATCH v1 1/2] aarch64: check GCS feature in GNU properties of input dynamic objects

[Richard Sandiford]

Matthieu Longo <matthieu.longo@arm.com> writes:
> The Guarded Control Stack (GCS) feature requires that two things:
> - at static link time, all the input objects of a link unit have to
>   be compatible with GCS.
> - at runtime, the executable and the shared libraries which it
>   depends on have to be compatible with GCS.
> Both of those criteria are checked with the GCS feature stored in
> the GNU property note.
>
> The previous patch, adding support for the GCS feature check in GNU
> note properties for input objects, ignored the input dynamic objects.
> Although this support was better than no check, it was still
> delaying the detection of compatibility issues up to the runtime
> linker.
>
> In order to help the developer in detecting such an incompatibility
> issue as early as possible, this patch adds a check for input dynamic
> objects lacking the GCS marking. This check can be controlled via the
> linker option '-z gcs-report-dynamic[=none|warning|error]'. By default,
> if the option is omitted, it inherits the value from '-z gcs-report'.
> However, the inherited value is capped to 'warning' as a user might
> want to only report errors in the currently built module, and not the
> shared dependencies. If a user also wants to error on GCS issues in
> the shared libraries, '-z gcs-report-dynamic=error' will have to be
> specified explicitly.
> ---
>  bfd/elfnn-aarch64.c        |  14 +++++
>  bfd/elfxx-aarch64.c        | 119 +++++++++++++++++++++++++++++++------
>  bfd/elfxx-aarch64.h        |  13 +++-
>  ld/emultempl/aarch64elf.em |  34 +++++++++--
>  ld/ld.texi                 |  30 ++++++++--
>  5 files changed, 181 insertions(+), 29 deletions(-)
>
> diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
> index 701191470dc..d5f43a86cbe 100644
> --- a/bfd/elfnn-aarch64.c
> +++ b/bfd/elfnn-aarch64.c
> [...]
> @@ -790,6 +788,69 @@ _bfd_aarch64_report_summary_merge_issues (struct bfd_link_info *info)
>  	    "GCS requirements.\n");
>        info->callbacks->einfo (msg, tdata->n_gcs_issues);
>      }
> +
> +  if (tdata->n_gcs_dynamic_issues > GNU_PROPERTY_ISSUES_MAX
> +      && tdata->sw_protections.gcs_report_dynamic != MARKING_NONE)
> +    {
> +      const char *msg
> +	= (tdata->sw_protections.gcs_report_dynamic == MARKING_ERROR)
> +	? _("%Xerror: found a total of %d dynamically-linked objects"
> +	    "incompatible with GCS requirements.\n")
> +	: _("warning: found a total of %d dynamically-linked objects"
> +	    "incompatible with GCS requirements.\n");

There's a missing space after "objects" in both messages.

> +      info->callbacks->einfo (msg, tdata->n_gcs_dynamic_issues);
> +    }
> +}
> +
> [...]
> @@ -1047,21 +1111,42 @@ void
>  _bfd_aarch64_elf_check_gcs_report (struct bfd_link_info *info, bfd *ebfd)
>  {
>    struct elf_aarch64_obj_tdata *tdata = elf_aarch64_tdata (info->output_bfd);
> +  bool dynamic_obj = (ebfd->flags & DYNAMIC) != 0;
>  
> -  if (tdata->sw_protections.gcs_report == MARKING_NONE)
> +  if ((!dynamic_obj && (tdata->sw_protections.gcs_report == MARKING_NONE))
> +      || (tdata->sw_protections.gcs_report_dynamic == MARKING_NONE))
>      return;

I think we should sink this into:

> -  ++tdata->n_gcs_issues;
> -
> -  if (tdata->n_gcs_issues > GNU_PROPERTY_ISSUES_MAX)
> -    return;
> +  if (dynamic_obj)
> +    {
> +      ++tdata->n_gcs_dynamic_issues;
> +      if (tdata->n_gcs_dynamic_issues > GNU_PROPERTY_ISSUES_MAX)
> +	return;
> +    }
> +  else
> +    {
> +      ++tdata->n_gcs_issues;
> +      if (tdata->n_gcs_issues > GNU_PROPERTY_ISSUES_MAX)
> +	return;
> +    }

...here, i.e.

  if (dynamic_obj)
    {
      if (tdata->sw_protections.gcs_report == MARKING_NONE)
	return;
      ++tdata->n_gcs_dynamic_issues;
      if (tdata->n_gcs_dynamic_issues > GNU_PROPERTY_ISSUES_MAX)
	return;
    }
  else
    {
      if (tdata->sw_protections.gcs_report == MARKING_NONE)
	return;
      ++tdata->n_gcs_issues;
      if (tdata->n_gcs_issues > GNU_PROPERTY_ISSUES_MAX)
	return;
    }

so that the unlikely combination -z gcs-report=none
-z gcs-report-dynamic=warning still warns for dynamic objects.

> [...]  
> diff --git a/ld/ld.texi b/ld/ld.texi
> index eb36eaf21d6..3e9628dab83 100644
> --- a/ld/ld.texi
> +++ b/ld/ld.texi
> @@ -8235,15 +8235,33 @@ mark the output with GCS.
>  
>  @kindex -z gcs-report[=none|warning|error]
>  @cindex Control warnings for missing GCS markings.
> -The @samp{-z gcs-report[=none|warning|error]} specifies how to report the missing
> -GCS markings on inputs, i.e. the GNU_PROPERTY_AARCH64_FEATURE_1_GCS property.
> -By default, if the option is omitted and @samp{-z gcs} is provided, warnings are
> -emitted.
> +The @samp{-z gcs-report[=none|warning|error]} option specifies how to report the
> +missing GCS markings on inputs, i.e. the GNU_PROPERTY_AARCH64_FEATURE_1_GCS
> +property. By default, if the option is omitted and @samp{-z gcs} is provided,
> +warnings are emitted.
>  @itemize
>  @item@samp{none} disables any warning messages.
>  @item@samp{warning} (the default value) emits warning messages when input objects
> -composing the link unit are missing GCS markings, or dynamic objects containing
> -external symbols used in the link unit.
> +composing the link unit are missing GCS markings.
> +@item@samp{error} turns the warning messages into errors.
> +@end itemize
> +If issues are found, a maximum of 20 messages will be emitted, and then a summary
> +with the total number of issues will be displayed at the end.
> +
> +@kindex -z gcs-report-dynamic=none|warning|error
> +@cindex Control warnings for missing GCS markings on dynamic input objects.
> +The @samp{-z gcs-report-dynamic=none|warning|error} option specifies how to
> +report the missing GCS markings on dynamic input objects, i.e. the
> +GNU_PROPERTY_AARCH64_FEATURE_1_GCS property. By default, if the option is
> +omitted, it inherits the value of @samp{-z gcs-report}. However, the inherited
> +value is capped to @samp{warning} as a user might want to only report errors in

In this context, where we're talking to potential users, I think it'd
be more usual to say s/a user/some users/ (i.e. maybe you the user
reading this, but maybe a different user).

> +the currently built module, and not the shared dependencies. If a user also wants
> +to error on GCS issues in the shared libraries, @samp{-z gcs-report-dynamic=error}
> +will have to be specified explicitly.

For the same reason, maybe:

  It is therefore necessary to use an explicit @samp{-z
  gcs-report-dynamic=error} option if you want the linker to
  error on GCS issues in the shared libraries.

> +@itemize
> +@item@samp{none} disables any warning messages.
> +@item@samp{warning} emits warning messages when dynamic objects containing
> +external symbols used in the link unit are missing GCS markings.

Maybe just "when linked dynamic objects are missing GCS markings".
Dynamic objects can be linked directly, even if they aren't needed
to satisfy a symbol dependency.

OK with those changes, thanks.

Richard

>  @item@samp{error} turns the warning messages into errors.
>  @end itemize
>  If issues are found, a maximum of 20 messages will be emitted, and then a summary