Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210427204315.24153-1-yu-cheng.yu@intel.com> <20210427204315.24153-26-yu-cheng.yu@intel.com>
 <CALCETrVTeYfzO-XWh+VwTuKCyPyp-oOMGH=QR_msG9tPQ4xPmA@mail.gmail.com> <YIpgB5HbnNPWX4FP@grain>
In-Reply-To: <YIpgB5HbnNPWX4FP@grain>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Thu, 29 Apr 2021 07:44:03 -0700
Message-ID: <CALCETrW27rTeaymtuJYJCyNgHfuCqA90KinvzNzwBg_vCnZLTw@mail.gmail.com>
Subject: Re: extending ucontext (Re: [PATCH v26 25/30] x86/cet/shstk: Handle
 signals for shadow stack)
To:     Cyrill Gorcunov <gorcunov@gmail.com>
Cc:     Andy Lutomirski <luto@kernel.org>,
        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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Apr 29, 2021 at 12:28 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> 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.

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