diff mbox series

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

Message ID 20200123070533.19699-1-thuth@redhat.com
State New
Headers show
Series target/s390x/translate: Do not leak stack address in translate_one() | expand

Commit Message

Thomas Huth Jan. 23, 2020, 7:05 a.m. UTC 
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(+)

Comments

David Hildenbrand Jan. 23, 2020, 7:49 a.m. UTC | #1 
> 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
>
Cornelia Huck Jan. 23, 2020, 10:25 a.m. UTC | #2 
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.
diff mbox series

Patch

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;