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",
