target/s390x/translate: Do not leak stack address in translate_one()

[PATCH] target/s390x/translate: Do not leak stack address in translate_one()

Posted by Thomas Huth 4 years, 3 months ago

The code in translate_one() leaks a stack address via "s->field" parameter:

 static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
 {
     DisasJumpType ret = DISAS_NEXT;
     DisasFields f;
     [...]
     s->fields = &f;
     [...]
     return ret;
 }

It's currently harmless since the caller does not seem to use "fields"
anymore, but let's better play safe (and please static code analyzers)
by setting the fields back to NULL before returning.

Buglink: https://bugs.launchpad.net/qemu/+bug/1661815
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4292bb0dd0..9122fb36da 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6435,6 +6435,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
     }
 #endif
 
+    s->fields = NULL;
+
     /* Advance to the next instruction.  */
     s->base.pc_next = s->pc_tmp;
     return ret;
-- 
2.18.1

Re: [PATCH] target/s390x/translate: Do not leak stack address in translate_one()

Posted by David Hildenbrand 4 years, 3 months ago


> Am 23.01.2020 um 08:05 schrieb Thomas Huth <thuth@redhat.com>:
> 
> The code in translate_one() leaks a stack address via "s->field" parameter:
> 
> static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
> {
>     DisasJumpType ret = DISAS_NEXT;
>     DisasFields f;
>     [...]
>     s->fields = &f;
>     [...]
>     return ret;
> }
> 
> It's currently harmless since the caller does not seem to use "fields"
> anymore, but let's better play safe (and please static code analyzers)
> by setting the fields back to NULL before returning.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1661815
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Hildenbrand <david@redhat.com>

> ---
> target/s390x/translate.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4292bb0dd0..9122fb36da 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -6435,6 +6435,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>     }
> #endif
> 
> +    s->fields = NULL;
> +
>     /* Advance to the next instruction.  */
>     s->base.pc_next = s->pc_tmp;
>     return ret;
> -- 
> 2.18.1
>

Re: [PATCH] target/s390x/translate: Do not leak stack address in translate_one()

Posted by Cornelia Huck 4 years, 3 months ago

On Thu, 23 Jan 2020 08:05:33 +0100
Thomas Huth <thuth@redhat.com> wrote:

> The code in translate_one() leaks a stack address via "s->field" parameter:
> 
>  static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>  {
>      DisasJumpType ret = DISAS_NEXT;
>      DisasFields f;
>      [...]
>      s->fields = &f;
>      [...]
>      return ret;
>  }
> 
> It's currently harmless since the caller does not seem to use "fields"
> anymore, but let's better play safe (and please static code analyzers)
> by setting the fields back to NULL before returning.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1661815
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/translate.c | 2 ++
>  1 file changed, 2 insertions(+)

Thanks, applied.