| Message ID | 20220510004251.3952265-2-cascardo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | LP: #1972740 Unprivileged users may use PTRACE_SEIZE to set PTRACE_O_SUSPEND_SECCOMP option | expand |
On Mon, 2022-05-09 at 21:42 -0300, Thadeu Lima de Souza Cascardo wrote: > From: Jann Horn <jannh@google.com> > > BugLink: https://bugs.launchpad.net/bugs/1972740 > > Setting PTRACE_O_SUSPEND_SECCOMP is supposed to be a highly > privileged > operation because it allows the tracee to completely bypass all > seccomp > filters on kernels with CONFIG_CHECKPOINT_RESTORE=y. It is only > supposed to > be settable by a process with global CAP_SYS_ADMIN, and only if that > process is not subject to any seccomp filters at all. > > However, while these permission checks were done on the > PTRACE_SETOPTIONS > path, they were missing on the PTRACE_SEIZE path, which also sets > user-specified ptrace flags. > > Move the permissions checks out into a helper function and let both > ptrace_attach() and ptrace_setoptions() call it. > > Cc: stable@kernel.org > Fixes: 13c4a90119d2 ("seccomp: add ptrace options for > suspend/resume") > Signed-off-by: Jann Horn <jannh@google.com> > Link: > https://lkml.kernel.org/r/20220319010838.1386861-1-jannh@google.com > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > (cherry picked from commit ee1fee900537b5d9560e9f937402de5ddc8412f3) > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Acked-by: Cengiz Can <cengiz.can@canonical.com> > --- > kernel/ptrace.c | 47 ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 32 insertions(+), 15 deletions(-) > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index 2997ca600d18..0a0fd0e74a6b 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -371,6 +371,26 @@ bool ptrace_may_access(struct task_struct *task, > unsigned int mode) > return !err; > } > > +static int check_ptrace_options(unsigned long data) > +{ > + if (data & ~(unsigned long)PTRACE_O_MASK) > + return -EINVAL; > + > + if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) { > + if (!IS_ENABLED(CONFIG_CHECKPOINT_RESTORE) || > + !IS_ENABLED(CONFIG_SECCOMP)) > + return -EINVAL; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + if (seccomp_mode(¤t->seccomp) != > SECCOMP_MODE_DISABLED || > + current->ptrace & PT_SUSPEND_SECCOMP) > + return -EPERM; > + } > + return 0; > +} > + > static int ptrace_attach(struct task_struct *task, long request, > unsigned long addr, > unsigned long flags) > @@ -382,8 +402,16 @@ static int ptrace_attach(struct task_struct > *task, long request, > if (seize) { > if (addr != 0) > goto out; > + /* > + * This duplicates the check in > check_ptrace_options() because > + * ptrace_attach() and ptrace_setoptions() have > historically > + * used different error codes for unknown ptrace > options. > + */ > if (flags & ~(unsigned long)PTRACE_O_MASK) > goto out; > + retval = check_ptrace_options(flags); > + if (retval) > + return retval; > flags = PT_PTRACED | PT_SEIZED | (flags << > PT_OPT_FLAG_SHIFT); > } else { > flags = PT_PTRACED; > @@ -656,22 +684,11 @@ int ptrace_writedata(struct task_struct *tsk, > char __user *src, unsigned long ds > static int ptrace_setoptions(struct task_struct *child, unsigned > long data) > { > unsigned flags; > + int ret; > > - if (data & ~(unsigned long)PTRACE_O_MASK) > - return -EINVAL; > - > - if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) { > - if (!IS_ENABLED(CONFIG_CHECKPOINT_RESTORE) || > - !IS_ENABLED(CONFIG_SECCOMP)) > - return -EINVAL; > - > - if (!capable(CAP_SYS_ADMIN)) > - return -EPERM; > - > - if (seccomp_mode(¤t->seccomp) != > SECCOMP_MODE_DISABLED || > - current->ptrace & PT_SUSPEND_SECCOMP) > - return -EPERM; > - } > + ret = check_ptrace_options(data); > + if (ret) > + return ret; > > /* Avoid intermediate state when all opts are cleared */ > flags = child->ptrace; > -- > 2.32.0 > >
diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 2997ca600d18..0a0fd0e74a6b 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -371,6 +371,26 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode) return !err; } +static int check_ptrace_options(unsigned long data) +{ + if (data & ~(unsigned long)PTRACE_O_MASK) + return -EINVAL; + + if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) { + if (!IS_ENABLED(CONFIG_CHECKPOINT_RESTORE) || + !IS_ENABLED(CONFIG_SECCOMP)) + return -EINVAL; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (seccomp_mode(¤t->seccomp) != SECCOMP_MODE_DISABLED || + current->ptrace & PT_SUSPEND_SECCOMP) + return -EPERM; + } + return 0; +} + static int ptrace_attach(struct task_struct *task, long request, unsigned long addr, unsigned long flags) @@ -382,8 +402,16 @@ static int ptrace_attach(struct task_struct *task, long request, if (seize) { if (addr != 0) goto out; + /* + * This duplicates the check in check_ptrace_options() because + * ptrace_attach() and ptrace_setoptions() have historically + * used different error codes for unknown ptrace options. + */ if (flags & ~(unsigned long)PTRACE_O_MASK) goto out; + retval = check_ptrace_options(flags); + if (retval) + return retval; flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT); } else { flags = PT_PTRACED; @@ -656,22 +684,11 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds static int ptrace_setoptions(struct task_struct *child, unsigned long data) { unsigned flags; + int ret; - if (data & ~(unsigned long)PTRACE_O_MASK) - return -EINVAL; - - if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) { - if (!IS_ENABLED(CONFIG_CHECKPOINT_RESTORE) || - !IS_ENABLED(CONFIG_SECCOMP)) - return -EINVAL; - - if (!capable(CAP_SYS_ADMIN)) - return -EPERM; - - if (seccomp_mode(¤t->seccomp) != SECCOMP_MODE_DISABLED || - current->ptrace & PT_SUSPEND_SECCOMP) - return -EPERM; - } + ret = check_ptrace_options(data); + if (ret) + return ret; /* Avoid intermediate state when all opts are cleared */ flags = child->ptrace;