Re: CVS commit: src/sys/kern

[Kamil Rytarowski]

> Sent: Wednesday, March 29, 2017 at 11:27 PM
> From: "Kamil Rytarowski" 
> To: "Christos Zoulas" 
> Cc: source-changes-d@NetBSD.org
> Subject: Re: CVS commit: src/sys/kern
>
> 
> 
> > Sent: Wednesday, March 29, 2017 at 10:09 PM
> > From: "Christos Zoulas" 
> > To: source-changes-d@NetBSD.org
> > Subject: Re: CVS commit: src/sys/kern
> >
> > In article <20170329195230.d895af...@cvs.netbsd.org>,
> > Kamil Rytarowski  wrote:
> > >-=-=-=-=-=-
> > >
> > >Module Name:   src
> > >Committed By:  kamil
> > >Date:  Wed Mar 29 19:52:30 UTC 2017
> > >
> > >Modified Files:
> > >   src/sys/kern: core_elf32.c sys_ptrace_common.c
> > >
> > >Log Message:
> > >Generate ELF AUXV for core(5) and ptrace(2) limited to the vector TYPE x V
> > >
> > >Previously PT_DUMPCORE and PIOD_READ_AUXV and regular core dumping 
> > >retrieved
> > >the vector of AuxInfo {a_type, a_v} + MAXPATHLEN + ALIGN(1).
> > >
> > >The extra data is not actually needed in the returned chunk. It can be
> > >retrieved with PT_READ_I operations and it's the preferred way to access
> > >them as the AuxInfo fields contain pointers (void* format) to them.
> > >
> > >This changes the behavior of the kernel, no stable releases are affected
> > >with this move. Current software is not affected as other systems already
> > >stop generating data on AT_NULL. This streamlines the NetBSD behavior with
> > >other ELF format OSes. This move also simplifies determination if we got
> > >all the needed data inside the debugger and we no longer need to eliminate
> > >the unneeded chunk at the end.
> > 
> > This is both incomplete and broken:
> > 
> > 1. The size needed is not just the maximum number of entries * the size of
> >an entry. You have to account for the string entry storage that
> >AT_SUN_EXECNAME needs (and alignment).
> 
> My intention was to remove the content of it. Do we need it? Why?
> AT_SUN_EXECNAME has a pointer and we can retrieve it with a debugger.
> I'm not sure it's safe to read and data after trailing AT_NULL and make
> assumptions what is it.
> 
> I checked SunOS (perhaps the best reference for AT_SUN_EXECNAME) and
> they don't offer this either.
> 
> AUXV is stored in /proc/$pid/auxv
> 
> $ uname -rms
> SunOS 5.11 i86pc
> 
> $ hexdump  /home/kamil/auxv.sol.txt   
> 
> 000 07d8  7fcf 0804 07de  7fd5 0804
> 010 0019  7fec 0804 0003  0034 0805
> 020 0004  0020  0005  0006 
> 030 0009  6480 0805 07e0  b000 feff
> 040 0007  5000 fefb 0008   
> 050 0006  1000  07e1  0002 
> 060 07d9  5c6f 7dd7 07e7   
> 070        
> *
> 0b0
> 
> 07d8  - AT_SUN_PLATFORM
> 07de  - AT_SUN_EXECNAME
> 0019  - AT_RANDOM
> 0003  - AT_PHDR
> 0004  - AT_PHENT
> 0005  - AT_PHNUM
> 0009  - AT_ENTRY
> 07e0  - AT_SUN_LDDATA
> 0007  - AT_BASE
> 0008  - AT_FLAGS
> 0006  - AT_PAGESZ
> 07e1  - AT_SUN_AUXFLAGS
> 07d9  - AT_SUN_HWCAP
> 07e7  - AT_SUN_HWCAP2
>   - AT_NULL
> 
> 
> > 2. There are other places where es_arglen is used, and needs to be cleaned
> >up not to mention all the size info defines in the emulations
> >(svr4/osf1/linux).
> 
> I will check emulation core dumps.
> 
> > 3. You can't assume that ELF_AUX_ENTRIES is the max number of entries needed
> >since emulations might need more (they currently don't) and some need
> >less.
> > 
> > In other words, you strictly made the situation worse and you possibly broke
> > coredumps. Fortunately you did not break exec code :-)
> > 
> > christos
> > 
> > 
> 

I've checked the emulation code and indeed, there is a variety of possibilities.
There are longer and shorter AUX vectors.

Two simple solutions:
 - revert and keep some trailing content after AT_NULL,
 - check PK_32, stack grow direction and find AT_NULL before dumping the vector.

The former seems safer and I will go for it.

Thanks for pointing it out.

Re: CVS commit: src/sys/kern

[Kamil Rytarowski]

> Sent: Wednesday, March 29, 2017 at 10:09 PM
> From: "Christos Zoulas" 
> To: source-changes-d@NetBSD.org
> Subject: Re: CVS commit: src/sys/kern
>
> In article <20170329195230.d895af...@cvs.netbsd.org>,
> Kamil Rytarowski  wrote:
> >-=-=-=-=-=-
> >
> >Module Name: src
> >Committed By:kamil
> >Date:Wed Mar 29 19:52:30 UTC 2017
> >
> >Modified Files:
> > src/sys/kern: core_elf32.c sys_ptrace_common.c
> >
> >Log Message:
> >Generate ELF AUXV for core(5) and ptrace(2) limited to the vector TYPE x V
> >
> >Previously PT_DUMPCORE and PIOD_READ_AUXV and regular core dumping retrieved
> >the vector of AuxInfo {a_type, a_v} + MAXPATHLEN + ALIGN(1).
> >
> >The extra data is not actually needed in the returned chunk. It can be
> >retrieved with PT_READ_I operations and it's the preferred way to access
> >them as the AuxInfo fields contain pointers (void* format) to them.
> >
> >This changes the behavior of the kernel, no stable releases are affected
> >with this move. Current software is not affected as other systems already
> >stop generating data on AT_NULL. This streamlines the NetBSD behavior with
> >other ELF format OSes. This move also simplifies determination if we got
> >all the needed data inside the debugger and we no longer need to eliminate
> >the unneeded chunk at the end.
> 
> This is both incomplete and broken:
> 
> 1. The size needed is not just the maximum number of entries * the size of
>an entry. You have to account for the string entry storage that
>AT_SUN_EXECNAME needs (and alignment).

My intention was to remove the content of it. Do we need it? Why?
AT_SUN_EXECNAME has a pointer and we can retrieve it with a debugger.
I'm not sure it's safe to read and data after trailing AT_NULL and make
assumptions what is it.

I checked SunOS (perhaps the best reference for AT_SUN_EXECNAME) and
they don't offer this either.

AUXV is stored in /proc/$pid/auxv

$ uname -rms
SunOS 5.11 i86pc

$ hexdump  /home/kamil/auxv.sol.txt 
  
000 07d8  7fcf 0804 07de  7fd5 0804
010 0019  7fec 0804 0003  0034 0805
020 0004  0020  0005  0006 
030 0009  6480 0805 07e0  b000 feff
040 0007  5000 fefb 0008   
050 0006  1000  07e1  0002 
060 07d9  5c6f 7dd7 07e7   
070        
*
0b0

07d8  - AT_SUN_PLATFORM
07de  - AT_SUN_EXECNAME
0019  - AT_RANDOM
0003  - AT_PHDR
0004  - AT_PHENT
0005  - AT_PHNUM
0009  - AT_ENTRY
07e0  - AT_SUN_LDDATA
0007  - AT_BASE
0008  - AT_FLAGS
0006  - AT_PAGESZ
07e1  - AT_SUN_AUXFLAGS
07d9  - AT_SUN_HWCAP
07e7  - AT_SUN_HWCAP2
  - AT_NULL


> 2. There are other places where es_arglen is used, and needs to be cleaned
>up not to mention all the size info defines in the emulations
>(svr4/osf1/linux).

I will check emulation core dumps.

> 3. You can't assume that ELF_AUX_ENTRIES is the max number of entries needed
>since emulations might need more (they currently don't) and some need
>less.
> 
> In other words, you strictly made the situation worse and you possibly broke
> coredumps. Fortunately you did not break exec code :-)
> 
> christos
> 
> 

Re: CVS commit: src/sys/kern

[Christos Zoulas]

In article <20170329195230.d895af...@cvs.netbsd.org>,
Kamil Rytarowski  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  kamil
>Date:  Wed Mar 29 19:52:30 UTC 2017
>
>Modified Files:
>   src/sys/kern: core_elf32.c sys_ptrace_common.c
>
>Log Message:
>Generate ELF AUXV for core(5) and ptrace(2) limited to the vector TYPE x V
>
>Previously PT_DUMPCORE and PIOD_READ_AUXV and regular core dumping retrieved
>the vector of AuxInfo {a_type, a_v} + MAXPATHLEN + ALIGN(1).
>
>The extra data is not actually needed in the returned chunk. It can be
>retrieved with PT_READ_I operations and it's the preferred way to access
>them as the AuxInfo fields contain pointers (void* format) to them.
>
>This changes the behavior of the kernel, no stable releases are affected
>with this move. Current software is not affected as other systems already
>stop generating data on AT_NULL. This streamlines the NetBSD behavior with
>other ELF format OSes. This move also simplifies determination if we got
>all the needed data inside the debugger and we no longer need to eliminate
>the unneeded chunk at the end.

This is both incomplete and broken:

1. The size needed is not just the maximum number of entries * the size of
   an entry. You have to account for the string entry storage that
   AT_SUN_EXECNAME needs (and alignment).
2. There are other places where es_arglen is used, and needs to be cleaned
   up not to mention all the size info defines in the emulations
   (svr4/osf1/linux).
3. You can't assume that ELF_AUX_ENTRIES is the max number of entries needed
   since emulations might need more (they currently don't) and some need
   less.

In other words, you strictly made the situation worse and you possibly broke
coredumps. Fortunately you did not break exec code :-)

christos