[PATCH 0/2] linux-user/gen-vdso: minor error handling improvements

[PATCH 0/2] linux-user/gen-vdso: minor error handling improvements

[Peter Maydell]

These two small patches improve the error handling in the gen-vdso
program. Error handling isn't particularly critical here because
the tool only gets run during the QEMU build process on input
that we trust (because we generated it by calling a compiler
for the guest architecture), but these were easy to do.

The main motivation here is that Coverity complained about not
checking fseek()'s return value.

thanks
-- PMM

Peter Maydell (2):
  linux-user/gen-vdso: Handle fseek() failure
  linux-user/gen-vdso: Don't write off the end of buf[]

 linux-user/gen-vdso.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] linux-user/gen-vdso: Handle fseek() failure

[Peter Maydell]

Coverity points out that we don't check for fseek() failure in gen-vdso.c,
and so we might pass -1 to malloc(). Add the error checking.

(This is a standalone executable that doesn't link against glib, so
we can't do the easy thing and use g_file_get_contents().)

Coverity: CID 1523742
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/gen-vdso.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/linux-user/gen-vdso.c b/linux-user/gen-vdso.c
index fce9d5cbc3c..1c406d1b10f 100644
--- a/linux-user/gen-vdso.c
+++ b/linux-user/gen-vdso.c
@@ -113,9 +113,16 @@ int main(int argc, char **argv)
      * We expect the vdso to be small, on the order of one page,
      * therefore we do not expect a partial read.
      */
-    fseek(inf, 0, SEEK_END);
+    if (fseek(inf, 0, SEEK_END) < 0) {
+        goto perror_inf;
+    }
     total_len = ftell(inf);
-    fseek(inf, 0, SEEK_SET);
+    if (total_len < 0) {
+        goto perror_inf;
+    }
+    if (fseek(inf, 0, SEEK_SET) < 0) {
+        goto perror_inf;
+    }
 
     buf = malloc(total_len);
     if (buf == NULL) {
-- 
2.43.0

Re: [PATCH 1/2] linux-user/gen-vdso: Handle fseek() failure

[Richard Henderson]

On 7/10/25 11:07, Peter Maydell wrote:
> Coverity points out that we don't check for fseek() failure in gen-vdso.c,
> and so we might pass -1 to malloc(). Add the error checking.
> 
> (This is a standalone executable that doesn't link against glib, so
> we can't do the easy thing and use g_file_get_contents().)
> 
> Coverity: CID 1523742
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/gen-vdso.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/2] linux-user/gen-vdso: Don't write off the end of buf[]

[Peter Maydell]

In gen-vdso we load in a file and assume it's a valid ELF file.  In
particular we assume it's big enough to be able to read the ELF
information in e_ident in the ELF header.

Add a check that the total file length is at least big enough for all
the e_ident bytes, which is good enough for the code in gen-vdso.c.
This will catch the most obvious possible bad input file (truncated)
and allow us to run the sanity checks like "not actually an ELF file"
without potentially crashing.

The code in elf32_process() and elf64_process() still makes
assumptions about the file being well-formed, but this is OK because
we only run it on the vdso binaries that we create ourselves in the
build process by running the compiler.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Hardening all of elf*_process() seems like overkill, but this is
an easy check to add.
---
 linux-user/gen-vdso.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-user/gen-vdso.c b/linux-user/gen-vdso.c
index 1c406d1b10f..aeaa927db8f 100644
--- a/linux-user/gen-vdso.c
+++ b/linux-user/gen-vdso.c
@@ -124,6 +124,11 @@ int main(int argc, char **argv)
         goto perror_inf;
     }
 
+    if (total_len < EI_NIDENT) {
+        fprintf(stderr, "%s: file too small (truncated?)\n", inf_name);
+        return EXIT_FAILURE;
+    }
+
     buf = malloc(total_len);
     if (buf == NULL) {
         goto perror_inf;
-- 
2.43.0

Re: [PATCH 2/2] linux-user/gen-vdso: Don't write off the end of buf[]

[Richard Henderson]

On 7/10/25 11:07, Peter Maydell wrote:
> In gen-vdso we load in a file and assume it's a valid ELF file.  In
> particular we assume it's big enough to be able to read the ELF
> information in e_ident in the ELF header.
> 
> Add a check that the total file length is at least big enough for all
> the e_ident bytes, which is good enough for the code in gen-vdso.c.
> This will catch the most obvious possible bad input file (truncated)
> and allow us to run the sanity checks like "not actually an ELF file"
> without potentially crashing.
> 
> The code in elf32_process() and elf64_process() still makes
> assumptions about the file being well-formed, but this is OK because
> we only run it on the vdso binaries that we create ourselves in the
> build process by running the compiler.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> Hardening all of elf*_process() seems like overkill, but this is
> an easy check to add.
> ---
>   linux-user/gen-vdso.c | 5 +++++
>   1 file changed, 5 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/2] linux-user/gen-vdso: Don't write off the end of buf[]

[Peter Maydell]

On Thu, 10 Jul 2025 at 18:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In gen-vdso we load in a file and assume it's a valid ELF file.  In
> particular we assume it's big enough to be able to read the ELF
> information in e_ident in the ELF header.
>
> Add a check that the total file length is at least big enough for all
> the e_ident bytes, which is good enough for the code in gen-vdso.c.
> This will catch the most obvious possible bad input file (truncated)
> and allow us to run the sanity checks like "not actually an ELF file"
> without potentially crashing.
>
> The code in elf32_process() and elf64_process() still makes
> assumptions about the file being well-formed, but this is OK because
> we only run it on the vdso binaries that we create ourselves in the
> build process by running the compiler.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Hardening all of elf*_process() seems like overkill, but this is
> an easy check to add.

Subject line should say "read off the end", of course.

-- PMM

Re: [PATCH 0/2] linux-user/gen-vdso: minor error handling improvements

[Richard Henderson]

On 7/10/25 11:07, Peter Maydell wrote:
> These two small patches improve the error handling in the gen-vdso
> program. Error handling isn't particularly critical here because
> the tool only gets run during the QEMU build process on input
> that we trust (because we generated it by calling a compiler
> for the guest architecture), but these were easy to do.
> 
> The main motivation here is that Coverity complained about not
> checking fseek()'s return value.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    linux-user/gen-vdso: Handle fseek() failure
>    linux-user/gen-vdso: Don't write off the end of buf[]
> 
>   linux-user/gen-vdso.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 

Queued, thanks.

r~

Re: [PATCH 0/2] linux-user/gen-vdso: minor error handling improvements

[Peter Maydell]

On Thu, 10 Jul 2025 at 18:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/10/25 11:07, Peter Maydell wrote:
> > These two small patches improve the error handling in the gen-vdso
> > program. Error handling isn't particularly critical here because
> > the tool only gets run during the QEMU build process on input
> > that we trust (because we generated it by calling a compiler
> > for the guest architecture), but these were easy to do.
> >
> > The main motivation here is that Coverity complained about not
> > checking fseek()'s return value.
> >
> > thanks
> > -- PMM
> >
> > Peter Maydell (2):
> >    linux-user/gen-vdso: Handle fseek() failure
> >    linux-user/gen-vdso: Don't write off the end of buf[]
> >
> >   linux-user/gen-vdso.c | 16 ++++++++++++++--
> >   1 file changed, 14 insertions(+), 2 deletions(-)
> >
>
> Queued, thanks.

Would you mind fixing up the read/write thinko in the subject
line of patch 2?

thanks
-- PMM