Re: extending ucontext (Re: [PATCH v26 25/30] x86/cet/shstk: Handle signals for shadow stack)

On Thu, Apr 29, 2021 at 07:44:03AM -0700, Andy Lutomirski wrote:

On Thu, Apr 29, 2021 at 12:28 AM Cyrill Gorcunov [off-list ref] wrote:

quoted

On Wed, Apr 28, 2021 at 04:03:55PM -0700, Andy Lutomirski wrote:

quoted

On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu [off-list ref] wrote:

quoted

When shadow stack is enabled, a task's shadow stack states must be saved
along with the signal context and later restored in sigreturn.  However,
currently there is no systematic facility for extending a signal context.
There is some space left in the ucontext, but changing ucontext is likely
to create compatibility issues and there is not enough space for further
extensions.

Introduce a signal context extension struct 'sc_ext', which is used to save
shadow stack restore token address.  The extension is located above the fpu
states, plus alignment.  The struct can be extended (such as the ibt's
wait_endbr status to be introduced later), and sc_ext.total_size field
keeps track of total size.

I still don't like this.

Here's how the signal layout works, for better or for worse:

The kernel has:

struct rt_sigframe {
    char __user *pretcode;
    struct ucontext uc;
    struct siginfo info;
    /* fp state follows here */
};

This is roughly the actual signal frame.  But userspace does not have
this struct declared, and user code does not know the sizes of the
fields.  So it's accessed in a nonsensical way.  The signal handler

Well, not really. While indeed this is not declared as a part of API
the structure is widely used for rt_sigreturn syscall (and we're using
it inside criu thus any change here will simply break the restore
procedure). Sorry out of time right now, I'll read your mail more
carefully once time permit.

I skimmed the CRIU code.  You appear to declare struct rt_sigframe,
and you use the offset from the start of rt_sigframe to uc.  You also
use the offset to the /* fp state follows here */ part, but that's
unnecessary -- you could just as easily have put the fp state at any
other address -- the kernel will happily follow the pointer you supply
regardless of where it points.  So the only issues I can see are if

Yes, since rt_sigreturn is a very late stage of restore we've to preallocate
memory early and surely doing it in one slab is a way more convenient, thus
that's why fp context follows the structure declaration (well, it was at
least I didn't touch this code for years already). Anyway, I meant that
assuming "user code does not know the sizes of the fields" is not exactly
true and strictly speaking current layout became a part of nondeclared api
(simply because kernel use this structure run-time only and even if something
get changed in kernel declaration for this structure the kernel itself will
be fine because it doesn't save it on disk between kernel updates) but for
tools like criu such change might require update. IOW, I mean that changing
rt_sigreturn structure itself is not safe and we can't reorder the members
and theirs size.

Or I misunderstood you, and you meant only the fact that fp may be arbitrary,
pointing for any place in userspace? If so then indeed, fp can be pointing
to context placed anywere since kernel will fetch it from the address during
rt_sigreturn processing.

you write the fp state on top of something else or if you
inadvertently fill in the proposed extension part of uc_flags.  Right
now you seem to be ignoring uc_flags, which I presume means that you
are filling it in as zero.  Even if the offset of the fp state in the

yes

kernel rt_sigframe changes, the kernel should still successfully parse
the signal frame you generate.

I suppose there is another potential issue: would CRIU have issues if
the *save* runs on a kernel that uses this proposed extension
mechanism?  Are you doing something with the saved state that would
get confused?

You know, actually I guess criu may not work with new extensions, we have
a predefined max xsave frame size and any new extensions to cpu states
will require additional support.

I must confess I didn't follow much on CET internals yet so I must be simply
wrong. CC'ing Anrew just in case.

	Cyrill