Re: [PATCH v2 1/5] elf2dmp: replace PE export name check with PDB name check

[Peter Maydell]

On Fri, 15 Sept 2023 at 18:02, Viktor Prutyanov <viktor@daynix.com> wrote:
>
> PE export name check introduced in d399d6b179 isn't reliable enough,
> because a page with the export directory may be not present for some
> reason. On the other hand, elf2dmp retrieves the PDB name in any case.
> It can be also used to check that a PE image is the kernel image. So,
> check PDB name when searching for Windows kernel image.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2165917
>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> ---
>  contrib/elf2dmp/main.c | 93 +++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 60 deletions(-)

Hi; Coverity points out a bug in this code (CID 1521598):


> -static int pe_get_pdb_symstore_hash(uint64_t base, void *start_addr,
> -        char *hash, struct va_space *vs)
> +static bool pe_check_pdb_name(uint64_t base, void *start_addr,
> +        struct va_space *vs, OMFSignatureRSDS *rsds)
>  {
>      const char sign_rsds[4] = "RSDS";

sign_rsds[] is not NUL-terminated...

>      IMAGE_DEBUG_DIRECTORY debug_dir;
> -    OMFSignatureRSDS rsds;
> -    char *pdb_name;
> -    size_t pdb_name_sz;
> -    size_t i;
> +    char pdb_name[sizeof(PDB_NAME)];
>
>      if (pe_get_data_dir_entry(base, start_addr, IMAGE_FILE_DEBUG_DIRECTORY,
>                  &debug_dir, sizeof(debug_dir), vs)) {
>          eprintf("Failed to get Debug Directory\n");
> -        return 1;
> +        return false;
>      }
>
>      if (debug_dir.Type != IMAGE_DEBUG_TYPE_CODEVIEW) {
> -        return 1;
> +        eprintf("Debug Directory type is not CodeView\n");
> +        return false;
>      }
>
>      if (va_space_rw(vs,
>                  base + debug_dir.AddressOfRawData,
> -                &rsds, sizeof(rsds), 0)) {
> -        return 1;
> +                rsds, sizeof(*rsds), 0)) {
> +        eprintf("Failed to resolve OMFSignatureRSDS\n");
> +        return false;
>      }
>
> -    printf("CodeView signature is \'%.4s\'\n", rsds.Signature);
> -
> -    if (memcmp(&rsds.Signature, sign_rsds, sizeof(sign_rsds))) {
> -        return 1;
> +    if (memcmp(&rsds->Signature, sign_rsds, sizeof(sign_rsds))) {
> +        eprintf("CodeView signature is \'%.4s\', \'%s\' expected\n",
> +                rsds->Signature, sign_rsds);

...but in this printf() we pass sign_rsds to a "%s" format
specifier, which requires NUL termination.

If you want to print a non-NUL-terminated string you need
to do the same thing we do for rsds->Signature, which is
give it a precision which is the correct length so printf
doesn't read off the end (i.e. "%.4s").

thanks
-- PMM