Re: [PATCH 1/7] sparc: Fix property/field size mismatch for iu-version

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 04/11/2020 17:25, Eduardo Habkost wrote:

> The "iu-version" property is declared as uint64_t but points to a
> target_ulong struct field.  On the sparc 32-bit target, This
> makes every write of iu-version corrupt the 4 bytes after
> sparc_def_t.iu_version (where the fpu_version field is located).
> 
> Change the type of the iu_version struct field to uint64_t,
> and just use DEFINE_PROP_UINT64.
> 
> The only place where env.def.iu_version field is read is the
>      env->version = env->def.iu_version;
> assignment at sparc_cpu_realizefn().  This means behavior will be
> kept exactly the same (except for the memory corruption bug fix).
> 
> It would be nice to explicitly validate iu_version against
> target_ulong limits, but that would be a new feature (since the
> first version of this code, iu-version was parsed as 64-bit value
> value).  It can be done later, once we have an appropriate API to
> define limits for integer properties.
> 
> Fixes: de05005bf785 ("sparc: convert cpu features to qdev properties")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: qemu-devel@nongnu.org
> ---
>   target/sparc/cpu.h | 2 +-
>   target/sparc/cpu.c | 5 ++---
>   2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> index b9369398f2..8ed0f4fef3 100644
> --- a/target/sparc/cpu.h
> +++ b/target/sparc/cpu.h
> @@ -252,7 +252,7 @@ typedef struct trap_state {
>   
>   struct sparc_def_t {
>       const char *name;
> -    target_ulong iu_version;
> +    uint64_t iu_version;
>       uint32_t fpu_version;
>       uint32_t mmu_version;
>       uint32_t mmu_bm;
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index ec59a13eb8..5a9397f19a 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -576,7 +576,7 @@ void sparc_cpu_list(void)
>       unsigned int i;
>   
>       for (i = 0; i < ARRAY_SIZE(sparc_defs); i++) {
> -        qemu_printf("Sparc %16s IU " TARGET_FMT_lx
> +        qemu_printf("Sparc %16s IU %" PRIx64
>                       " FPU %08x MMU %08x NWINS %d ",
>                       sparc_defs[i].name,
>                       sparc_defs[i].iu_version,
> @@ -838,8 +838,7 @@ static Property sparc_cpu_properties[] = {
>       DEFINE_PROP_BIT("hypv",     SPARCCPU, env.def.features, 11, false),
>       DEFINE_PROP_BIT("cmt",      SPARCCPU, env.def.features, 12, false),
>       DEFINE_PROP_BIT("gl",       SPARCCPU, env.def.features, 13, false),
> -    DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0,
> -                         prop_info_uint64, target_ulong),
> +    DEFINE_PROP_UINT64("iu-version", SPARCCPU, env.def.iu_version, 0),
>       DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0),
>       DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0),
>       DEFINE_PROP("nwindows",     SPARCCPU, env.def.nwindows,

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.