[PULL,06/35] spapr: Fail CAS if option vector table cannot be parsed
diff mbox series

Message ID 20200203061123.59150-7-david@gibson.dropbear.id.au
State New
Headers show
Series
  • Untitled series #235979
Related show

Commit Message

David Gibson Feb. 3, 2020, 6:10 a.m. UTC
From: Greg Kurz <groug@kaod.org>

Most of the option vector helpers have assertions to check their
arguments aren't null. The guest can provide an arbitrary address
for the CAS structure that would result in such null arguments.
Fail CAS with H_PARAMETER and print a warning instead of aborting
QEMU.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <157925255250.397143.10855183619366882459.stgit@bahia.lan>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Peter Maydell March 20, 2020, 5:38 p.m. UTC | #1
On Mon, 3 Feb 2020 at 06:11, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> From: Greg Kurz <groug@kaod.org>
>
> Most of the option vector helpers have assertions to check their
> arguments aren't null. The guest can provide an arbitrary address
> for the CAS structure that would result in such null arguments.
> Fail CAS with H_PARAMETER and print a warning instead of aborting
> QEMU.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <157925255250.397143.10855183619366882459.stgit@bahia.lan>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_hcall.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Hi; Coverity points out that this change introduces a
memory leak (CID 1421924):

>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f1799b1b70..ffb14641f9 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1703,7 +1703,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      ov_table = addr;
>
>      ov1_guest = spapr_ovec_parse_vector(ov_table, 1);

spapr_ovec_parse_vector() allocates memory...

> +    if (!ov1_guest) {
> +        warn_report("guest didn't provide option vector 1");
> +        return H_PARAMETER;
> +    }
>      ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
> +    if (!ov5_guest) {
> +        warn_report("guest didn't provide option vector 5");
> +        return H_PARAMETER;

...but if we take this early exit code path it is never freed
(via spapr_ovec_cleanup()).

> +    }
>      if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {
>          error_report("guest requested hash and radix MMU, which is invalid.");
>          exit(EXIT_FAILURE);

All the other error paths in the function either precede
allocation of the vectors or just call exit() rather than
returning, so this is the only leak.

thanks
-- PMM

Patch
diff mbox series

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f1799b1b70..ffb14641f9 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1703,7 +1703,15 @@  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     ov_table = addr;
 
     ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
+    if (!ov1_guest) {
+        warn_report("guest didn't provide option vector 1");
+        return H_PARAMETER;
+    }
     ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
+    if (!ov5_guest) {
+        warn_report("guest didn't provide option vector 5");
+        return H_PARAMETER;
+    }
     if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {
         error_report("guest requested hash and radix MMU, which is invalid.");
         exit(EXIT_FAILURE);