Re: [PATCH v4 13/20] KVM: s390: vsie: Add struct vsie_sca with pin and unpin methods

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Guest-triggerable WARN_ON_ONCE and implicit cast of negative error code to unsigned int in pin_sca().
--

--- Patch [13]: [PATCH v4 13/20] KVM: s390: vsie: Add struct vsie_sca with pin and unpin methods ---
commit ba3e6e1a8cfd384294d05c97179c56428f5bc9cf
Author: Christoph Schlameuss <[email protected]>

KVM: s390: vsie: Add struct vsie_sca with pin and unpin methods

Introduce the struct vsie_sca to hold the SSCA and required management
data along with alloc and free methods to use with the struct. On this
basis we can also add methods {,un}pin_sca() to simplify the pinning
SCAs in g1.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 442e7997ee51b..e632311222002 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c

[ ... ]

> @@ -845,6 +872,74 @@ static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	return 0;
>  }
>  
> +/*
> + * Unpin g2 original sca in g1 memory.
> + *
> + * Called with vsie_sca_lock held.
> + */
> +static void unpin_sca(struct kvm *kvm, struct vsie_sca *vsie_sca)

[ ... ]

> +/*
> + * Pin g2 original sca in g1 memory.
> + *
> + * Called with vsie_sca_lock held.
> + */
> +static int pin_sca(struct kvm *kvm, struct vsie_sca *vsie_sca)
> +{
> +	bool is_esca = test_bit(VSIE_SCA_ESCA, &vsie_sca->flags);
> +	gpa_t offset = vsie_sca->sca_gpa & ~PAGE_MASK;
> +	int nr_pages;
> +
> +	if (test_bit(VSIE_SCA_SCA_PINNED, &vsie_sca->flags))
> +		return 0;
> +
> +	if (is_esca) {
> +		nr_pages = 4;
> +		if (offset + sizeof(struct esca_block) > 4 * PAGE_SIZE)
> +			nr_pages = 5;
> +	} else {
> +		nr_pages = 1;
> +		if (offset + sizeof(struct bsca_block) > PAGE_SIZE)
> +			nr_pages = 2;
> +	}
> +
> +	vsie_sca->sca_o_nr_pages = pin_guest_pages(kvm, vsie_sca->sca_gpa, nr_pages,
> +						   vsie_sca->sca_o_pages);
> +	if (WARN_ON_ONCE(vsie_sca->sca_o_nr_pages != nr_pages))
> +		return -EIO;

[Severity: High]
Can a malicious guest trigger this WARN_ON_ONCE?

If the guest provides an invalid or unmapped physical address for the SCA,
pin_guest_pages() can fail and return a negative error code (like -EINVAL).

Since vsie_sca->sca_o_nr_pages is declared as an unsigned int, the negative 
error code is implicitly cast to a large unsigned value. This causes the 
subsequent comparison vsie_sca->sca_o_nr_pages != nr_pages to evaluate to true, 
triggering the WARN_ON_ONCE.

Because guest-reachable warnings can cause a host panic when panic_on_warn is 
enabled, should this code gracefully handle the error code from 
pin_guest_pages() and return it without warning?

> +	__set_bit(VSIE_SCA_SCA_PINNED, &vsie_sca->flags);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13