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@xxxxxxxxxxxxxxx
> Signed-off-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>
