Message-ID: <20180912122702.GA16972@redhat.com>
Date:   Wed, 12 Sep 2018 14:27:02 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Rik van Riel <riel@...hat.com>, Michal Hocko <mhocko@...e.com>,
        Stanislav Kozina <skozina@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: get_arg_page() && ptr_size accounting

On 09/11, Kees Cook wrote:
>
> Oh, I like this patch! This is much cleaner.

it's pity. cause this means I will have to actually test this change and
(worse) write the changelog ;)

> > @@ -410,11 +365,6 @@ static int bprm_mm_init(struct linux_binprm *bprm)
> >         if (!mm)
> >                 goto err;
> >
> > -       /* Save current stack limit for all calculations made during exec. */
> > -       task_lock(current->group_leader);
> > -       bprm->rlim_stack = current->signal->rlim[RLIMIT_STACK];
> > -       task_unlock(current->group_leader);
> > -
>
> I would prefer this hunk stay here since it will be more robust
> against weird arch-specific things happening against rlim_stack. I had
> to clean up some of these tests in arch code, so I'm nervous about
> moving this further away. Here is before we call arch_bprm_mm_init(),
> and I think it's better to do this as early as possible.

Well, I don't reaally agree but I won't argue, this is cosmetic at least
right now.

> > +static int prepare_rlim_stack(struct linux_binprm *bprm)
>
> How about collapsing this with:
>
>         bprm->argc = count(argv, MAX_ARG_STRINGS);
>         if ((retval = bprm->argc) < 0)
>                 goto out;
>
>         bprm->envc = count(envp, MAX_ARG_STRINGS);
>         if ((retval = bprm->envc) < 0)
>                 goto out;
>
> and call it prepare_arg_count(struct linux_binprm *bprm,
>                                                 struct user_arg_ptr argv,
>                                                 struct user_arg_ptr envp)

OK, agreed,

> Let's try to retain the comments here:

Yes, sure, I wasn't going to remove the comments,

> > +       /* COMMENT */
>
> This comment should likely be something like:
>
> /*
>  * We must account for the size of all the argv and envp pointers to
>  * the argv and envp strings, since they will also take up space in
>  * the stack. They aren't stored until much later when we can't
>  * signal to the parent that the child has run out of stack space.
>  * Instead, calculate it here so it's possible to fail gracefully.
>  */

Thanks!

> > +       ptr_size = (bprm->argc + bprm->envc) * sizeof(void *);
>
> BTW, in re-reading create_elf_tables() and its calculation of "items",
> I realize the above should actually include the trailing NULL pointers
> and argc, so it should be:
>
> ptr_size = (1 + bprm->argc + 1 + bprm->envc + 1) * sizeof(void *);

Yes, I noticed this too. But can we do this later please?

Firstly, this change needs a special note in the changelog, and thus I
think a separate patch makes more sense.

And in fact I am not sure we really care about "small" O(1) errors in
ptr_size calculations.

> > -       unsigned long p; /* current top of mem */
> > +       unsigned long p, p_min; /* current top of mem */
>
> Can you split this out to a separate line (with a new comment) to
> avoid comment-confusion? Something like:
>
> unsigned long p; /* current top of mem */
> unsigned long p_min; /* the minimum allowed mem position */

OK, but "minimum allowed mem position" explains nothing... The comment
should explain that ->p_min (can you suggest a better name?) is artificial
marker pre-computed for rlim-like checks in copy_strings()...

> I've also spent some more time convincing myself again that there
> aren't races between count(), copy_strings(), and create_elf_tables().

Yes, I thought about this too, do not see anything dangerous.

BTW. I think we can simply kill count(). But this needs another cleanup
and dicsussion.

Thanks for review!

Oleg.

Powered by blists - more mailing lists