Hi Richard,
On Thu, Oct 24, 2024 at 05:27:24PM +0100, Richard Sandiford wrote:
> Yury Khrustalev <yury.khrusta...@arm.com> writes:
> > From: Szabolcs Nagy <szabolcs.n...@arm.com>
>
> Could you explain these testsuite changes in more detail? It seems
> on the face of it that they're changing the tests to test something
> other than the original intention.
After reviewing I agree that this change to the tests is not needed in
the context of GCS work. I will remove this from the patch.
>
> Having new tests alongside the same lines would be fine though.
Agree, but I think it's better to submit these test changes separately.
> > +/* On signal entry the OS places a token on the GCS that can be used to
> > + verify the integrity of the GCS pointer on signal return. It also
> > + places the signal handler return address (the restorer that calls the
> > + signal return syscall) on the GCS so the handler can return.
> > + Because of this token, each stack frame visited during unwinding has
> > + exactly one corresponding entry on the GCS, so the frame count is
> > + the number of entries that will have to be popped at EH return time.
> > +
> > + Note: This depends on the GCS signal ABI of the OS.
> > +
> > + When unwinding across a stack frame for each frame the corresponding
> > + entry is checked on the GCS against the computed return address from
> > + the normal stack. If they don't match then _URC_FATAL_PHASE2_ERROR
> > + is returned. This check is omitted if
> > +
> > + 1. GCS is disabled. Note: asynchronous GCS disable is supported here
> > + if GCSPR and the GCS remains readable.
> > + 2. Non-catchable exception where exception_class == 0. Note: the
> > + pthread cancellation implementation in glibc sets exception_class
> > + to 0 when the unwinder is used for cancellation cleanup handling,
> > + so this allows the GCS to get out of sync during cancellation.
> > + This weakens security but avoids an ABI break in glibc.
> > + 3. Zero return address which marks the outermost stack frame.
>
> I suppose this is a question for the x86 implementation too, but:
> doesn't this weaken the checks somewhat? I would imagine zero is the
> easiest value to force. Would it be possible to add some extra sanity
> check that this really is the outermost frame? E.g. IIUC, the entry
> above the outermost frame's would be a cap, or at least would not be
> a valid procedure return record, so could we check for that?
Zero might be indeeded easier to forge, however in practice it would mean
that execution would not be able to return anywhere, so one could speculate
this would be hard to exploit in practice.
When we are in the middle of unwinding and hit zero return address (as seen
by the undwinder) we could theoretically check if current GCS entry is non-
zero (in which case it would mean that stack has been corrupted and we need
to raise an error). However if we are really at the outermost frame and GCS
stack has been unwound properly, we would not be able to read from shadow
stack as we have already reached to top of its allocation. This is why we
need to if _Unwind_GetIP (context) == 0 before we access GCS pointer via
__builtin_aarch64_gcspr ().
So I will leave this as implemented by Szabolcs.
Kind regards,
Yury
>
> Thanks,
> Richard
>
> > + 4. Signal stack frame, the GCS entry is an OS specific token then
> > + with the top bit set.
> > + */
> > +#undef _Unwind_Frames_Increment
> > +#define _Unwind_Frames_Increment(exc, context, frames) \
> > + do \
> > + { \
> > + frames++; \
> > + if (__builtin_aarch64_chkfeat (CHKFEAT_GCS) != 0 \
> > + || exc->exception_class == 0 \
> > + || _Unwind_GetIP (context) == 0) \
> > + break; \
> > + const _Unwind_Word *gcs = __builtin_aarch64_gcspr (); \
> > + if (_Unwind_IsSignalFrame (context)) \
> > + { \
> > + if (gcs[frames] >> 63 == 0) \
> > + return _URC_FATAL_PHASE2_ERROR; \
> > + } \
> > + else \
> > + { \
> > + if (gcs[frames] != _Unwind_GetIP (context)) \
> > + return _URC_FATAL_PHASE2_ERROR; \
> > + } \
> > } \
> > while (0)
