[PATCH v1 1/2] aarch64: check GCS feature in GNU properties of input dynamic objects
Matthieu Longo
matthieu.longo@arm.com
Wed Jan 15 17:00:51 GMT 2025
On 2025-01-15 13:52, Richard Sandiford wrote:
> 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.
Fixed.
>> + 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.
>
Fixed.
>> [...]
>> 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.
Fixed.
>> +@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.
>
Fixed.
> 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
More information about the Binutils
mailing list