Re: [PATCH v1 11/12] hw/s390x/s390-skeys: check if an address is valid before dumping the key

[Thomas Huth <thuth@redhat.com>]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Let's validate the given address and report a proper error in case it's
> not. All call paths now properly check the validity of the given GFN.
> Remove the TODO.
> 
> The errors inside the getter and setter should only trigger if something
> really goes wrong now, for example, with a broken migration stream. Or
> when we forget to update the storage key allocation with memory hotplug.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-skeys.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 56a47fe180..53e16f1b9c 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -18,6 +18,7 @@
>   #include "qapi/qmp/qdict.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/memory_mapping.h"
> +#include "exec/address-spaces.h"
>   #include "sysemu/kvm.h"
>   #include "migration/qemu-file-types.h"
>   #include "migration/register.h"
> @@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
>           return;
>       }
>   
> +    if (!address_space_access_valid(&address_space_memory,
> +                                    addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
> +                                    false, MEMTXATTRS_UNSPECIFIED)) {
> +        monitor_printf(mon, "Error: The given address is not valid\n");

I think the code should return here?

> +    }
> +
>       r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
>       if (r < 0) {
>           monitor_printf(mon, "Error: %s\n", strerror(-r));
> @@ -197,11 +204,6 @@ static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>       return 1;
>   }
>   
> -/*
> - * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get
> - * will have to make sure that the given gfn belongs to a memory region and not
> - * a memory hole.
> - */
>   static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
>                                 uint64_t count, uint8_t *keys)
>   {
> 

  Thomas