Re: [PATCH v7 7/9] coredump: validate socket name as it is written

[Christian Brauner <brauner@kernel.org>]

On Thu, May 15, 2025 at 10:56:51PM +0200, Jann Horn wrote:
> On Thu, May 15, 2025 at 12:04 AM Christian Brauner <brauner@kernel.org> wrote:
> > In contrast to other parameters written into
> > /proc/sys/kernel/core_pattern that never fail we can validate enabling
> > the new AF_UNIX support. This is obviously racy as hell but it's always
> > been that way.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> Reviewed-by: Jann Horn <jannh@google.com>
> 
> > ---
> >  fs/coredump.c | 37 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 6ee38e3da108..d4ff08ef03e5 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -1228,13 +1228,44 @@ void validate_coredump_safety(void)
> >         }
> >  }
> >
> > +static inline bool check_coredump_socket(void)
> > +{
> > +       if (core_pattern[0] != '@')
> > +               return true;
> > +
> > +       /*
> > +        * Coredump socket must be located in the initial mount
> > +        * namespace. Don't give the that impression anything else is
> > +        * supported right now.
> > +        */
> > +       if (current->nsproxy->mnt_ns != init_task.nsproxy->mnt_ns)
> > +               return false;
> 
> (Ah, dereferencing init_task.nsproxy without locks is safe because
> init_task is actually the boot cpu's swapper/idle task, which never
> switches namespaces, right?)

I would be very worried if it did. It would fsck everyone over that
relies on copying its credentials and assumes that the set of namespaces
is stable.

> 
> > +       /* Must be an absolute path. */
> > +       if (*(core_pattern + 1) != '/')
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> >  static int proc_dostring_coredump(const struct ctl_table *table, int write,
> >                   void *buffer, size_t *lenp, loff_t *ppos)
> >  {
> > -       int error = proc_dostring(table, write, buffer, lenp, ppos);
> > +       int error;
> > +       ssize_t retval;
> > +       char old_core_pattern[CORENAME_MAX_SIZE];
> > +
> > +       retval = strscpy(old_core_pattern, core_pattern, CORENAME_MAX_SIZE);
> > +
> > +       error = proc_dostring(table, write, buffer, lenp, ppos);
> > +       if (error)
> > +               return error;
> > +       if (!check_coredump_socket()) {
> 
> (non-actionable note: This is kiiinda dodgy under
> SYSCTL_WRITES_LEGACY, but I guess we can assume that new users of the
> new coredump socket feature aren't actually going to write the
> coredump path one byte at a time, so I guess it's fine.)

So this is all kinds of broken already imho. Because there's not really
mutual exclusion between multiple writers to such sysctls from what I
remember. Which means that this buffer can be trampled in all kinds of
ways if multiple tasks decide to update it at the same time. That's
super unlikely of course but whatever.

> 
> > +               strscpy(core_pattern, old_core_pattern, retval + 1);
> 
> The third strscpy() argument is semantically supposed to be the
> destination buffer size, not the amount of data to copy. For trivial
> invocations like here, strscpy() actually allows you to leave out the
> third argument.

Eeeeewww, that's really implicit behavior. I can use the destination
buffer size but given that retval will always be smaller than that I
didn't bother but ok. I'll fix that in-tree.