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

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
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
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?

--Andy