* Re: [RFC PATCH v2 11/27] x86/mm: Modify ptep_set_wrprotect and pmdp_set_wrprotect for _PAGE_DIRTY_SW
From: Peter Zijlstra @ 2018-07-11 8:48 UTC (permalink / raw)
To: Dave Hansen
Cc: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <fbf45667-5388-44a6-1f22-07bcc03e1804@linux.intel.com>
On Tue, Jul 10, 2018 at 03:44:32PM -0700, Dave Hansen wrote:
> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> > + /*
> > + * On platforms before CET, other threads could race to
> > + * create a RO and _PAGE_DIRTY_HW PMD again. However,
> > + * on CET platforms, this is safe without a TLB flush.
> > + */
>
> If I didn't work for Intel, I'd wonder what the heck CET is and what the
> heck it has to do with _PAGE_DIRTY_HW. I think we need a better comment
And Changelog, the provided one is abysmal.
> than this. How about:
>
> Some processors can _start_ a write, but end up seeing
> a read-only PTE by the time they get to getting the
> Dirty bit. In this case, they will set the Dirty bit,
> leaving a read-only, Dirty PTE which looks like a Shadow
> Stack PTE.
>
> However, this behavior has been improved and will *not* occur on
> processors supporting Shadow Stacks. Without this guarantee, a
> transition to a non-present PTE and flush the TLB would be
> needed.
I'm still struggling. I think I get the first paragraph, but then what?
^ permalink raw reply
* Re: [RFC PATCH v2 10/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Peter Zijlstra @ 2018-07-11 8:45 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek, Ravi
In-Reply-To: <20180710222639.8241-11-yu-cheng.yu@intel.com>
On Tue, Jul 10, 2018 at 03:26:22PM -0700, Yu-cheng Yu wrote:
> + pte = pte_clear_flags(pte, _PAGE_DIRTY_SW);
> return pte_set_flags(pte, _PAGE_DIRTY_HW | _PAGE_SOFT_DIRTY);
Having both _PAGE_DIRTY_SW and _PAGE_SOFT_DIRTY is really confusing.
I'm not sure I have an anwser for this, but urggh.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-11 8:43 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Al Viro, Linus Torvalds, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <CALCETrXWZJfBq-HnQ0vnGUBsDW75d4XFp1aQfTSqrFobQtFfbQ@mail.gmail.com>
Andy Lutomirski <luto@kernel.org> wrote:
> > Umm... How about "use credentials of opener for everything"?
>
> If you want to audit every single filesystem for any code that uses
> credentials for anything and add all the right kernel APIs and make
> sure the filesystem uses them and somehow keep screwups from getting
> added down the line, then okay I guess. As far as I know, we don't
> even *have* an API for "open this device node using this struct cred
> *".
You can use override_creds() too.
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-11 8:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Andy Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <CA+55aFzknfBLjg4-pRSCUeBFY3DWq04zX3PPyc949M0OygHktw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Yeah, Andy is right that we should *not* make "write()" have side effects.
Note that write() has side effects all over the place: procfs, sysfs, debugfs,
tracefs, ... Though for the most part they're single-shot jobs and not
cumulative (I'm not sure this is always true for debugfs - there's a lot of
weird stuff in there).
> > (b) Keep the current structure but use a new syscall instead of write().
> >
> > (c) Keep using write() but literally just buffer the data. Then have a new
> > syscall to commit it. In other words, replace “x” with a syscall and call
> > all the fs_context_operations helpers in that context instead of from
> > write().
>
> But yeah, b-or-c sounds fine.
I would prefer to avoid the "let's buffer everything" but rather parse the
data as we go along. What I currently do is store the parsed data in the
context and only actually *apply* it when someone sends the 'x' command.
There are two reasons for this:
(1) mount()'s error handling is slight: it can only return an error code, but
creating and mounting something has so many different and interesting
ways of going wrong and I want to be able to give better error reporting.
This gets more interesting if it happens inside a container where you
can't see dmesg.
(2) Parsing the data means you only need to store the result of the parse and
can reject anything that's unknown or contradictory.
Buffering till the end means you have to buffer *everything* - and,
unless you limit your buffer, you risk running out of RAM.
Now, I can replace the 'x' command with an ioctl() so that just writing random
rubbish to the fd won't cause anything to actually happen.
fd = fsopen("ext4");
write(fd, "s /dev/sda1");
write(fd, "o user_xattr");
ioctl(fd, FSOPEN_IOC_CREATE_SB, 0);
or I could make a special syscall for it:
fscommit(fd, FSCOMMIT_CREATE);
or:
fscommit(fd, FSCOMMIT_RECONFIGURE);
and require that you have CAP_SYS_ADMIN to enact it.
David
^ permalink raw reply
* Re: [RFC PATCH v2 08/27] mm: Introduce VM_SHSTK for shadow stack memory
From: Peter Zijlstra @ 2018-07-11 8:34 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek, Ravi
In-Reply-To: <20180710222639.8241-9-yu-cheng.yu@intel.com>
On Tue, Jul 10, 2018 at 03:26:20PM -0700, Yu-cheng Yu wrote:
> VM_SHSTK indicates a shadow stack memory area.
>
> A shadow stack PTE must be read-only and dirty. For non shadow
> stack, we use a spare bit of the 64-bit PTE for dirty. The PTE
> changes are in the next patch.
This doesn't make any sense.. the $subject and the patch seem completely
unrelated to this Changelog.
^ permalink raw reply
* Re: [RFC PATCH v2 05/27] Documentation/x86: Add CET description
From: Pavel Machek @ 2018-07-11 8:27 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Peter Zijlstra
In-Reply-To: <20180710222639.8241-6-yu-cheng.yu@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]
On Tue 2018-07-10 15:26:17, Yu-cheng Yu wrote:
> Explain how CET works and the no_cet_shstk/no_cet_ibt kernel
> parameters.
>
> --- /dev/null
> +++ b/Documentation/x86/intel_cet.txt
> @@ -0,0 +1,250 @@
> +=========================================
> +Control Flow Enforcement Technology (CET)
> +=========================================
We normally use .rst for this kind of formatted text.
> +[6] The implementation of the SHSTK
> +===================================
> +
> +SHSTK size
> +----------
> +
> +A task's SHSTK is allocated from memory to a fixed size that can
> +support 32 KB nested function calls; that is 256 KB for a 64-bit
> +application and 128 KB for a 32-bit application. The system admin
> +can change the default size.
How does admin change that? We already have ulimit for stack size,
should those be somehow tied together?
$ ulimit -a
...
stack size (kbytes, -s) 8192
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-11 7:22 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, viro, linux-api, linux-fsdevel, torvalds, linux-kernel,
jannh
In-Reply-To: <686E805C-81F3-43D0-A096-50C644C57EE3@amacapital.net>
Andy Lutomirski <luto@amacapital.net> wrote:
> > sfd = fsopen("ext4", FSOPEN_CLOEXEC);
> > write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
>
> Imagine some malicious program passes sfd as stdout to a setuid
> program. That program gets persuaded to write "s /etc/shadow". What
> happens? You’re okay as long as *every single fs* gets it right, but that’s
> asking a lot.
Do note that you must already have CAP_SYS_ADMIN to be able to call fsopen().
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-11 1:48 UTC (permalink / raw)
To: Al Viro
Cc: Andy Lutomirski, David Howells, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <20180711011520.GL30522@ZenIV.linux.org.uk>
On Tue, Jul 10, 2018 at 6:15 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Umm... How about "use credentials of opener for everything"?
yeah, we have that for writes in general.
Nobody ever actually follows that rule. They may *think* they do, and
then they call to some helper that does "capability(CAP_SYS_WHATEVAH)"
without even realizing it.
But I'm certainly ok with writes, if it's just filling a buffer.
Preferably a standard buffer we already have, like a seqfile or pipe
(hey, splice!) or whatever.
And then you have that final op to actually "commit" the state. Which
shouldn't be a write (and not the close).
Linus
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-11 1:33 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, David Howells, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <20180711011520.GL30522@ZenIV.linux.org.uk>
On Tue, Jul 10, 2018 at 6:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Jul 10, 2018 at 06:05:49PM -0700, Linus Torvalds wrote:
>> Yeah, Andy is right that we should *not* make "write()" have side effects.
>>
>> Use it to queue things by all means, but not "do" things. Not unless
>> there's a very sane security model.
>>
>> On Tue, Jul 10, 2018 at 4:59 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> >
>> > I think the right solution is one of:
>> >
>> > (a) Pass a netlink-formatted blob to fsopen() and do the whole thing in one syscall. I don’t mean using netlink sockets — just the nlattr format. Or you could use a different format. The part that matters is using just one syscall to do the whole thing.
>>
>> Please no. Not another nasty marshalling thing.
>>
>> > (b) Keep the current structure but use a new syscall instead of write().
>> >
>> > (c) Keep using write() but literally just buffer the data. Then have a new syscall to commit it. In other words, replace “x” with a syscall and call all the fs_context_operations helpers in that context instead of from write().
>>
>> But yeah, b-or-c sounds fine.
>
> Umm... How about "use credentials of opener for everything"?
If you want to audit every single filesystem for any code that uses
credentials for anything and add all the right kernel APIs and make
sure the filesystem uses them and somehow keep screwups from getting
added down the line, then okay I guess. As far as I know, we don't
even *have* an API for "open this device node using this struct cred
*".
I kind of want to add a hack to set some poison bit in current->cred
in sys_write() and clear it on the way out. Sigh.
--Andy
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Al Viro @ 2018-07-11 1:16 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Linux API, linux-fsdevel,
Linus Torvalds, kernel list
In-Reply-To: <CAG48ez2-p=jGZwmb3soh87RM6qHGEi6ctbLOVZ99LG6aHXUX_g@mail.gmail.com>
On Tue, Jul 10, 2018 at 06:14:10PM -0700, Jann Horn wrote:
> I also love ioctls, so I think you could also use an ioctl to do the
> commit? You can do anything (well, almost anything) that you can do in
> syscall context in ioctl context, too; and when you already have a
> file descriptor of a specific type that you want to perform an
> operation on, an ioctl works just fine.
Poe's Law in action...
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Al Viro @ 2018-07-11 1:15 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, David Howells, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <CA+55aFzknfBLjg4-pRSCUeBFY3DWq04zX3PPyc949M0OygHktw@mail.gmail.com>
On Tue, Jul 10, 2018 at 06:05:49PM -0700, Linus Torvalds wrote:
> Yeah, Andy is right that we should *not* make "write()" have side effects.
>
> Use it to queue things by all means, but not "do" things. Not unless
> there's a very sane security model.
>
> On Tue, Jul 10, 2018 at 4:59 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > I think the right solution is one of:
> >
> > (a) Pass a netlink-formatted blob to fsopen() and do the whole thing in one syscall. I don’t mean using netlink sockets — just the nlattr format. Or you could use a different format. The part that matters is using just one syscall to do the whole thing.
>
> Please no. Not another nasty marshalling thing.
>
> > (b) Keep the current structure but use a new syscall instead of write().
> >
> > (c) Keep using write() but literally just buffer the data. Then have a new syscall to commit it. In other words, replace “x” with a syscall and call all the fs_context_operations helpers in that context instead of from write().
>
> But yeah, b-or-c sounds fine.
Umm... How about "use credentials of opener for everything"?
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Jann Horn @ 2018-07-11 1:14 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Al Viro, Linux API, linux-fsdevel, Linus Torvalds,
kernel list
In-Reply-To: <686E805C-81F3-43D0-A096-50C644C57EE3@amacapital.net>
On Tue, Jul 10, 2018 at 4:59 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> [cc Jann - you love this stuff]
>
> > On Jul 10, 2018, at 3:44 PM, David Howells <dhowells@redhat.com> wrote:
> >
> > Provide an fsopen() system call that starts the process of preparing to
> > create a superblock that will then be mountable, using an fd as a context
> > handle. fsopen() is given the name of the filesystem that will be used:
> >
> > int mfd = fsopen(const char *fsname, unsigned int flags);
>
> This is great in principle, but I think you’re seriously playing with fire with the API.
>
> >
> > where flags can be 0 or FSOPEN_CLOEXEC.
> >
> > For example:
> >
> > sfd = fsopen("ext4", FSOPEN_CLOEXEC);
> > write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
>
> Imagine some malicious program passes sfd as stdout to a setuid program. That program gets persuaded to write “s /etc/shadow”. What happens? You’re okay as long as *every single fs* gets it right, but that’s asking a lot.
>
> > write(sfd, "o noatime");
> > write(sfd, "o acl");
> > write(sfd, "o user_attr");
> > write(sfd, "o iversion");
> > write(sfd, "o ");
> > write(sfd, "r /my/container"); // root inside the fs
> > write(sfd, "x create"); // create the superblock
>
> From cursory inspection of a bunch of the code, I think the expectation is that the actual device access happens in the “x” action. This is not okay. You can’t do this kind of thing in a write() handler, unless you somehow make every single access using f_cred, which is a real pain.
>
> I think the right solution is one of:
>
> (a) Pass a netlink-formatted blob to fsopen() and do the whole thing in one syscall. I don’t mean using netlink sockets — just the nlattr format. Or you could use a different format. The part that matters is using just one syscall to do the whole thing.
>
> (b) Keep the current structure but use a new syscall instead of write().
>
> (c) Keep using write() but literally just buffer the data. Then have a new syscall to commit it. In other words, replace “x” with a syscall and call all the fs_context_operations helpers in that context instead of from write().
I also love ioctls, so I think you could also use an ioctl to do the
commit? You can do anything (well, almost anything) that you can do in
syscall context in ioctl context, too; and when you already have a
file descriptor of a specific type that you want to perform an
operation on, an ioctl works just fine.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-11 1:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <686E805C-81F3-43D0-A096-50C644C57EE3@amacapital.net>
Yeah, Andy is right that we should *not* make "write()" have side effects.
Use it to queue things by all means, but not "do" things. Not unless
there's a very sane security model.
On Tue, Jul 10, 2018 at 4:59 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> I think the right solution is one of:
>
> (a) Pass a netlink-formatted blob to fsopen() and do the whole thing in one syscall. I don’t mean using netlink sockets — just the nlattr format. Or you could use a different format. The part that matters is using just one syscall to do the whole thing.
Please no. Not another nasty marshalling thing.
> (b) Keep the current structure but use a new syscall instead of write().
>
> (c) Keep using write() but literally just buffer the data. Then have a new syscall to commit it. In other words, replace “x” with a syscall and call all the fs_context_operations helpers in that context instead of from write().
But yeah, b-or-c sounds fine.
Linus
^ permalink raw reply
* Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch tracking support
From: Dave Hansen @ 2018-07-11 0:11 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-23-yu-cheng.yu@intel.com>
Is this feature *integral* to shadow stacks? Or, should it just be in a
different series?
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> index d9ae3d86cdd7..71da2cccba16 100644
> --- a/arch/x86/include/asm/cet.h
> +++ b/arch/x86/include/asm/cet.h
> @@ -12,7 +12,10 @@ struct task_struct;
> struct cet_status {
> unsigned long shstk_base;
> unsigned long shstk_size;
> + unsigned long ibt_bitmap_addr;
> + unsigned long ibt_bitmap_size;
> unsigned int shstk_enabled:1;
> + unsigned int ibt_enabled:1;
> };
Is there a reason we're not using pointers here? This seems like the
kind of place that we probably want __user pointers.
> +static unsigned long ibt_mmap(unsigned long addr, unsigned long len)
> +{
> + struct mm_struct *mm = current->mm;
> + unsigned long populate;
> +
> + down_write(&mm->mmap_sem);
> + addr = do_mmap(NULL, addr, len, PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE,
> + VM_DONTDUMP, 0, &populate, NULL);
> + up_write(&mm->mmap_sem);
> +
> + if (populate)
> + mm_populate(addr, populate);
> +
> + return addr;
> +}
We're going to have to start consolidating these at some point. We have
at least three of them now, maybe more.
> +int cet_setup_ibt_bitmap(void)
> +{
> + u64 r;
> + unsigned long bitmap;
> + unsigned long size;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> + return -EOPNOTSUPP;
> +
> + size = TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE;
Just a note: this table is going to be gigantic on 5-level paging
systems, and userspace won't, by default use any of that extra address
space. I think it ends up being a 512GB allocation in a 128TB address
space.
Is that a problem?
On 5-level paging systems, maybe we should just stick it up in the high
part of the address space.
> + bitmap = ibt_mmap(0, size);
> +
> + if (bitmap >= TASK_SIZE_MAX)
> + return -ENOMEM;
> +
> + bitmap &= PAGE_MASK;
We're page-aligning the result of an mmap()? Why?
> + rdmsrl(MSR_IA32_U_CET, r);
> + r |= (MSR_IA32_CET_LEG_IW_EN | bitmap);
> + wrmsrl(MSR_IA32_U_CET, r);
Comments, please. What is this doing, logically? Also, why are we
OR'ing the results into this MSR? What are we trying to preserve?
> + current->thread.cet.ibt_bitmap_addr = bitmap;
> + current->thread.cet.ibt_bitmap_size = size;
> + return 0;
> +}
> +
> +void cet_disable_ibt(void)
> +{
> + u64 r;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> + return;
Does this need a check for being already disabled?
> + rdmsrl(MSR_IA32_U_CET, r);
> + r &= ~(MSR_IA32_CET_ENDBR_EN | MSR_IA32_CET_LEG_IW_EN |
> + MSR_IA32_CET_NO_TRACK_EN);
> + wrmsrl(MSR_IA32_U_CET, r);
> + current->thread.cet.ibt_enabled = 0;
> +}
What's the locking for current->thread.cet?
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 705467839ce8..c609c9ce5691 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -413,7 +413,8 @@ __setup("nopku", setup_disable_pku);
>
> static __always_inline void setup_cet(struct cpuinfo_x86 *c)
> {
> - if (cpu_feature_enabled(X86_FEATURE_SHSTK))
> + if (cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> + cpu_feature_enabled(X86_FEATURE_IBT))
> cr4_set_bits(X86_CR4_CET);
> }
>
> @@ -434,6 +435,23 @@ static __init int setup_disable_shstk(char *s)
> __setup("no_cet_shstk", setup_disable_shstk);
> #endif
>
> +#ifdef CONFIG_X86_INTEL_BRANCH_TRACKING_USER
> +static __init int setup_disable_ibt(char *s)
> +{
> + /* require an exact match without trailing characters */
> + if (strlen(s))
> + return 0;
> +
> + if (!boot_cpu_has(X86_FEATURE_IBT))
> + return 1;
> +
> + setup_clear_cpu_cap(X86_FEATURE_IBT);
> + pr_info("x86: 'no_cet_ibt' specified, disabling Branch Tracking\n");
> + return 1;
> +}
> +__setup("no_cet_ibt", setup_disable_ibt);
> +#endif
> /*
> * Some CPU features depend on higher CPUID levels, which may not always
> * be available due to CPUID level capping or broken virtualization
> diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> index 233f6dad9c1f..42e08d3b573e 100644
> --- a/arch/x86/kernel/elf.c
> +++ b/arch/x86/kernel/elf.c
> @@ -15,6 +15,7 @@
> #include <linux/fs.h>
> #include <linux/uaccess.h>
> #include <linux/string.h>
> +#include <linux/compat.h>
>
> /*
> * The .note.gnu.property layout:
> @@ -222,7 +223,8 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
>
> struct elf64_hdr *ehdr64 = ehdr_p;
>
> - if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> + !cpu_feature_enabled(X86_FEATURE_IBT))
> return 0;
>
> if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
> @@ -250,6 +252,9 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
> current->thread.cet.shstk_enabled = 0;
> current->thread.cet.shstk_base = 0;
> current->thread.cet.shstk_size = 0;
> + current->thread.cet.ibt_enabled = 0;
> + current->thread.cet.ibt_bitmap_addr = 0;
> + current->thread.cet.ibt_bitmap_size = 0;
> if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> if (shstk) {
> err = cet_setup_shstk();
> @@ -257,6 +262,15 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
> goto out;
> }
> }
> +
> + if (cpu_feature_enabled(X86_FEATURE_IBT)) {
> + if (ibt) {
> + err = cet_setup_ibt();
> + if (err < 0)
> + goto out;
> + }
> + }
You introduced 'ibt' before it was used. Please wait to introduce it
until you actually use it to make it easier to review.
Also, what's wrong with:
if (cpu_feature_enabled(X86_FEATURE_IBT) && ibt) {
...
}
?
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-10 23:59 UTC (permalink / raw)
To: David Howells
Cc: viro, linux-api, linux-fsdevel, torvalds, linux-kernel, jannh
In-Reply-To: <153126264966.14533.3388004240803696769.stgit@warthog.procyon.org.uk>
[cc Jann - you love this stuff]
> On Jul 10, 2018, at 3:44 PM, David Howells <dhowells@redhat.com> wrote:
>
> Provide an fsopen() system call that starts the process of preparing to
> create a superblock that will then be mountable, using an fd as a context
> handle. fsopen() is given the name of the filesystem that will be used:
>
> int mfd = fsopen(const char *fsname, unsigned int flags);
This is great in principle, but I think you’re seriously playing with fire with the API.
>
> where flags can be 0 or FSOPEN_CLOEXEC.
>
> For example:
>
> sfd = fsopen("ext4", FSOPEN_CLOEXEC);
> write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
Imagine some malicious program passes sfd as stdout to a setuid program. That program gets persuaded to write “s /etc/shadow”. What happens? You’re okay as long as *every single fs* gets it right, but that’s asking a lot.
> write(sfd, "o noatime");
> write(sfd, "o acl");
> write(sfd, "o user_attr");
> write(sfd, "o iversion");
> write(sfd, "o ");
> write(sfd, "r /my/container"); // root inside the fs
> write(sfd, "x create"); // create the superblock
From cursory inspection of a bunch of the code, I think the expectation is that the actual device access happens in the “x” action. This is not okay. You can’t do this kind of thing in a write() handler, unless you somehow make every single access using f_cred, which is a real pain.
I think the right solution is one of:
(a) Pass a netlink-formatted blob to fsopen() and do the whole thing in one syscall. I don’t mean using netlink sockets — just the nlattr format. Or you could use a different format. The part that matters is using just one syscall to do the whole thing.
(b) Keep the current structure but use a new syscall instead of write().
(c) Keep using write() but literally just buffer the data. Then have a new syscall to commit it. In other words, replace “x” with a syscall and call all the fs_context_operations helpers in that context instead of from write().
^ permalink raw reply
* Re: [RFC PATCH v2 23/27] mm/mmap: Add IBT bitmap size to address space limit check
From: Dave Hansen @ 2018-07-10 23:57 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-24-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> The indirect branch tracking legacy bitmap takes a large address
> space. This causes may_expand_vm() failure on the address limit
> check. For a IBT-enabled task, add the bitmap size to the
> address limit.
This appears to require that we set up
current->thread.cet.ibt_bitmap_size _before_ calling may_expand_vm().
What keeps the ibt_mmap() itself from hitting the address limit?
^ permalink raw reply
* Re: [RFC PATCH v2 11/27] x86/mm: Modify ptep_set_wrprotect and pmdp_set_wrprotect for _PAGE_DIRTY_SW
From: Dave Hansen @ 2018-07-10 23:52 UTC (permalink / raw)
To: Nadav Amit, Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Andy Lutomirski, Balbir Singh,
Cyrill Gorcunov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Oleg Nesterov,
Pavel Machek, Peter Zijlstra <pet>
In-Reply-To: <DDFF4AF4-51D2-44F5-8B63-E8454F712EC6@gmail.com>
On 07/10/2018 04:23 PM, Nadav Amit wrote:
> at 6:44 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
>> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
>>> + /*
>>> + * On platforms before CET, other threads could race to
>>> + * create a RO and _PAGE_DIRTY_HW PMD again. However,
>>> + * on CET platforms, this is safe without a TLB flush.
>>> + */
>>
>> If I didn't work for Intel, I'd wonder what the heck CET is and what the
>> heck it has to do with _PAGE_DIRTY_HW. I think we need a better comment
>> than this. How about:
>>
>> Some processors can _start_ a write, but end up seeing
>> a read-only PTE by the time they get to getting the
>> Dirty bit. In this case, they will set the Dirty bit,
>> leaving a read-only, Dirty PTE which looks like a Shadow
>> Stack PTE.
>>
>> However, this behavior has been improved and will *not* occur on
>> processors supporting Shadow Stacks. Without this guarantee, a
>> transition to a non-present PTE and flush the TLB would be
>> needed.
>
> Interesting. Does that regard the knights landing bug or something more
> general?
It's more general.
> Will the write succeed or trigger a page-fault in this case?
It will trigger a page fault.
^ permalink raw reply
* Re: [RFC PATCH v2 18/27] x86/cet/shstk: Introduce WRUSS instruction
From: Dave Hansen @ 2018-07-10 23:48 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-19-yu-cheng.yu@intel.com>
> +/*
> + * WRUSS is a kernel instrcution and but writes to user
> + * shadow stack memory. When a fault occurs, both
> + * X86_PF_USER and X86_PF_SHSTK are set.
> + */
> +static int is_wruss(struct pt_regs *regs, unsigned long error_code)
> +{
> + return (((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
> + (X86_PF_USER | X86_PF_SHSTK)) && !user_mode(regs));
> +}
I thought X86_PF_USER was set based on the mode in which the fault
occurred. Does this mean that the architecture of this bit is different
now?
That seems like something we need to call out if so. It also means we
need to update the SDM because some of the text is wrong.
> static void
> show_fault_oops(struct pt_regs *regs, unsigned long error_code,
> unsigned long address)
> @@ -848,7 +859,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
> struct task_struct *tsk = current;
>
> /* User mode accesses just cause a SIGSEGV */
> - if (error_code & X86_PF_USER) {
> + if ((error_code & X86_PF_USER) && !is_wruss(regs, error_code)) {
> /*
> * It's possible to have interrupts off here:
> */
This needs commenting about why is_wruss() is special.
^ permalink raw reply
* Re: [RFC PATCH v2 17/27] x86/cet/shstk: User-mode shadow stack support
From: Dave Hansen @ 2018-07-10 23:40 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-18-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> +static __init int setup_disable_shstk(char *s)
> +{
> + /* require an exact match without trailing characters */
> + if (strlen(s))
> + return 0;
> +
> + if (!boot_cpu_has(X86_FEATURE_SHSTK))
> + return 1;
> +
> + setup_clear_cpu_cap(X86_FEATURE_SHSTK);
> + pr_info("x86: 'no_cet_shstk' specified, disabling Shadow Stack\n");
> + return 1;
> +}
> +__setup("no_cet_shstk", setup_disable_shstk);
Why do we need a boot-time disable for this?
^ permalink raw reply
* Re: [RFC PATCH v2 16/27] mm: Modify can_follow_write_pte/pmd for shadow stack
From: Dave Hansen @ 2018-07-10 23:37 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-17-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> There are three possible shadow stack PTE settings:
>
> Normal SHSTK PTE: (R/O + DIRTY_HW)
> SHSTK PTE COW'ed: (R/O + DIRTY_HW)
> SHSTK PTE shared as R/O data: (R/O + DIRTY_SW)
>
> Update can_follow_write_pte/pmd for the shadow stack.
First of all, thanks for the excellent patch headers. It's nice to have
that reference every time even though it's repeated.
> -static inline bool can_follow_write_pte(pte_t pte, unsigned int flags)
> +static inline bool can_follow_write_pte(pte_t pte, unsigned int flags,
> + bool shstk)
> {
> + bool pte_cowed = shstk ? is_shstk_pte(pte):pte_dirty(pte);
> +
> return pte_write(pte) ||
> - ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pte_dirty(pte));
> + ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pte_cowed);
> }
Can we just pass the VMA in here? This use is OK-ish, but I generally
detest true/false function arguments because you can't tell what they
are when they show up without a named variable.
But... Why does this even matter? Your own example showed that all
shadowstack PTEs have either DIRTY_HW or DIRTY_SW set, and pte_dirty()
checks both.
That makes this check seem a bit superfluous.
^ permalink raw reply
* Re: [RFC PATCH v2 12/27] x86/mm: Shadow stack page fault error checking
From: Dave Hansen @ 2018-07-10 23:24 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-13-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> + /*
> + * Verify X86_PF_SHSTK is within a shadow stack VMA.
> + * It is always an error if there is a shadow stack
> + * fault outside a shadow stack VMA.
> + */
> + if (error_code & X86_PF_SHSTK) {
> + if (!(vma->vm_flags & VM_SHSTK))
> + return 1;
> + return 0;
> + }
It turns out that a X86_PF_SHSTK just means that the processor faulted
while doing access to something it thinks should be a shadow-stack
virtual address.
But, we *can* have faults on shadow stack accesses for non-shadow-stack
reasons.
I think you need to remove the 'return 0' and let it fall through to the
other access checks that we might be failing. If it's a shadow stack
access, it has to be a shadow stack VMA. But, a shadow-stack access
fault to a shadow stack VMA isn't _necessarily_ OK.
^ permalink raw reply
* Re: [RFC PATCH v2 11/27] x86/mm: Modify ptep_set_wrprotect and pmdp_set_wrprotect for _PAGE_DIRTY_SW
From: Nadav Amit @ 2018-07-10 23:23 UTC (permalink / raw)
To: Dave Hansen, Yu-cheng Yu, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Oleg Nesterov, Pavel Machek
Cc: X86 ML, Linux Kernel Mailing List
In-Reply-To: <fbf45667-5388-44a6-1f22-07bcc03e1804@linux.intel.com>
at 6:44 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
>> + /*
>> + * On platforms before CET, other threads could race to
>> + * create a RO and _PAGE_DIRTY_HW PMD again. However,
>> + * on CET platforms, this is safe without a TLB flush.
>> + */
>
> If I didn't work for Intel, I'd wonder what the heck CET is and what the
> heck it has to do with _PAGE_DIRTY_HW. I think we need a better comment
> than this. How about:
>
> Some processors can _start_ a write, but end up seeing
> a read-only PTE by the time they get to getting the
> Dirty bit. In this case, they will set the Dirty bit,
> leaving a read-only, Dirty PTE which looks like a Shadow
> Stack PTE.
>
> However, this behavior has been improved and will *not* occur on
> processors supporting Shadow Stacks. Without this guarantee, a
> transition to a non-present PTE and flush the TLB would be
> needed.
Interesting. Does that regard the knights landing bug or something more
general?
Will the write succeed or trigger a page-fault in this case?
[ I know it is not related to the patch, but I would appreciate if you share
your knowledge ]
Regards,
Nadav
^ permalink raw reply
* Re: [RFC PATCH v2 15/27] mm/mprotect: Prevent mprotect from changing shadow stack
From: Dave Hansen @ 2018-07-10 23:10 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-16-yu-cheng.yu@intel.com>
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?
^ permalink raw reply
* Re: [RFC PATCH v2 14/27] mm: Handle THP/HugeTLB shadow stack page fault
From: Dave Hansen @ 2018-07-10 23:08 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-15-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> @@ -1347,6 +1353,8 @@ int do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
> pmd_t entry;
> entry = mk_huge_pmd(new_page, vma->vm_page_prot);
> entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> + if (is_shstk_mapping(vma->vm_flags))
> + entry = pmd_mkdirty_shstk(entry);
This pattern is repeated enough that it makes me wonder if we should
just be doing the shadowstack PTE creation in mk_huge_pmd() itself.
Or, should we just be setting the shadowstack pte bit combination in
vma->vm_page_prot so we don't have to go set it explicitly every time?
^ permalink raw reply
* Re: [RFC PATCH v2 13/27] mm: Handle shadow stack page fault
From: Dave Hansen @ 2018-07-10 23:06 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-14-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> + if (is_shstk_mapping(vma->vm_flags))
> + entry = pte_mkdirty_shstk(entry);
> + else
> + entry = pte_mkdirty(entry);
> +
> + entry = maybe_mkwrite(entry, vma);
> if (ptep_set_access_flags(vma, vmf->address, vmf->pte, entry, 1))
> update_mmu_cache(vma, vmf->address, vmf->pte);
> pte_unmap_unlock(vmf->pte, vmf->ptl);
> @@ -2526,7 +2532,11 @@ static int wp_page_copy(struct vm_fault *vmf)
> }
> flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));
> entry = mk_pte(new_page, vma->vm_page_prot);
> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> + if (is_shstk_mapping(vma->vm_flags))
> + entry = pte_mkdirty_shstk(entry);
> + else
> + entry = pte_mkdirty(entry);
> + entry = maybe_mkwrite(entry, vma);
Do we want to lift this hunk of code and put it elsewhere? Maybe:
entry = pte_set_vma_features(entry, vma);
and then:
pte_t pte_set_vma_features(pte_t entry, struct vm_area_struct)
{
/*
* Shadow stack PTEs are always dirty and always
* writable. They have a different encoding for
* this than normal PTEs, though.
*/
if (is_shstk_mapping(vma->vm_flags))
entry = pte_mkdirty_shstk(entry);
else
entry = pte_mkdirty(entry);
entry = maybe_mkwrite(entry, vma);
return entry;
}
> /*
> * Clear the pte entry and flush it first, before updating the
> * pte with the new entry. This will avoid a race condition
> @@ -3201,6 +3211,14 @@ static int do_anonymous_page(struct vm_fault *vmf)
> mem_cgroup_commit_charge(page, memcg, false, false);
> lru_cache_add_active_or_unevictable(page, vma);
> setpte:
> + /*
> + * If this is within a shadow stack mapping, mark
> + * the PTE dirty. We don't use pte_mkdirty(),
> + * because the PTE must have _PAGE_DIRTY_HW set.
> + */
> + if (is_shstk_mapping(vma->vm_flags))
> + entry = pte_mkdirty_shstk(entry);
> +
> set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
I'm not sure this is the right spot to do this.
The other code does pte_mkdirty_shstk() near where we do the
pte_mkwrite(). Why not here? I think you might have picked this
because it's a common path used by both allocated pages and zero pages.
But, we can't have the zero pages in shadow stack areas since they can't
be read-only. I think you need to move this up. Can you even
consolidate it with the other two pte_mkdirt_shstk() call sites?
> /* No need to invalidate - it was non-present before */
> @@ -3983,6 +4001,14 @@ static int handle_pte_fault(struct vm_fault *vmf)
> entry = vmf->orig_pte;
> if (unlikely(!pte_same(*vmf->pte, entry)))
> goto unlock;
> +
> + /*
> + * Shadow stack PTEs are copy-on-access, so do_wp_page()
> + * handling on them no matter if we have write fault or not.
> + */
I'd say this differently:
Shadow stack PTEs can not be read-only and because of that can
not have traditional copy-on-write semantics. This essentially
performs a copy-on-write operation, but on *any* access, not
just actual writes.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox