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
