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

[Tycho Andersen <tycho@tycho.pizza>]

On Thu, Oct 17, 2024 at 08:47:03AM -0700, Kees Cook wrote:
> 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)

Yes, it's valid, but we need a kernel pointer for __set_task_comm().

> > > 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.

Ok, cool. But with your notes below, the allocation will still be
before begin_new_execexit(), just only in the cases where we actually
need it, hopefully that's okay.

> > +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?

Yes, sounds good.

> > +		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()?

Yep, this is much clearer, thanks. I will respin with these as a Real
Patch.

Tycho