| Message ID | 20240717111358.415712-1-adrian.ratiu@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | proc: add config to block FOLL_FORCE in mem writes | expand |
On Wed, Jul 17, 2024 at 02:13:58PM +0300, Adrian Ratiu wrote: > This simple Kconfig option removes the FOLL_FORCE flag from > procfs write calls because it can be abused. For this to be available for general distros, I still want to have a bootparam to control this, otherwise this mitigation will never see much testing as most kernel deployments don't build their own kernels. A simple __ro_after_init variable can be used. In the future if folks want a more flexible version, we could make this a one-way per-process flag, like no_new_privs.
On Wed, 17 Jul 2024 at 10:23, Kees Cook <kees@kernel.org> wrote: > > For this to be available for general distros, I still want to have a > bootparam to control this, otherwise this mitigation will never see much > testing as most kernel deployments don't build their own kernels. A > simple __ro_after_init variable can be used. Oh, btw, I looked at the FOLL_FORCE back in 2017 when we did this: 8ee74a91ac30 ("proc: try to remove use of FOLL_FORCE entirely") and then we had to undo that with f511c0b17b08 (""Yes, people use FOLL_FORCE ;)"") but at the time I also had an experimental patch that worked for me, but I seem to have only sent that out in private to the people involved with the original issue. And then that whole discussion petered out, and nothing happened. But maybe we can try again. In particular, while people piped up about other uses (see the quotes in that commit f511c0b17b08) they were fairly rare and specialized. The one *common* use was gdb. But my old diff from years ago mostly still applies, so I resurrected it. It basically restricts FOLL_FORCE to just ptracers. That's *not* good for some of the people that piped up back when (eg Julia JIT), but it might be a more palatable halfway state. In particular, this patch would make it easy to make that SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice" where you pick "never, ptrace, always" by just changing the rules in proc_is_ptracing(). I guess that function should be renamed too, I only did a minimal "forward-port an old patch" thing. Linus
On Wed, Jul 17, 2024 at 02:13:58PM +0300, Adrian Ratiu wrote: > +config SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE > + bool "Remove FOLL_FORCE usage from /proc/pid/mem writes" > + default n > + help > + This restricts FOLL_FORCE flag usage in procfs mem write calls > + because it bypasses memory permission checks and can be used by > + attackers to manipulate process memory contents that would be > + otherwise protected. > + > + Enabling this will break GDB, gdbserver and other debuggers > + which require FOLL_FORCE for basic functionalities. > + > + If you are unsure how to answer this question, answer N. FOLL_FORCE is an internal flag, and people who aren't kernel developers aren't going to know what it is. Could this option be named and documented in a way that would be more understandable to people who aren't kernel developers? What is the effect on how /proc/pid/mem behaves? - Eric
On Wed, Jul 17, 2024 at 01:53:35PM -0700, Eric Biggers wrote: > On Wed, Jul 17, 2024 at 02:13:58PM +0300, Adrian Ratiu wrote: > > +config SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE > > + bool "Remove FOLL_FORCE usage from /proc/pid/mem writes" > > + default n > > + help > > + This restricts FOLL_FORCE flag usage in procfs mem write calls > > + because it bypasses memory permission checks and can be used by > > + attackers to manipulate process memory contents that would be > > + otherwise protected. > > + > > + Enabling this will break GDB, gdbserver and other debuggers > > + which require FOLL_FORCE for basic functionalities. > > + > > + If you are unsure how to answer this question, answer N. > > FOLL_FORCE is an internal flag, and people who aren't kernel developers aren't > going to know what it is. Could this option be named and documented in a way > that would be more understandable to people who aren't kernel developers? What > is the effect on how /proc/pid/mem behaves? "Do not bypass RO memory permissions via /proc/$pid/mem writes" ?
On Wed, Jul 17, 2024 at 11:16:56AM -0700, Linus Torvalds wrote: > On Wed, 17 Jul 2024 at 10:23, Kees Cook <kees@kernel.org> wrote: > > > > For this to be available for general distros, I still want to have a > > bootparam to control this, otherwise this mitigation will never see much > > testing as most kernel deployments don't build their own kernels. A > > simple __ro_after_init variable can be used. > > Oh, btw, I looked at the FOLL_FORCE back in 2017 when we did this: > > 8ee74a91ac30 ("proc: try to remove use of FOLL_FORCE entirely") > > and then we had to undo that with > > f511c0b17b08 (""Yes, people use FOLL_FORCE ;)"") > > but at the time I also had an experimental patch that worked for me, > but I seem to have only sent that out in private to the people > involved with the original issue. > > And then that whole discussion petered out, and nothing happened. > > But maybe we can try again. > > In particular, while people piped up about other uses (see the quotes > in that commit f511c0b17b08) they were fairly rare and specialized. > > The one *common* use was gdb. > > But my old diff from years ago mostly still applies, so I resurrected it. > > It basically restricts FOLL_FORCE to just ptracers. > > That's *not* good for some of the people that piped up back when (eg > Julia JIT), but it might be a more palatable halfway state. > > In particular, this patch would make it easy to make that > SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice" > where you pick "never, ptrace, always" by just changing the rules in > proc_is_ptracing(). So the original patch could be reduced to just the single tristate option instead of 3 tristates? I think that would be a decent middle ground, and IIUC, will still provide the coverage Chrome OS is looking for[1]. -Kees [1] https://lore.kernel.org/lkml/CABi2SkWDwAU2ARyMVTeCqFeOXyQZn3hbkdWv-1OzzgG=MNoU8Q@mail.gmail.com/
On Wed, 17 Jul 2024 at 15:24, Kees Cook <kees@kernel.org> wrote: > > > In particular, this patch would make it easy to make that > > SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice" > > where you pick "never, ptrace, always" by just changing the rules in > > proc_is_ptracing(). > > So the original patch could be reduced to just the single tristate option > instead of 3 tristates? I think that would be a decent middle ground, > and IIUC, will still provide the coverage Chrome OS is looking for[1]. So here's what I kind of think might be ok. ENTIRELY UNTESTED! This is more of a "look, something like this, perhaps" patch than a real one. If somebody tests this, and it is ok for Chrome OS, you can consider this signed-off-on, but only with actual testing. I might have gotten something hroribly wrong. Linus
On Thursday, July 18, 2024 03:04 EEST, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, 17 Jul 2024 at 15:24, Kees Cook <kees@kernel.org> wrote: > > > > > In particular, this patch would make it easy to make that > > > SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice" > > > where you pick "never, ptrace, always" by just changing the rules in > > > proc_is_ptracing(). > > > > So the original patch could be reduced to just the single tristate option > > instead of 3 tristates? I think that would be a decent middle ground, > > and IIUC, will still provide the coverage Chrome OS is looking for[1]. > > So here's what I kind of think might be ok. > > ENTIRELY UNTESTED! This is more of a "look, something like this, > perhaps" patch than a real one. > > If somebody tests this, and it is ok for Chrome OS, you can consider > this signed-off-on, but only with actual testing. I might have gotten > something hroribly wrong. Thanks for the patch! I tested it on ChromeOS and it does what it intends, just with two minor fixes applied: --- a/security/Kconfig +++ b/security/Kconfig -config CONFIG_PROC_MEM_FORCE_PTRACE +config PROC_MEM_FORCE_PTRACE ..... -config CONFIG_PROC_MEM_NO_FORCE +config PROC_MEM_NO_FORCE As Kees suggested, I'll add a bootparam with a simple __ro_after_init variable to select this and then send a v2 for review.
diff --git a/fs/proc/base.c b/fs/proc/base.c index 72a1acd03675..53ad71d7d785 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -855,7 +855,11 @@ static ssize_t mem_rw(struct file *file, char __user *buf, if (!mmget_not_zero(mm)) goto free; - flags = FOLL_FORCE | (write ? FOLL_WRITE : 0); + flags = (write ? FOLL_WRITE : 0); + +#ifndef CONFIG_SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE + flags |= FOLL_FORCE; +#endif while (count > 0) { size_t this_len = min_t(size_t, count, PAGE_SIZE); diff --git a/security/Kconfig b/security/Kconfig index 412e76f1575d..24053b8ff786 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -19,6 +19,20 @@ config SECURITY_DMESG_RESTRICT If you are unsure how to answer this question, answer N. +config SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE + bool "Remove FOLL_FORCE usage from /proc/pid/mem writes" + default n + help + This restricts FOLL_FORCE flag usage in procfs mem write calls + because it bypasses memory permission checks and can be used by + attackers to manipulate process memory contents that would be + otherwise protected. + + Enabling this will break GDB, gdbserver and other debuggers + which require FOLL_FORCE for basic functionalities. + + If you are unsure how to answer this question, answer N. + config SECURITY bool "Enable different security models" depends on SYSFS
This simple Kconfig option removes the FOLL_FORCE flag from procfs write calls because it can be abused. Enabling it breaks some debuggers like GDB so it defaults off. Previously we tried a more sophisticated approach allowing distributions to fine-tune proc/pid/mem behaviour via both kconfig and boot params, however that got NAK-ed by Linus [1] who prefers this simpler approach. Link: https://lore.kernel.org/lkml/CAHk-=wiGWLChxYmUA5HrT5aopZrB7_2VTa0NLZcxORgkUe5tEQ@mail.gmail.com/ [1] Cc: Doug Anderson <dianders@chromium.org> Cc: Jeff Xu <jeffxu@google.com> Cc: Jann Horn <jannh@google.com> Cc: Kees Cook <kees@kernel.org> Cc: Christian Brauner <brauner@kernel.org> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> --- fs/proc/base.c | 6 +++++- security/Kconfig | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)