| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4b54bb98-593d-4ef0-8479-6e0446ee902a@linux.ibm.com>
Date: Fri, 17 Feb 2023 09:21:45 +0100
From: Pierre Morel <pmorel@...ux.ibm.com>
To: Janosch Frank <frankja@...ux.ibm.com>, david@...hat.com
Cc: kvm@...r.kernel.org, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, borntraeger@...ibm.com,
cohuck@...hat.com, thuth@...hat.com, imbrenda@...ux.ibm.com,
hca@...ux.ibm.com, gor@...ux.ibm.com, svens@...ux.ibm.com
Subject: Re: [PATCH v3 1/1] KVM: s390: vsie: clarifications on setting the
APCB
On 2/16/23 14:41, Janosch Frank wrote:
> On 2/14/23 13:28, Pierre Morel wrote:
>> The APCB is part of the CRYCB.
>> The calculation of the APCB origin can be done by adding
>> the APCB offset to the CRYCB origin.
>>
>> Current code makes confusing transformations, converting
>> the CRYCB origin to a pointer to calculate the APCB origin.
>>
>> Let's make things simpler and keep the CRYCB origin to make
>> these calculations.
>>
>> Signed-off-by: Pierre Morel <pmorel@...ux.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@...ux.ibm.com>
>
> LGTM:
> Acked-by: Janosch Frank <frankja@...ux.ibm.com>
Thanks,
Pierre
>
>> ---
>> arch/s390/kvm/vsie.c | 50 +++++++++++++++++++++++++-------------------
>> 1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index b6a0219e470a..8d6b765abf29 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -138,11 +138,15 @@ static int prepare_cpuflags(struct kvm_vcpu
>> *vcpu, struct vsie_page *vsie_page)
>> }
>> /* Copy to APCB FORMAT1 from APCB FORMAT0 */
>> static int setup_apcb10(struct kvm_vcpu *vcpu, struct kvm_s390_apcb1
>> *apcb_s,
>> - unsigned long apcb_o, struct kvm_s390_apcb1 *apcb_h)
>> + unsigned long crycb_gpa, struct kvm_s390_apcb1 *apcb_h)
>> {
>> struct kvm_s390_apcb0 tmp;
>> + unsigned long apcb_gpa;
>> - if (read_guest_real(vcpu, apcb_o, &tmp, sizeof(struct
>> kvm_s390_apcb0)))
>> + apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb0);
>> +
>> + if (read_guest_real(vcpu, apcb_gpa, &tmp,
>> + sizeof(struct kvm_s390_apcb0)))
>> return -EFAULT;
>> apcb_s->apm[0] = apcb_h->apm[0] & tmp.apm[0];
>> @@ -157,15 +161,19 @@ static int setup_apcb10(struct kvm_vcpu *vcpu,
>> struct kvm_s390_apcb1 *apcb_s,
>> * setup_apcb00 - Copy to APCB FORMAT0 from APCB FORMAT0
>> * @vcpu: pointer to the virtual CPU
>> * @apcb_s: pointer to start of apcb in the shadow crycb
>> - * @apcb_o: pointer to start of original apcb in the guest2
>> + * @crycb_gpa: guest physical address to start of original guest crycb
>> * @apcb_h: pointer to start of apcb in the guest1
>> *
>> * Returns 0 and -EFAULT on error reading guest apcb
>> */
>> static int setup_apcb00(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
>> - unsigned long apcb_o, unsigned long *apcb_h)
>> + unsigned long crycb_gpa, unsigned long *apcb_h)
>> {
>> - if (read_guest_real(vcpu, apcb_o, apcb_s,
>> + unsigned long apcb_gpa;
>> +
>> + apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb0);
>> +
>> + if (read_guest_real(vcpu, apcb_gpa, apcb_s,
>> sizeof(struct kvm_s390_apcb0)))
>> return -EFAULT;
>> @@ -178,16 +186,20 @@ static int setup_apcb00(struct kvm_vcpu *vcpu,
>> unsigned long *apcb_s,
>> * setup_apcb11 - Copy the FORMAT1 APCB from the guest to the shadow
>> CRYCB
>> * @vcpu: pointer to the virtual CPU
>> * @apcb_s: pointer to start of apcb in the shadow crycb
>> - * @apcb_o: pointer to start of original guest apcb
>> + * @crycb_gpa: guest physical address to start of original guest crycb
>> * @apcb_h: pointer to start of apcb in the host
>> *
>> * Returns 0 and -EFAULT on error reading guest apcb
>> */
>> static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
>> - unsigned long apcb_o,
>> + unsigned long crycb_gpa,
>> unsigned long *apcb_h)
>> {
>> - if (read_guest_real(vcpu, apcb_o, apcb_s,
>> + unsigned long apcb_gpa;
>> +
>> + apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb1);
>> +
>> + if (read_guest_real(vcpu, apcb_gpa, apcb_s,
>> sizeof(struct kvm_s390_apcb1)))
>> return -EFAULT;
>> @@ -200,7 +212,7 @@ static int setup_apcb11(struct kvm_vcpu *vcpu,
>> unsigned long *apcb_s,
>> * setup_apcb - Create a shadow copy of the apcb.
>> * @vcpu: pointer to the virtual CPU
>> * @crycb_s: pointer to shadow crycb
>> - * @crycb_o: pointer to original guest crycb
>> + * @crycb_gpa: guest physical address of original guest crycb
>> * @crycb_h: pointer to the host crycb
>> * @fmt_o: format of the original guest crycb.
>> * @fmt_h: format of the host crycb.
>> @@ -211,50 +223,46 @@ static int setup_apcb11(struct kvm_vcpu *vcpu,
>> unsigned long *apcb_s,
>> * Return 0 or an error number if the guest and host crycb are
>> incompatible.
>> */
>> static int setup_apcb(struct kvm_vcpu *vcpu, struct
>> kvm_s390_crypto_cb *crycb_s,
>> - const u32 crycb_o,
>> + const u32 crycb_gpa,
>> struct kvm_s390_crypto_cb *crycb_h,
>> int fmt_o, int fmt_h)
>> {
>> - struct kvm_s390_crypto_cb *crycb;
>> -
>> - crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
>> -
>> switch (fmt_o) {
>> case CRYCB_FORMAT2:
>> - if ((crycb_o & PAGE_MASK) != ((crycb_o + 256) & PAGE_MASK))
>> + if ((crycb_gpa & PAGE_MASK) != ((crycb_gpa + 256) & PAGE_MASK))
>> return -EACCES;
>> if (fmt_h != CRYCB_FORMAT2)
>> return -EINVAL;
>> return setup_apcb11(vcpu, (unsigned long *)&crycb_s->apcb1,
>> - (unsigned long) &crycb->apcb1,
>> + crycb_gpa,
>> (unsigned long *)&crycb_h->apcb1);
>> case CRYCB_FORMAT1:
>> switch (fmt_h) {
>> case CRYCB_FORMAT2:
>> return setup_apcb10(vcpu, &crycb_s->apcb1,
>> - (unsigned long) &crycb->apcb0,
>> + crycb_gpa,
>> &crycb_h->apcb1);
>> case CRYCB_FORMAT1:
>> return setup_apcb00(vcpu,
>> (unsigned long *) &crycb_s->apcb0,
>> - (unsigned long) &crycb->apcb0,
>> + crycb_gpa,
>> (unsigned long *) &crycb_h->apcb0);
>> }
>> break;
>> case CRYCB_FORMAT0:
>> - if ((crycb_o & PAGE_MASK) != ((crycb_o + 32) & PAGE_MASK))
>> + if ((crycb_gpa & PAGE_MASK) != ((crycb_gpa + 32) & PAGE_MASK))
>> return -EACCES;
>> switch (fmt_h) {
>> case CRYCB_FORMAT2:
>> return setup_apcb10(vcpu, &crycb_s->apcb1,
>> - (unsigned long) &crycb->apcb0,
>> + crycb_gpa,
>> &crycb_h->apcb1);
>> case CRYCB_FORMAT1:
>> case CRYCB_FORMAT0:
>> return setup_apcb00(vcpu,
>> (unsigned long *) &crycb_s->apcb0,
>> - (unsigned long) &crycb->apcb0,
>> + crycb_gpa,
>> (unsigned long *) &crycb_h->apcb0);
>> }
>> }
>
--
Pierre Morel
IBM Lab Boeblingen
Powered by blists - more mailing lists