On Thu, Sep 23, 2021 at 04:31:05PM -0700, Kees Cook wrote: > The /proc/$pid/wchan file has been broken by default on x86_64 for 4 > years now[1]. As this remains a potential leak of either kernel > addresses (when symbolization fails) or limited observation of kernel > function progress, just remove the contents for good. > > Unconditionally set the contents to "0" and also mark the wchan > field in /proc/$pid/stat with 0. I forgot to CC Qi Zheng on this patch. Now corrected. :) > This leaves kernel/sched/fair.c as the only user of get_wchan(). But > again, since this was broken for 4 years, was this profiling logic > actually doing anything useful? If the fair scheduler would actually benefit from still using get_wchan, I think this patch: https://lore.kernel.org/all/20210831083625.59554-1-zhengqi.arch@xxxxxxxxxxxxx/ should still be applied too. If not, we can rip get_wchan() out completely (across all architectures). -Kees > [1] https://lore.kernel.org/lkml/20210922001537.4ktg3r2ky3b3r6yp@treble/ > > Cc: Josh Poimboeuf <jpoimboe@xxxxxxxxxx> > Cc: Vito Caputo <vcaputo@xxxxxxxxxxx> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > --- > arch/x86/kernel/process.c | 2 +- > fs/proc/array.c | 16 +++++----------- > fs/proc/base.c | 16 +--------------- > 3 files changed, 7 insertions(+), 27 deletions(-) > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > index 1d9463e3096b..84a4f9f3f0c2 100644 > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -937,7 +937,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm) > } > > /* > - * Called from fs/proc with a reference on @p to find the function > + * Called from scheduler with a reference on @p to find the function > * which called into schedule(). This needs to be done carefully > * because the task might wake up and we might look at a stack > * changing under us. > diff --git a/fs/proc/array.c b/fs/proc/array.c > index 49be8c8ef555..8a4ecfd901b8 100644 > --- a/fs/proc/array.c > +++ b/fs/proc/array.c > @@ -452,7 +452,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns, > static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, > struct pid *pid, struct task_struct *task, int whole) > { > - unsigned long vsize, eip, esp, wchan = 0; > + unsigned long vsize, eip, esp; > int priority, nice; > int tty_pgrp = -1, tty_nr = 0; > sigset_t sigign, sigcatch; > @@ -540,8 +540,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, > unlock_task_sighand(task, &flags); > } > > - if (permitted && (!whole || num_threads < 2)) > - wchan = get_wchan(task); > if (!whole) { > min_flt = task->min_flt; > maj_flt = task->maj_flt; > @@ -600,16 +598,12 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, > seq_put_decimal_ull(m, " ", sigcatch.sig[0] & 0x7fffffffUL); > > /* > - * We used to output the absolute kernel address, but that's an > - * information leak - so instead we show a 0/1 flag here, to signal > - * to user-space whether there's a wchan field in /proc/PID/wchan. > - * > + * We used to output the absolute kernel address, and then just > + * a symbol. But both are information leaks, so just report 0 > + * to indicate there is no wchan field in /proc/$PID/wchan. > * This works with older implementations of procps as well. > */ > - if (wchan) > - seq_puts(m, " 1"); > - else > - seq_puts(m, " 0"); > + seq_puts(m, " 0"); > > seq_put_decimal_ull(m, " ", 0); > seq_put_decimal_ull(m, " ", 0); > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 533d5836eb9a..52484cd77f99 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -378,24 +378,10 @@ static const struct file_operations proc_pid_cmdline_ops = { > }; > > #ifdef CONFIG_KALLSYMS > -/* > - * Provides a wchan file via kallsyms in a proper one-value-per-file format. > - * Returns the resolved symbol. If that fails, simply return the address. > - */ > static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns, > struct pid *pid, struct task_struct *task) > { > - unsigned long wchan; > - > - if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) > - wchan = get_wchan(task); > - else > - wchan = 0; > - > - if (wchan) > - seq_printf(m, "%ps", (void *) wchan); > - else > - seq_putc(m, '0'); > + seq_putc(m, '0'); > > return 0; > } > -- > 2.30.2 > -- Kees Cook
