Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

["Kewen.Lin" <linkw@linux.ibm.com>]

on 2024/1/12 19:03, Alexandre Oliva wrote:
> On Jan 12, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>>>> By checking PR112917, IMHO we should keep this unbiasing
>>>> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
>>>> TARGET_STACK_BIAS), similar to some existing code special
>>>> treating SPARC stack bias.
>>>
>>> I'm afraid this change will most certainly regress 32-bit sparc, because
>>> of the large register save area.
> 
>> Oh, I read the comments and commit logs in PR112917, mainly
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112917#{c4,c5,c6},
>> and the "sparc64" in subject of commit r14-6737 also implies
>> that this unbiasing is only required for sparc64, so I thought
>> it should be safe to guard with SPARC_STACK_BOUNDARY_HACK.
> 
> It is safe, in a way, because that protects potentially active stack
> areas, but it's unsafe in that it may leak data that stack scrubbing was
> supposed to scrub.  There's no conservative solution here, alas; we have
> to get it just right.
> 
> Specifically on sparc32, if __builtin_scrub_leave allocated its own
> frame (it doesn't) with the large register-save area for its potential
> (but inexistent) callees to use, it could overlap with a large chunk of
> the very stack frame that it's supposed to clear.

Thanks for the further explanation!

> 
> Unfortunately, this is slowly drifting away from the notion of stack
> address.  I mean, all of the following could conceivably be returned by
> __builtin_stack_address:
> 
> - the (biased) stack pointer
> 
> - the address of the red zone
> 
> - the unbiased stack pointer
> 
> - the address of the save area reserved by callees for potential callees
> 
> - the boundary between caller- and callee-used stack space
> 
> The last one is what we need for stack scrubbing, so that's what I'm
> planning to implement, but I'm pondering whether to change
> __builtin_stack_address() to take an extra argument to select among the
> different possibilities, or of other means to query these various
> offsets.  It feels like overthinking, so I'm trying to push these
> thoughts aside, but...  Does anyone think that would be a desirable
> feature?  We can always add it later.

One immature idea: maybe we can introduce a hook with clear meaning for
the last one and its default implementation still adopts the function
__builtin_stack_address directly, if this default implementation for
some port is imperfect, someone who is familiar with its own ABIs can
further enhance it with its own hook implementation.

BR,
Kewen