Re: [PATCH v4 18/36] KVM: S390: protvirt: Introduce instruction data area bounce buffer

[David Hildenbrand <david@redhat.com>]

On 25.02.20 08:50, Christian Borntraeger wrote:
> 
> 
> On 24.02.20 20:13, David Hildenbrand wrote:
>> On 24.02.20 12:40, Christian Borntraeger wrote:
>>> From: Janosch Frank <frankja@linux.ibm.com>
>>>
>>> Now that we can't access guest memory anymore, we have a dedicated
>>> satellite block that's a bounce buffer for instruction data.
>>>
>>> We re-use the memop interface to copy the instruction data to / from
>>> userspace. This lets us re-use a lot of QEMU code which used that
>>> interface to make logical guest memory accesses which are not possible
>>> anymore in protected mode anyway.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> [borntraeger@de.ibm.com: patch merging, splitting, fixing]
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>> [...]
>>
>>> +
>>>  long kvm_arch_vcpu_async_ioctl(struct file *filp,
>>>  			       unsigned int ioctl, unsigned long arg)
>>>  {
>>> @@ -4683,7 +4732,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>>  		struct kvm_s390_mem_op mem_op;
>>>  
>>>  		if (copy_from_user(&mem_op, argp, sizeof(mem_op)) == 0)
>>> -			r = kvm_s390_guest_mem_op(vcpu, &mem_op);
>>> +			r = kvm_s390_guest_memsida_op(vcpu, &mem_op);
>>>  		else
>>>  			r = -EFAULT;
>>>  		break;
>>> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
>>> index 014e53a41ead..cd81a58349a9 100644
>>> --- a/arch/s390/kvm/pv.c
>>> +++ b/arch/s390/kvm/pv.c
>>> @@ -33,10 +33,13 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
>>>  	if (!cc)
>>>  		free_pages(vcpu->arch.pv.stor_base,
>>>  			   get_order(uv_info.guest_cpu_stor_len));
>>> +
>>> +	free_page(sida_origin(vcpu->arch.sie_block));
>>>  	vcpu->arch.sie_block->pv_handle_cpu = 0;
>>>  	vcpu->arch.sie_block->pv_handle_config = 0;
>>>  	memset(&vcpu->arch.pv, 0, sizeof(vcpu->arch.pv));
>>>  	vcpu->arch.sie_block->sdf = 0;
>>> +	vcpu->arch.sie_block->gbea = 1;
>>
>> I am very confused why gbea is set to 1 when destroying the CPU. It's
>> otherwise never set (always 0). What's the meaning of this?
> 
> This is the guest breaking event address. So a guest (and QEMU) can read it.
> It is kind of overlaid sida and gbea. Something like this:
> 
> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> index cd81a58349a9..055bf0ec8fbb 100644
> --- a/arch/s390/kvm/pv.c
> +++ b/arch/s390/kvm/pv.c
> @@ -39,6 +39,11 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
>         vcpu->arch.sie_block->pv_handle_config = 0;
>         memset(&vcpu->arch.pv, 0, sizeof(vcpu->arch.pv));
>         vcpu->arch.sie_block->sdf = 0;
> +       /*
> +        * the sidad field (for sdf == 2) is now the gbea field (for sdf == 0).
> +        * Use the reset value of gbea to not leak the kernel pointer of the
> +        * just free sida

"freed sida."

I guess I would have set the sidad to NULL in addition before the
"vcpu->arch.sie_block->sdf = 0", so access to the sidad becomes actually
grep-able.


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

-- 
Thanks,

David / dhildenb