Re: [PATCH RESEND] elf: add AT_ARGV and AT_ENVV auxiliary vector entries

[Andrew Burgess <aburgess@redhat.com>]

Alexandra Hájková <ahajkova@redhat.com> writes:

> Add AT_ARGV (52), which contains the address of the argv pointer
> array on the initial process stack, and AT_ENVV (53), which contains
> the address of the envp pointer array.
>
> The motivation is to allow GDB to find argv and envp in core dumps
> cleanly. Currently GDB locates them by scanning backwards through
> stack memory from AT_EXECFN, which is fragile. With AT_ARGV and
> AT_ENVV it can read the addresses directly from the core dump's
> auxv note.

Background: I'm a GDB maintainer, not a kernel maintainer.  Alexandra
created this patch after I mentioned that Linux lacks this feature that
FreeBSD has, and as a result GDB has to search for the ARGV and ENVV on
the stack[1][2].

Despite not being a kernel maintainer, I took a look through this patch
and had some thoughts, see inline below.

[1] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/linux-tdep.c;h=23e43ba5c5f952d695a41efccab87cf7dc27b1fa;hb=HEAD#l1981
[2] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/fbsd-tdep.c;h=419f935ea72fb5e6a5db65917af2d147aba65676;hb=HEAD#l2373

>
> Signed-off-by: Alexandra Hájková <ahajkova@redhat.com>
> ---
> Resending after no response for 4 weeks.
>
>  fs/binfmt_elf.c             | 9 ++++++++-
>  include/linux/auxvec.h      | 2 +-
>  include/uapi/linux/auxvec.h | 2 ++
>  3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 8e89cc5b2820..c2b3032adccb 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -182,6 +182,8 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
>  	int ei_index;
>  	const struct cred *cred = current_cred();
>  	struct vm_area_struct *vma;
> +	elf_addr_t *at_argv_val;
> +	elf_addr_t *at_envv_val;
>  
>  	/*
>  	 * In some cases (e.g. Hyper-Threading), we want to avoid L1
> @@ -288,6 +290,10 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
>  	NEW_AUX_ENT(AT_RSEQ_FEATURE_SIZE, offsetof(struct rseq, end));
>  	NEW_AUX_ENT(AT_RSEQ_ALIGN, __alignof__(struct rseq));
>  #endif
> +	at_argv_val = elf_info + 1;
> +	NEW_AUX_ENT(AT_ARGV, 0);
> +	at_envv_val = elf_info + 1;
> +	NEW_AUX_ENT(AT_ENVV, 0);
>  #undef NEW_AUX_ENT
>  	/* AT_NULL is zero; clear the rest too */
>  	memset(elf_info, 0, (char *)mm->saved_auxv +
> @@ -309,7 +315,8 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
>  #else
>  	sp = (elf_addr_t __user *)bprm->p;
>  #endif
> -
> +	*at_argv_val = (unsigned long)(sp + sizeof(elf_addr_t));
> +	*at_envv_val = (unsigned long)(sp + (argc + 2));

Is this correct?  SP is of type `elf_addr_t __user *sp;`, as such
additions to it are in units of `elf_addr_t`, right?

So for the `*at_envv_val` line you are increasing SP by:

  (argc + 2) * sizeof(elf_addr_t)

But for the `*at_argv_val` line you are increasing SP by:

  sizeof(elf_addr_t) * sizeof(elf_addr_t)

Which I don't think is what you want.  I think the line should be:

  *at_argv_val = (unsigned long)(sp + 1);

Also, I wonder about the cast to (unsigned long) here. In the
NEW_AUX_ENT macro calls above, when casting is needed, the pattern is to
cast to '(elf_addr_t)(unsigned long)' which would make more sense given:

        elf_addr_t *at_argv_val;
        elf_addr_t *at_envv_val;

Also, I noticed the file fs/binfmt_elf_fdpic.c, which contains the
function create_elf_fdpic_tables which is similar to create_elf_tables
that you are patching.  Some research indicating that the fdpic file is
used for some targets without an MMU, but they might also benefit from
the same feature, so maybe that file should be patched too?  It doesn't
look like the exact same fix will work there as things are done in a
slightly different order, but I'm sure it should be possible.

If that file isn't patched then the commit message should at least
mention it, and justify why that's being left undone.

Thanks,
Andrew


>  
>  	/*
>  	 * Grow the stack manually; some architectures have a limit on how
> diff --git a/include/linux/auxvec.h b/include/linux/auxvec.h
> index 8bcb9b726262..7184de95b89d 100644
> --- a/include/linux/auxvec.h
> +++ b/include/linux/auxvec.h
> @@ -4,6 +4,6 @@
>  
>  #include <uapi/linux/auxvec.h>
>  
> -#define AT_VECTOR_SIZE_BASE 24 /* NEW_AUX_ENT entries in auxiliary table */
> +#define AT_VECTOR_SIZE_BASE 26 /* NEW_AUX_ENT entries in auxiliary table */
>    /* number of "#define AT_.*" above, minus {AT_NULL, AT_IGNORE, AT_NOTELF} */
>  #endif /* _LINUX_AUXVEC_H */
> diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h
> index cc61cb9b3e9a..e7af32709aba 100644
> --- a/include/uapi/linux/auxvec.h
> +++ b/include/uapi/linux/auxvec.h
> @@ -40,5 +40,7 @@
>  #ifndef AT_MINSIGSTKSZ
>  #define AT_MINSIGSTKSZ	51	/* minimal stack size for signal delivery */
>  #endif
> +#define AT_ARGV		52	/* address of argv[] */
> +#define AT_ENVV		53	/* address of envp[] */
>  
>  #endif /* _UAPI_LINUX_AUXVEC_H */
> -- 
> 2.52.0