Re: [PATCH v2 0/9] Add new command to print the shadow stack backtrace

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Christina,

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> Hi Tom and Thiago, 
>
> First, thanks a lot for your review efforts on this series so far!

Thanks for moving this forward!

> As indicated in the cover letter of v2, I would like to post v3, including the -past-main option.

Great!

> Enabling this made me realize that I need some refactoring of the commits:
> - "gdb: Add command option 'bt -shadow' to print the shadow stack backtrace."
> - "gdb: Provide gdbarch hook to distinguish shadow stack backtrace elements."
>
> I have mixed feelings about this. On the one hand, the patches have already been very well
> reviewed (thanks again!) and the refactoring means that new review effort is required. But
> on the other hand, I think these changes improve the series overall. Below, I briefly describe
> the main changes to those commits. If this approach makes sense to you, I will post v3
> shortly after.

I think it makes sense to do the refactoring. Changing approaches as the
patch series evolve is part of the process.

> To avoid increasing your review effort, I also considered adding these changes as new
> patches on top. However, it is probably cleaner to merge them into the existing commits.
> Another option is that I post the V3 including fixups on top (and merge in a v4) to make
> the review easier for you. Or, we could also consider merging the series (once fully approved)
> without -past-main and I could submit a follow-up series for that feature. If you have any
> preference, I'm happy about your feedback.

Thank you for your concern in this regard. Of course I can't speak for
Tom but IMHO you can merge the changes into the existing commits.

I use git range-diff between versions to see what changed within each
patch, which provides similar output to the fixups approach.

> My main changes to enable -past-main are:

The changes below look good to me.

> 1) A new commit (required before "gdb: Add command option 'bt -shadow' to print the shadow stack backtrace."): 
>
>     gdb: Add get_main_func_start_pc to refactor frame.c:inside_main_func.
>
>     Refactor frame.c:inside_main_func to use the new function
>     get_main_func_start_pc, which will be used in a following commit.
>
> 2) Refactoring of "gdb: Add command option 'bt -shadow' to print the shadow stack backtrace." :
>
>     To enable the -past-main option in the shadow stack backtrace the function
>     get_main_func_start_pc (gdbarch* gdbarch, const language lang) is called by a
>     new function shadow_stack_frame_info::inside_main_func.  This probably requires
>     a frame specific gdbarch and language in the struct shadow_stack_frame_info.
>     I am not sure the gdbarch or language ever changes for IA, but it might be a good
>     idea to add it to be safe.

I agree that it's a good idea to add them.

>     So, due to that we need some refactoring of the shadow
>     stack backtrace implementation:
>     a. The frame specific arch is extracted based on the symtab_and_line (SAL)
>        object.  To avoid that we extract the SAL and gdbarch multiple times for
>        the same frame we store it in the shadow stack frame.
>     b. The gdbarch hook get_shadow_stack_size is obsolete since the function
>        get_trailing_outermost_shadow_stack_frame_info now unwinds each frame.  This
>        helps to configure the previous frame attributes in case the gdbarch or SAL
>        information cannot be extracted for special shadow stack elements.  We don't need
>        the COUNT parameter anymore in the function update_shadow_stack_pointer, too.
>        Now, this is also inline with the function trailing_outermost_frame which is used
>        for the normal backtrace.
>     c. Introduce function get_shadow_stack_frame_info to share some more logic:
>     ~~~
>     /* If possible, get shadow stack frame info for the shadow stack pointer
>        SSP and its current frame LEVEL.  Pass a fallback gdbarch FALLBACK_ARCH
>        which can be used as fallback in case the gdbarch cannot be extracted
>        from the SAL.  Usually this is the gdbarch of the previous frame.  */
>
>     static std::optional<shadow_stack_frame_info>
>     get_shadow_stack_frame_info
>       (gdbarch *fallback_arch, const CORE_ADDR ssp, ULONGEST level)
>     ~~~
>
> 3) Refactoring of "gdb: Provide gdbarch hook to distinguish shadow stack backtrace elements.
>
>     a. Like normal frames used in the ordinary backtrace command, introduce an enum
>        ssp_frame_type (normal_frame, non_return_frame) which classifies each frame and is
>        an attribute of the shadow stack frame info.  Calling of inside_main_func () to check
>        if we should stop unwinding is only done for normal frames.  For the currently known
>        cases of non-return-address shadow stack elements, the value is not a PC of the program
>        and the past-main check is not applicable.
>        More frame types will be added in the following patches to enable sigtramp and inferior
>        call frames.
>     b. The gdbarch hook is_no_return_shadow_stack_address now returns the complete shadow stack
>        frame (classified as non_return_frame) in case the element on the shadow stack is not a return
>        address (instead of the optional return address description string used before). The target can
>        then assign the correct gdbarch and an optional SAL object. Due to that hook is no longer called 
>        when printing the frame, but already when constructing it in get_shadow_stack_frame_info and
>        we don't pass the shadow stack frame as argument anymore, but the shadow stack pointer, its
>        value and level.
>
>        The new interface looks as follows:
>     ~~~
>     Method(
>         comment="""
>     There can be elements on the shadow stack which are not return addresses.
>     This happens for example on x86 with CET in case of signals.
>     If an architecture implements the command options 'backtrace -shadow' and

s/options/option/

>     the shadow stack can contain elements which are not return addresses, this
>     function has to be provided.
>     Return a shadow stack frame info with frame type
>     ssp_frame_type::non_return_frame, if the shadow stack pointer SSP belongs
>     to a valid shadow stack frame while the element on the shadow stack
>     VALUE does not refer to a return address.  In that case, also the frame's
>     attribute non_return_description must be set to a string which is displayed
>     instead of the element on the shadow stack in the shadow stack backtrace.
>     Otherwise, return an empty optional.
>     """,
>         type="std::optional<shadow_stack_frame_info>",
>         name="is_no_return_shadow_stack_address",
>         params=[
>           ("const CORE_ADDR", "ssp"),
>           ("const CORE_ADDR", "value"),
>           ("const unsigned long", "level")
>         ],
>         predicate=True,
>     )
>     ~~~
>     c. The shadow stack frame contains a new string attribute "non_return_description"
>         which is printed instead of the element on the shadow stack in case the frame is
>         classified as non-return address frame.  It is empty in case the frame is not a
>        " ssp_frame_type::non_return_frame" frame.  We could also use an optional here,
>        I am not sure what is preferred.

I'm not sure either, but I have a slight preference for an optional.

> This is now the class for a shadow stack frame:
>
> /* Information of a shadow stack frame belonging to a shadow stack element
>    at shadow stack pointer SSP.  */
>
> class shadow_stack_frame_info
> {

Looks good to me.

-- 
Thiago
(he/him)