Re: [PATCH] libdwfl: Read no more than required to parse dynamic sections

[Mark Wielaard <mark@klomp.org>]

Hi Gavin,

On Mon, 2022-11-28 at 22:26 -0800, gavin@matician.com wrote:
> Since size checking has been moved to
> dwfl_elf_phdr_memory_callback(),
> there is no longer a need for dwfl_segment_report_module() to enforce
> the same. Reading beyond the end of the dynamic section actually causes
> issues when passing the data to elfXX_xlatetom() because it is possible
> that src->d_size is not a multiple of recsize (for ELF_T_DYN, recsize is
> 16 while the minimum required alignment is 8), causing elfXX_xlatetom()
> to return ELF_E_INVALID_DATA.

I don't fully follow this logic.

The code as written doesn't seem to guarantee that
dwfl_segment_report_module will always be called with
dwfl_elf_phdr_memory_callback as memory_callback. Although it probably
will be in practice.

So you are removing this check:

>       && ! read_portion (&read_state, &dyn_data, &dyn_data_size,
>  			 start, segment, dyn_vaddr, dyn_filesz))
>      {
> -      /* dyn_data_size will be zero if we got everything from the initial
> -         buffer, otherwise it will be the size of the new buffer that
> -         could be read.  */
> -      if (dyn_data_size != 0)
> -	dyn_filesz = dyn_data_size;
> -

Reading read_portion it shows dyn_data_size being set to zero if
read_state->buffer_available has everything (dyn_filesz) requested.
Otherwise memory_callback (we assume dwfl_elf_phdr_memory_callback) is
called with *data_size = dyn_filesz. Which will then be set to the
actual buffer size being read.

So dyn_data_size might be bigger than the dynfilesz we are requesting?
Or smaller I assume.

If you are protecting against it becoming bigger, should the check be
changed to:

	if (dyn_data_size != 0 && dyn_data_size < dyn_filesz)
	  dyn_filesz = dyn_data_size;

?

Thanks,

Mark