lkml.org 
[lkml]   [2018]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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.

\
 
 \ /
  Last update: 2018-09-12 14:28    [W:0.089 / U:28.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site