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

From: Cyrill Gorcunov <gorcunov@gmail.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>,
	linux-arch <linux-arch@vger.kernel.org>, X86 ML <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>,
	Linux API <linux-api@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Balbir Singh <bsingharora@gmail.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Eugene Syromiatnikov <esyr@redhat.com>,
	Florian Weimer <fweimer@redhat.com>,
	"H.J. Lu" <hjl.tools@gmail.com>, Jann Horn <jannh@google.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Kees Cook <keescook@chromium.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>, Pavel Machek <pavel@ucw.cz>,
	Peter Zijlstra <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Vedvyas Shanbhogue <vedvyas.shanbhogue@intel.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Weijiang Yang <weijiang.yang@intel.com>,
	Pengfei Xu <pengfei.xu@intel.com>,
	Haitao Huang <haitao.huang@intel.com>
Subject: Re: extending ucontext (Re: [PATCH v26 25/30] x86/cet/shstk: Handle signals for shadow stack)
Date: Thu, 29 Apr 2021 10:28:07 +0300	[thread overview]
Message-ID: <YIpgB5HbnNPWX4FP@grain> (raw)
In-Reply-To: <CALCETrVTeYfzO-XWh+VwTuKCyPyp-oOMGH=QR_msG9tPQ4xPmA@mail.gmail.com>

On Wed, Apr 28, 2021 at 04:03:55PM -0700, Andy Lutomirski wrote:
> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > 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.