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

[Kees Cook <kees@kernel.org>]

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