| Message ID | 20230119212317.8324-24-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On 19.01.23 22:23, Rick Edgecombe wrote: > The x86 Control-flow Enforcement Technology (CET) feature includes a new > type of memory called shadow stack. This shadow stack memory has some > unusual properties, which requires some core mm changes to function > properly. > > Shadow stack memory is writable only in very specific, controlled ways. > However, since it is writable, the kernel treats it as such. As a result > there remain many ways for userspace to trigger the kernel to write to > shadow stack's via get_user_pages(, FOLL_WRITE) operations. To make this a > little less exposed, block writable GUPs for shadow stack VMAs. > > Still allow FOLL_FORCE to write through shadow stack protections, as it > does for read-only protections. So an app can simply modify the shadow stack itself by writing to /proc/self/mem ? Is that really intended? Looks like security hole to me at first sight, but maybe I am missing something important.
* David Hildenbrand: > On 19.01.23 22:23, Rick Edgecombe wrote: >> The x86 Control-flow Enforcement Technology (CET) feature includes a new >> type of memory called shadow stack. This shadow stack memory has some >> unusual properties, which requires some core mm changes to function >> properly. >> Shadow stack memory is writable only in very specific, controlled >> ways. >> However, since it is writable, the kernel treats it as such. As a result >> there remain many ways for userspace to trigger the kernel to write to >> shadow stack's via get_user_pages(, FOLL_WRITE) operations. To make this a >> little less exposed, block writable GUPs for shadow stack VMAs. >> Still allow FOLL_FORCE to write through shadow stack protections, as >> it >> does for read-only protections. > > So an app can simply modify the shadow stack itself by writing to > /proc/self/mem ? > > Is that really intended? Looks like security hole to me at first > sight, but maybe I am missing something important. Isn't it possible to overwrite GOT pointers using the same vector? So I think it's merely reflecting the status quo. Thanks, Florian
On Mon, 2023-01-23 at 11:45 +0100, Florian Weimer wrote: > * David Hildenbrand: > > > On 19.01.23 22:23, Rick Edgecombe wrote: > > > The x86 Control-flow Enforcement Technology (CET) feature > > > includes a new > > > type of memory called shadow stack. This shadow stack memory has > > > some > > > unusual properties, which requires some core mm changes to > > > function > > > properly. > > > Shadow stack memory is writable only in very specific, controlled > > > ways. > > > However, since it is writable, the kernel treats it as such. As a > > > result > > > there remain many ways for userspace to trigger the kernel to > > > write to > > > shadow stack's via get_user_pages(, FOLL_WRITE) operations. To > > > make this a > > > little less exposed, block writable GUPs for shadow stack VMAs. > > > Still allow FOLL_FORCE to write through shadow stack protections, > > > as > > > it > > > does for read-only protections. > > > > So an app can simply modify the shadow stack itself by writing to > > /proc/self/mem ? > > > > Is that really intended? Looks like security hole to me at first > > sight, but maybe I am missing something important. > > Isn't it possible to overwrite GOT pointers using the same vector? > So I think it's merely reflecting the status quo. There was some debate on this. /proc/self/mem can currently write through read-only memory which protects executable code. So should shadow stack get separate rules? Is ROP a worry when you can overwrite executable code? The consensus seemed to lean towards not making special rules for this case, and there was some discussion that /proc/self/mem should maybe be hardened generally.
On 23.01.23 21:46, Edgecombe, Rick P wrote: > On Mon, 2023-01-23 at 11:45 +0100, Florian Weimer wrote: >> * David Hildenbrand: >> >>> On 19.01.23 22:23, Rick Edgecombe wrote: >>>> The x86 Control-flow Enforcement Technology (CET) feature >>>> includes a new >>>> type of memory called shadow stack. This shadow stack memory has >>>> some >>>> unusual properties, which requires some core mm changes to >>>> function >>>> properly. >>>> Shadow stack memory is writable only in very specific, controlled >>>> ways. >>>> However, since it is writable, the kernel treats it as such. As a >>>> result >>>> there remain many ways for userspace to trigger the kernel to >>>> write to >>>> shadow stack's via get_user_pages(, FOLL_WRITE) operations. To >>>> make this a >>>> little less exposed, block writable GUPs for shadow stack VMAs. >>>> Still allow FOLL_FORCE to write through shadow stack protections, >>>> as >>>> it >>>> does for read-only protections. >>> >>> So an app can simply modify the shadow stack itself by writing to >>> /proc/self/mem ? >>> >>> Is that really intended? Looks like security hole to me at first >>> sight, but maybe I am missing something important. >> >> Isn't it possible to overwrite GOT pointers using the same vector? >> So I think it's merely reflecting the status quo. > > There was some debate on this. /proc/self/mem can currently write > through read-only memory which protects executable code. So should > shadow stack get separate rules? Is ROP a worry when you can overwrite > executable code? > The question is, if there is reasonable debugging reason to keep it. I assume if a debugger would adjust the ordinary stack, it would have to adjust the shadow stack as well (oh my ...). So it sounds reasonable to have it in theory at least ... not sure when debugger would support that, but maybe they already do. > The consensus seemed to lean towards not making special rules for this > case, and there was some discussion that /proc/self/mem should maybe be > hardened generally. I agree with that. It's a debugging mechanism that a process can abuse to do nasty stuff to its memory that it maybe shouldn't be able to do ...
Ping Cristina regarding GDB. Ping Kees regarding /proc/self/mem. On Tue, 2023-01-24 at 17:26 +0100, David Hildenbrand wrote: > > > Isn't it possible to overwrite GOT pointers using the same > > > vector? > > > So I think it's merely reflecting the status quo. > > > > There was some debate on this. /proc/self/mem can currently write > > through read-only memory which protects executable code. So should > > shadow stack get separate rules? Is ROP a worry when you can > > overwrite > > executable code? > > > > The question is, if there is reasonable debugging reason to keep it. > I > assume if a debugger would adjust the ordinary stack, it would have > to > adjust the shadow stack as well (oh my ...). So it sounds reasonable > to > have it in theory at least ... not sure when debugger would support > that, but maybe they already do. GDB support for shadow stack is queued up for whenever the kernel interface settles. I believe it just uses ptrace, and not this proc. But yea ptrace poke will still need to use FOLL_FORCE and be able to write through shadow stacks. > > > The consensus seemed to lean towards not making special rules for > > this > > case, and there was some discussion that /proc/self/mem should > > maybe be > > hardened generally. > > I agree with that. It's a debugging mechanism that a process can > abuse > to do nasty stuff to its memory that it maybe shouldn't be able to do > ... Ok.
On January 24, 2023 10:42:28 AM PST, "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote: >Ping Cristina regarding GDB. > >Ping Kees regarding /proc/self/mem. > >On Tue, 2023-01-24 at 17:26 +0100, David Hildenbrand wrote: >> > > Isn't it possible to overwrite GOT pointers using the same >> > > vector? >> > > So I think it's merely reflecting the status quo. >> > >> > There was some debate on this. /proc/self/mem can currently write >> > through read-only memory which protects executable code. So should >> > shadow stack get separate rules? Is ROP a worry when you can >> > overwrite >> > executable code? >> > >> >> The question is, if there is reasonable debugging reason to keep it. >> I >> assume if a debugger would adjust the ordinary stack, it would have >> to >> adjust the shadow stack as well (oh my ...). So it sounds reasonable >> to >> have it in theory at least ... not sure when debugger would support >> that, but maybe they already do. > >GDB support for shadow stack is queued up for whenever the kernel >interface settles. I believe it just uses ptrace, and not this proc. >But yea ptrace poke will still need to use FOLL_FORCE and be able to >write through shadow stacks. I'd prefer to avoid adding more FOLL_FORCE if we can. If gdb can do stack manipulations through a ptrace interface then let's leave off FOLL_FORCE. -Kees > >> >> > The consensus seemed to lean towards not making special rules for >> > this >> > case, and there was some discussion that /proc/self/mem should >> > maybe be >> > hardened generally. >> >> I agree with that. It's a debugging mechanism that a process can >> abuse >> to do nasty stuff to its memory that it maybe shouldn't be able to do >> ... > >Ok.
On Tue, 2023-01-24 at 15:08 -0800, Kees Cook wrote: > > GDB support for shadow stack is queued up for whenever the kernel > > interface settles. I believe it just uses ptrace, and not this > > proc. > > But yea ptrace poke will still need to use FOLL_FORCE and be able > > to > > write through shadow stacks. > > I'd prefer to avoid adding more FOLL_FORCE if we can. If gdb can do > stack manipulations through a ptrace interface then let's leave off > FOLL_FORCE. Ptrace and /proc/self/mem both use FOLL_FORCE. I think ptrace will always need it or something like it for debugging. To jog your memory, this series doesn't change what uses FOLL_FORCE. It just sets the shadow stack rules to be the same as read-only memory. So even though shadow stack memory is sort of writable, it's a bit more locked down and FOLL_FORCE is required to write to it with GUP. If we just remove FOLL_FORCE from /proc/self/mem, something will probably break right? How do we do this? Some sort of opt-in?
On 25.01.23 00:41, Edgecombe, Rick P wrote: > On Tue, 2023-01-24 at 15:08 -0800, Kees Cook wrote: >>> GDB support for shadow stack is queued up for whenever the kernel >>> interface settles. I believe it just uses ptrace, and not this >>> proc. >>> But yea ptrace poke will still need to use FOLL_FORCE and be able >>> to >>> write through shadow stacks. >> >> I'd prefer to avoid adding more FOLL_FORCE if we can. If gdb can do >> stack manipulations through a ptrace interface then let's leave off >> FOLL_FORCE. > > Ptrace and /proc/self/mem both use FOLL_FORCE. I think ptrace will > always need it or something like it for debugging. > > To jog your memory, this series doesn't change what uses FOLL_FORCE. It > just sets the shadow stack rules to be the same as read-only memory. So > even though shadow stack memory is sort of writable, it's a bit more > locked down and FOLL_FORCE is required to write to it with GUP. > > If we just remove FOLL_FORCE from /proc/self/mem, something will > probably break right? How do we do this? Some sort of opt-in? I don't think removing that is an option. It's another debug interface that has been allowing such access for ever ... Blocking /proc/self/mem access completely for selected processes might be the better alternative.
On January 25, 2023 1:29:20 AM PST, David Hildenbrand <david@redhat.com> wrote: >On 25.01.23 00:41, Edgecombe, Rick P wrote: >> On Tue, 2023-01-24 at 15:08 -0800, Kees Cook wrote: >>>> GDB support for shadow stack is queued up for whenever the kernel >>>> interface settles. I believe it just uses ptrace, and not this >>>> proc. >>>> But yea ptrace poke will still need to use FOLL_FORCE and be able >>>> to >>>> write through shadow stacks. >>> >>> I'd prefer to avoid adding more FOLL_FORCE if we can. If gdb can do >>> stack manipulations through a ptrace interface then let's leave off >>> FOLL_FORCE. >> >> Ptrace and /proc/self/mem both use FOLL_FORCE. I think ptrace will >> always need it or something like it for debugging. >> >> To jog your memory, this series doesn't change what uses FOLL_FORCE. It >> just sets the shadow stack rules to be the same as read-only memory. So >> even though shadow stack memory is sort of writable, it's a bit more >> locked down and FOLL_FORCE is required to write to it with GUP. >> >> If we just remove FOLL_FORCE from /proc/self/mem, something will >> probably break right? How do we do this? Some sort of opt-in? > >I don't think removing that is an option. It's another debug interface that has been allowing such access for ever ... > >Blocking /proc/self/mem access completely for selected processes might be the better alternative. > Yeah, this would be nice. Kind of like being undumpable or no_new_privs.
> On Tue, 2023-01-24 at 17:26 +0100, David Hildenbrand wrote: > > > > Isn't it possible to overwrite GOT pointers using the same vector? > > > > So I think it's merely reflecting the status quo. > > > > > > There was some debate on this. /proc/self/mem can currently write > > > through read-only memory which protects executable code. So should > > > shadow stack get separate rules? Is ROP a worry when you can > > > overwrite executable code? > > > > > > > The question is, if there is reasonable debugging reason to keep it. > > I > > assume if a debugger would adjust the ordinary stack, it would have to > > adjust the shadow stack as well (oh my ...). So it sounds reasonable > > to have it in theory at least ... not sure when debugger would support > > that, but maybe they already do. > > GDB support for shadow stack is queued up for whenever the kernel > interface settles. I believe it just uses ptrace, and not this proc. > But yea ptrace poke will still need to use FOLL_FORCE and be able to write > through shadow stacks. Our patches for GDB use /proc/PID/mem to read/write shadow stack memory. However, I think it should be possible to change this to ptrace but GDB normally uses /proc/PID/mem to read/write target memory. Regards, Christina Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
> > On Tue, 2023-01-24 at 17:26 +0100, David Hildenbrand wrote: > > > > > Isn't it possible to overwrite GOT pointers using the same vector? > > > > > So I think it's merely reflecting the status quo. > > > > > > > > There was some debate on this. /proc/self/mem can currently write > > > > through read-only memory which protects executable code. So should > > > > shadow stack get separate rules? Is ROP a worry when you can > > > > overwrite executable code? > > > > > > > > > > The question is, if there is reasonable debugging reason to keep it. > > > I > > > assume if a debugger would adjust the ordinary stack, it would have > > > to adjust the shadow stack as well (oh my ...). So it sounds > > > reasonable to have it in theory at least ... not sure when debugger > > > would support that, but maybe they already do. > > > > GDB support for shadow stack is queued up for whenever the kernel > > interface settles. I believe it just uses ptrace, and not this proc. > > But yea ptrace poke will still need to use FOLL_FORCE and be able to > > write through shadow stacks. > > Our patches for GDB use /proc/PID/mem to read/write shadow stack > memory. > However, I think it should be possible to change this to ptrace but GDB > normally uses /proc/PID/mem to read/write target memory. > > Regards, > Christina I just noticed that GDBSERVER actually uses ptrace, so our patches currently use both: ptrace and proc/PID/mem to read/write shadow stack memory. Regards, Christina Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 87d3068734ec..425ded5dd6ec 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -1671,6 +1671,11 @@ static inline bool __pte_access_permitted(unsigned long pteval, bool write) { unsigned long need_pte_bits = _PAGE_PRESENT|_PAGE_USER; + /* + * Write=0,Dirty=1 PTEs are shadow stack, which the kernel + * shouldn't generally allow access to, but since they + * are already Write=0, the below logic covers both cases. + */ if (write) need_pte_bits |= _PAGE_RW; diff --git a/mm/gup.c b/mm/gup.c index f45a3a5be53a..bfd33d9edb89 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -982,7 +982,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) return -EFAULT; if (write) { - if (!(vm_flags & VM_WRITE)) { + if (!(vm_flags & VM_WRITE) || (vm_flags & VM_SHADOW_STACK)) { if (!(gup_flags & FOLL_FORCE)) return -EFAULT; /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */