[PATCH for-5.0] dump: Fix writing of ELF section

[PATCH for-5.0] dump: Fix writing of ELF section

[Peter Maydell]

In write_elf_section() we set the 'shdr' pointer to point to local
structures shdr32 or shdr64, which we fill in to be written out to
the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
has a spurious '&' operator, so instead of writing out the section
header we write out the literal pointer value followed by whatever is
on the stack after the 'shdr' local variable.

Pass the correct address into fd_write_vmcore().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I have not tested this because I can't reproduce the conditions
under which we try to actually use write_elf_section() (they
must be rare, because currently we produce a bogus ELF file
for this code path). In dump_init() s->list.num must be
at least UINT16_MAX-1, which I think means it has to be a
paging-enabled dump and the guest's page table must be
extremely fragmented ?
---
 dump/dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dump/dump.c b/dump/dump.c
index 6fb6e1245ad..22ed1d3b0d4 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -364,7 +364,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
         shdr = &shdr64;
     }
 
-    ret = fd_write_vmcore(&shdr, shdr_size, s);
+    ret = fd_write_vmcore(shdr, shdr_size, s);
     if (ret < 0) {
         error_setg_errno(errp, -ret,
                          "dump: failed to write section header table");
-- 
2.20.1

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Peter Maydell]

On Tue, 24 Mar 2020 at 17:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In write_elf_section() we set the 'shdr' pointer to point to local
> structures shdr32 or shdr64, which we fill in to be written out to
> the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> has a spurious '&' operator, so instead of writing out the section
> header we write out the literal pointer value followed by whatever is
> on the stack after the 'shdr' local variable.
>
> Pass the correct address into fd_write_vmcore().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have not tested this because I can't reproduce the conditions
> under which we try to actually use write_elf_section() (they
> must be rare, because currently we produce a bogus ELF file
> for this code path). In dump_init() s->list.num must be
> at least UINT16_MAX-1, which I think means it has to be a
> paging-enabled dump and the guest's page table must be
> extremely fragmented ?
> ---
>  dump/dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 6fb6e1245ad..22ed1d3b0d4 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -364,7 +364,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
>          shdr = &shdr64;
>      }
>
> -    ret = fd_write_vmcore(&shdr, shdr_size, s);
> +    ret = fd_write_vmcore(shdr, shdr_size, s);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
>                           "dump: failed to write section header table");

Just realized this probably merits
Cc: qemu-stable@nongnu.org

thanks
-- PMM

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Marc-André Lureau]

Hi

On Tue, Mar 24, 2020 at 6:36 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In write_elf_section() we set the 'shdr' pointer to point to local
> structures shdr32 or shdr64, which we fill in to be written out to
> the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> has a spurious '&' operator, so instead of writing out the section
> header we write out the literal pointer value followed by whatever is
> on the stack after the 'shdr' local variable.
>
> Pass the correct address into fd_write_vmcore().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
> I have not tested this because I can't reproduce the conditions
> under which we try to actually use write_elf_section() (they
> must be rare, because currently we produce a bogus ELF file
> for this code path). In dump_init() s->list.num must be
> at least UINT16_MAX-1, which I think means it has to be a
> paging-enabled dump and the guest's page table must be
> extremely fragmented ?

yeah, I can't help either without spending more time playing with it,
but the fix looks good nonetheless.

> ---
>  dump/dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 6fb6e1245ad..22ed1d3b0d4 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -364,7 +364,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
>          shdr = &shdr64;
>      }
>
> -    ret = fd_write_vmcore(&shdr, shdr_size, s);
> +    ret = fd_write_vmcore(shdr, shdr_size, s);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
>                           "dump: failed to write section header table");
> --
> 2.20.1
>

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Peter Maydell]

On Tue, 24 Mar 2020 at 17:49, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Tue, Mar 24, 2020 at 6:36 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > In write_elf_section() we set the 'shdr' pointer to point to local
> > structures shdr32 or shdr64, which we fill in to be written out to
> > the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> > has a spurious '&' operator, so instead of writing out the section
> > header we write out the literal pointer value followed by whatever is
> > on the stack after the 'shdr' local variable.
> >
> > Pass the correct address into fd_write_vmcore().
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks for the review; since nobody else has picked the patch
up I'll put it in via target-arm.next just for convenience.

-- PMM

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Philippe Mathieu-Daudé]

On 3/24/20 6:36 PM, Peter Maydell wrote:
> In write_elf_section() we set the 'shdr' pointer to point to local
> structures shdr32 or shdr64, which we fill in to be written out to
> the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> has a spurious '&' operator, so instead of writing out the section
> header we write out the literal pointer value followed by whatever is
> on the stack after the 'shdr' local variable.

How did you notice this? While reviewing around?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Pass the correct address into fd_write_vmcore().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have not tested this because I can't reproduce the conditions
> under which we try to actually use write_elf_section() (they
> must be rare, because currently we produce a bogus ELF file
> for this code path). In dump_init() s->list.num must be
> at least UINT16_MAX-1, which I think means it has to be a
> paging-enabled dump and the guest's page table must be
> extremely fragmented ?
> ---
>   dump/dump.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 6fb6e1245ad..22ed1d3b0d4 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -364,7 +364,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
>           shdr = &shdr64;
>       }
>   
> -    ret = fd_write_vmcore(&shdr, shdr_size, s);
> +    ret = fd_write_vmcore(shdr, shdr_size, s);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret,
>                            "dump: failed to write section header table");
> 

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Peter Maydell]

On Fri, 3 Apr 2020 at 21:55, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 3/24/20 6:36 PM, Peter Maydell wrote:
> > In write_elf_section() we set the 'shdr' pointer to point to local
> > structures shdr32 or shdr64, which we fill in to be written out to
> > the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> > has a spurious '&' operator, so instead of writing out the section
> > header we write out the literal pointer value followed by whatever is
> > on the stack after the 'shdr' local variable.
>
> How did you notice this? While reviewing around?

Coverity, but I forgot to quote the CID in the commit message.

-- PMM

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Peter Maydell]

I forgot to cc qemu-stable, so doing that now (I've also added the
Cc: tag to the commit message for when I send this in the target-arm
pullreq today); for the record, the Coverity id is CID 1421970
(also added to the commit message).

thanks
-- PMM

On Tue, 24 Mar 2020 at 17:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In write_elf_section() we set the 'shdr' pointer to point to local
> structures shdr32 or shdr64, which we fill in to be written out to
> the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> has a spurious '&' operator, so instead of writing out the section
> header we write out the literal pointer value followed by whatever is
> on the stack after the 'shdr' local variable.
>
> Pass the correct address into fd_write_vmcore().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have not tested this because I can't reproduce the conditions
> under which we try to actually use write_elf_section() (they
> must be rare, because currently we produce a bogus ELF file
> for this code path). In dump_init() s->list.num must be
> at least UINT16_MAX-1, which I think means it has to be a
> paging-enabled dump and the guest's page table must be
> extremely fragmented ?
> ---
>  dump/dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 6fb6e1245ad..22ed1d3b0d4 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -364,7 +364,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
>          shdr = &shdr64;
>      }
>
> -    ret = fd_write_vmcore(&shdr, shdr_size, s);
> +    ret = fd_write_vmcore(shdr, shdr_size, s);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
>                           "dump: failed to write section header table");
> --
> 2.20.1
>