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.
