Re: [RFC] exec: add a flag for "reasonable" execveat() comm

[Tycho Andersen <tycho@tycho.pizza>]

On Mon, Oct 14, 2024 at 02:13:32PM -0700, Kees Cook wrote:
> On Wed, Oct 09, 2024 at 08:41:31AM -0600, Tycho Andersen wrote:
> > +static int bprm_add_fixup_comm(struct linux_binprm *bprm, struct user_arg_ptr argv)
> > +{
> > +	const char __user *p = get_user_arg_ptr(argv, 0);
> > +
> > +	/*
> > +	 * In keeping with the logic in do_execveat_common(), we say p == NULL
> > +	 * => "" for comm.
> > +	 */
> > +	if (!p) {
> > +		bprm->argv0 = kstrdup("", GFP_KERNEL);
> > +		return 0;
> > +	}
> > +
> > +	bprm->argv0 = strndup_user(p, MAX_ARG_STRLEN);
> > +	if (bprm->argv0)
> > +		return 0;
> > +
> > +	return -EFAULT;
> > +}
> 
> I'd rather this logic got done in copy_strings() and to avoid duplicating
> a copy for all exec users. I think it should be possible to just do
> this, to find the __user char *:
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 77364806b48d..e12fd706f577 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -642,6 +642,8 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
>  				goto out;
>  			}
>  		}
> +		if (argc == 0)
> +			bprm->argv0 = str;
>  	}
>  	ret = 0;
>  out:

Isn't str here a __user? We want a kernel string for setting comm, so
I guess kaddr+offset? But that's not mapped any more...

> Once we get to begin_new_exec(), only if we need to do the work (fdpath
> set), then we can do the strndup_user() instead of making every exec
> hold a copy regardless of whether it will be needed.

What happens if that allocation fails? begin_new_exec() says it is the
point of no return, so we would just swallow the exec? Or have
mysteriously inconsistent behavior?

I think we could check ->fdpath in the bprm_add_fixup_comm() above,
and only do the allocation when really necessary. I should have done
that in the above version, which would have made the comment about
checking fdpath even somewhat true :)

Something like the below?

Tycho



diff --git a/fs/exec.c b/fs/exec.c
index dad402d55681..7ec0bbfbc3c3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1416,7 +1416,16 @@ int begin_new_exec(struct linux_binprm * bprm)
 		set_dumpable(current->mm, SUID_DUMP_USER);
 
 	perf_event_exec();
-	__set_task_comm(me, kbasename(bprm->filename), true);
+
+	/*
+	 * If argv0 was set, execveat() made up a path that will
+	 * probably not be useful to admins running ps or similar.
+	 * Let's fix it up to be something reasonable.
+	 */
+	if (bprm->argv0)
+		__set_task_comm(me, kbasename(bprm->argv0), true);
+	else
+		__set_task_comm(me, kbasename(bprm->filename), true);
 
 	/* An exec changes our domain. We are no longer part of the thread
 	   group */
@@ -1566,9 +1575,36 @@ static void free_bprm(struct linux_binprm *bprm)
 	if (bprm->interp != bprm->filename)
 		kfree(bprm->interp);
 	kfree(bprm->fdpath);
+	kfree(bprm->argv0);
 	kfree(bprm);
 }
 
+static int bprm_add_fixup_comm(struct linux_binprm *bprm, struct user_arg_ptr argv)
+{
+	const char __user *p = get_user_arg_ptr(argv, 0);
+
+	/*
+	 * If this isn't an execveat(), we don't need to fix up the command.
+	 */
+	if (!bprm->fdpath)
+		return 0;
+
+	/*
+	 * In keeping with the logic in do_execveat_common(), we say p == NULL
+	 * => "" for comm.
+	 */
+	if (!p) {
+		bprm->argv0 = kstrdup("", GFP_KERNEL);
+		return 0;
+	}
+
+	bprm->argv0 = strndup_user(p, MAX_ARG_STRLEN);
+	if (bprm->argv0)
+		return 0;
+
+	return -EFAULT;
+}
+
 static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int flags)
 {
 	struct linux_binprm *bprm;
@@ -1975,6 +2011,10 @@ static int do_execveat_common(int fd, struct filename *filename,
 		goto out_ret;
 	}
 
+	retval = bprm_add_fixup_comm(bprm, argv);
+	if (retval != 0)
+		goto out_free;
+
 	retval = count(argv, MAX_ARG_STRINGS);
 	if (retval == 0)
 		pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n",