On Thu, Oct 17, 2024 at 08:34:43AM -0600, Tycho Andersen wrote:
> 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...
Yes, but it'll be valid __user addr in the new process. (IIUC)
> > 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?
If we can't alloc a string in begin_new_exec() we're going to have much
later problems, so yeah, I'm fine with it failing there.
> 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 :)
But to keep this more readable, I do like your version below, with some
notes.
>
> 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);
To keep this const but not call get_user_arg_ptr() before the fdpath
check, how about externalizing it. See further below...
> +
> + /*
> + * 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);
Do we want an empty argv0, though? Shouldn't an empty fall back to
fdpath?
> + 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;
How about:
if (unlikely(bprm->fdpath)) {
retval = bprm_add_fixup_comm(bprm, argv);
if (retval != 0)
goto out_free;
}
with the fdpath removed from bprm_add_fixup_comm()?
> +
> retval = count(argv, MAX_ARG_STRINGS);
> if (retval == 0)
> pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n",
--
Kees Cook
