Re: [PATCH v2 5/5] elf2dmp: rework PDB_STREAM_INDEXES::segments obtaining

[Peter Maydell]

On Fri, 15 Sept 2023 at 18:02, Viktor Prutyanov <viktor@daynix.com> wrote:
>
> PDB for Windows 11 kernel has slightly different structure compared to
> previous versions. Since elf2dmp don't use the other fields, copy only
> 'segments' field from PDB_STREAM_INDEXES.
>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>

Hi; this patch has triggered Coverity to report an issue with
the code:

> ---
>  contrib/elf2dmp/pdb.c | 15 ++++-----------
>  contrib/elf2dmp/pdb.h |  2 +-
>  2 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
> index adcfa7e154..6ca5086f02 100644
> --- a/contrib/elf2dmp/pdb.c
> +++ b/contrib/elf2dmp/pdb.c
> @@ -160,7 +160,7 @@ static void *pdb_ds_read_file(struct pdb_reader* r, 
> uint32_t file_number)
>  static int pdb_init_segments(struct pdb_reader *r)
>  {
>      char *segs;
> -    unsigned stream_idx = r->sidx.segments;
> +    unsigned stream_idx = r->segments;
>
>      segs = pdb_ds_read_file(r, stream_idx);
>      if (!segs) {

Here we set stream_idx from r->segments, and later in this
function we're going to call pdb_get_file_size(r, stream_idx),
which uses stream_idx as an index int o the toc->file_size[] array...

> @@ -177,9 +177,6 @@ static int pdb_init_symbols(struct pdb_reader *r)
>  {
>      int err = 0;
>      PDB_SYMBOLS *symbols;
> -    PDB_STREAM_INDEXES *sidx = &r->sidx;
> -
> -    memset(sidx, -1, sizeof(*sidx));
>
>      symbols = pdb_ds_read_file(r, 3);
>      if (!symbols) {
> @@ -188,15 +185,11 @@ static int pdb_init_symbols(struct pdb_reader *r)
>
>      r->symbols = symbols;
>
> -    if (symbols->stream_index_size != sizeof(PDB_STREAM_INDEXES)) {
> -        err = 1;
> -        goto out_symbols;
> -    }
> -
> -    memcpy(sidx, (const char *)symbols + sizeof(PDB_SYMBOLS) +
> +    r->segments = *(uint16_t *)((const char *)symbols + sizeof(PDB_SYMBOLS) +
>              symbols->module_size + symbols->offset_size +
>              symbols->hash_size + symbols->srcmodule_size +
> -            symbols->pdbimport_size + symbols->unknown2_size, sizeof(*sidx));
> +            symbols->pdbimport_size + symbols->unknown2_size +
> +            offsetof(PDB_STREAM_INDEXES, segments));

...but we initialized r->segments based on data from the file we're reading,
and we never do any kind of bounds checking on it. So we'll crash if the
file is corrupt/malicious.

Presumably there should be some sort of bounds check somewhere.

(This is CID 1521597.)

thanks
-- PMM