Re: [PATCH v1 1/2] KVM: s390: gaccess: check if guest address is in memslot

[Heiko Carstens <hca@linux.ibm.com>]

On Tue, Sep 17, 2024 at 05:18:33PM +0200, Nico Boehr wrote:
> Previously, access_guest_page() did not check whether the given guest
> address is inside of a memslot. This is not a problem, since
> kvm_write_guest_page/kvm_read_guest_page return -EFAULT in this case.
...
> To be able to distinguish these two cases, return PGM_ADDRESSING in
> access_guest_page() when the guest address is outside guest memory. In
> access_guest_real(), populate vcpu->arch.pgm.code such that
> kvm_s390_inject_prog_cond() can be used in the caller for injecting into
> the guest (if applicable).
> 
> Since this adds a new return value to access_guest_page(), we need to make
> sure that other callers are not confused by the new positive return value.
> 
> There are the following users of access_guest_page():
> - access_guest_with_key() does the checking itself (in
>   guest_range_to_gpas()), so this case should never happen. Even if, the
>   handling is set up properly.
> - access_guest_real() just passes the return code to its callers, which
>   are:
>     - read_guest_real() - see below
>     - write_guest_real() - see below
> 
> There are the following users of read_guest_real():
> - ar_translation() in gaccess.c which already returns PGM_*

With this patch you actually fix a bug in ar_translation(), where two
read_guest_real() invocations might have returned -EFAULT instead of
the correct PGM_ADDRESSING.

Looks like the author assumed read_guest_real() would do the right
thing. See commit 664b49735370 ("KVM: s390: Add access register mode").

> Fixes: 2293897805c2 ("KVM: s390: add architecture compliant guest access functions")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  arch/s390/kvm/gaccess.c |  7 +++++++
>  arch/s390/kvm/gaccess.h | 14 ++++++++------
>  2 files changed, 15 insertions(+), 6 deletions(-)

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index e65f597e3044..004047578729 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -828,6 +828,9 @@ static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
>  	const gfn_t gfn = gpa_to_gfn(gpa);
>  	int rc;
>  
> +	if (!gfn_to_memslot(kvm, gfn))
> +		return PGM_ADDRESSING;
> +
>  	if (mode == GACC_STORE)
>  		rc = kvm_write_guest_page(kvm, gfn, data, offset, len);

It would be nice to not add random empty lines to stay consistent with the
existing coding style.

>  	}
> +
> +	if (rc > 0)
> +		vcpu->arch.pgm.code = rc;
> +
>  	return rc;
>  }

Same here.

But whoever applies this can change this, or not.

In any case:
Reviewed-by: Heiko Carstens <hca@linux.ibm.com>