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

Viktor Prutyanov posted 5 patches 1 year, 1 month ago
Maintainers: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
[PATCH v2 5/5] elf2dmp: rework PDB_STREAM_INDEXES::segments obtaining
Posted by Viktor Prutyanov 1 year, 1 month ago
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>
---
 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) {
@@ -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));
 
     /* Read global symbol table */
     r->modimage = pdb_ds_read_file(r, symbols->gsym_file);
diff --git a/contrib/elf2dmp/pdb.h b/contrib/elf2dmp/pdb.h
index 4ea8925ee8..2a50da56ac 100644
--- a/contrib/elf2dmp/pdb.h
+++ b/contrib/elf2dmp/pdb.h
@@ -227,7 +227,7 @@ struct pdb_reader {
     } ds;
     uint32_t file_used[1024];
     PDB_SYMBOLS *symbols;
-    PDB_STREAM_INDEXES sidx;
+    uint16_t segments;
     uint8_t *modimage;
     char *segs;
     size_t segs_size;
-- 
2.21.0
Re: [PATCH v2 5/5] elf2dmp: rework PDB_STREAM_INDEXES::segments obtaining
Posted by Peter Maydell 1 year, 1 month ago
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