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

[David Hildenbrand <david@redhat.com>]

On 06.08.21 10:53, Thomas Huth wrote:
> 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?

Whoops, very right. Thanks!


-- 
Thanks,

David / dhildenb