| Message ID | 20180710222639.8241-16-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote: > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> This still needs a changelog, even if you think it's simple. > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -446,6 +446,15 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > error = -ENOMEM; > if (!vma) > goto out; > + > + /* > + * Do not allow changing shadow stack memory. > + */ > + if (vma->vm_flags & VM_SHSTK) { > + error = -EINVAL; > + goto out; > + } > + I think this is a _bit_ draconian. Why shouldn't we be able to use protection keys with a shadow stack? Or, set it to PROT_NONE?
On Tue, Jul 10, 2018 at 04:10:08PM -0700, Dave Hansen wrote: > On 07/10/2018 03:26 PM, Yu-cheng Yu wrote: > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > This still needs a changelog, even if you think it's simple. > > --- a/mm/mprotect.c > > +++ b/mm/mprotect.c > > @@ -446,6 +446,15 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > > error = -ENOMEM; > > if (!vma) > > goto out; > > + > > + /* > > + * Do not allow changing shadow stack memory. > > + */ > > + if (vma->vm_flags & VM_SHSTK) { > > + error = -EINVAL; > > + goto out; > > + } > > + > > I think this is a _bit_ draconian. Why shouldn't we be able to use > protection keys with a shadow stack? Or, set it to PROT_NONE? Right, and then there's also madvise() and some of the other accessors. Why do we need to disallow this? AFAICT the worst that can happen is that a process wrecks itself, so what?
On Wed, 2018-07-11 at 11:12 +0200, Peter Zijlstra wrote: > On Tue, Jul 10, 2018 at 04:10:08PM -0700, Dave Hansen wrote: > > > > On 07/10/2018 03:26 PM, Yu-cheng Yu wrote: > > > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > This still needs a changelog, even if you think it's simple. > > > > > > --- a/mm/mprotect.c > > > +++ b/mm/mprotect.c > > > @@ -446,6 +446,15 @@ static int do_mprotect_pkey(unsigned long > > > start, size_t len, > > > error = -ENOMEM; > > > if (!vma) > > > goto out; > > > + > > > + /* > > > + * Do not allow changing shadow stack memory. > > > + */ > > > + if (vma->vm_flags & VM_SHSTK) { > > > + error = -EINVAL; > > > + goto out; > > > + } > > > + > > I think this is a _bit_ draconian. Why shouldn't we be able to use > > protection keys with a shadow stack? Or, set it to PROT_NONE? > Right, and then there's also madvise() and some of the other > accessors. > > Why do we need to disallow this? AFAICT the worst that can happen is > that a process wrecks itself, so what? Agree. I will remove the patch.
On 07/11/2018 09:07 AM, Yu-cheng Yu wrote: >> Why do we need to disallow this? AFAICT the worst that can happen is >> that a process wrecks itself, so what? > Agree. I will remove the patch. No so quick. :) We still need to find out a way to handle things that ask for an mprotect() which is incompatible with shadow stacks. PROT_READ without PROT_WRITE comes to mind. We also need to be careful that we don't copy-on-write/copy-on-access pages which fault on PROT_NONE. I *think* it'll get done correctly but we have to be sure. BTW, where are all the selftests for this code? We're slowly building up a list of pathological things that need to get tested. I don't think this can or should get merged before we have selftests.
diff --git a/mm/mprotect.c b/mm/mprotect.c index 625608bc8962..128dcb880c12 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -446,6 +446,15 @@ static int do_mprotect_pkey(unsigned long start, size_t len, error = -ENOMEM; if (!vma) goto out; + + /* + * Do not allow changing shadow stack memory. + */ + if (vma->vm_flags & VM_SHSTK) { + error = -EINVAL; + goto out; + } + prev = vma->vm_prev; if (unlikely(grows & PROT_GROWSDOWN)) { if (vma->vm_start >= end)
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- mm/mprotect.c | 9 +++++++++ 1 file changed, 9 insertions(+)