On Thu, May 15, 2025 at 12:04 AM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxx>
Reviewed-by: Jann Horn <jannh@xxxxxxxxxx>
> ---
> 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?)
> + /* 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.)
> + 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.
> + return -EINVAL;
> + }
>
> - if (!error)
> - validate_coredump_safety();
> + validate_coredump_safety();
> return error;
> }
>
>
> --
> 2.47.2
>
