linux-kernel - Re: [PATCH v11 11/26] s390: vfio-ap: implement mediated device open callback

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Sep 2018 12:14:29 +0200
From:   Cornelia Huck <cohuck@...hat.com>
To:     Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, freude@...ibm.com, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, borntraeger@...ibm.com,
        kwankhede@...dia.com, bjsdjshi@...ux.vnet.ibm.com,
        pbonzini@...hat.com, alex.williamson@...hat.com,
        pmorel@...ux.vnet.ibm.com, alifm@...ux.vnet.ibm.com,
        mjrosato@...ux.vnet.ibm.com, jjherne@...ux.vnet.ibm.com,
        thuth@...hat.com, pasic@...ux.vnet.ibm.com, berrange@...hat.com,
        fiuczy@...ux.vnet.ibm.com, buendgen@...ibm.com,
        frankja@...ux.ibm.com, Tony Krowiak <akrowiak@...ux.ibm.com>
Subject: Re: [PATCH v11 11/26] s390: vfio-ap: implement mediated device open
 callback

On Tue, 25 Sep 2018 19:16:26 -0400
Tony Krowiak <akrowiak@...ux.vnet.ibm.com> wrote:

> From: Tony Krowiak <akrowiak@...ux.ibm.com>
> 
> Implements the open callback on the mediated matrix device.
> The function registers a group notifier to receive notification
> of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
> the vfio_ap device driver will get access to the guest's
> kvm structure. The open callback must ensure that only one
> mediated device shall be opened per guest.
> 
> Signed-off-by: Tony Krowiak <akrowiak@...ux.ibm.com>
> Acked-by: Halil Pasic <pasic@...ux.ibm.com>
> Tested-by: Michael Mueller <mimu@...ux.ibm.com>
> Tested-by: Farhan Ali <alifm@...ux.ibm.com>
> Tested-by: Pierre Morel <pmorel@...ux.ibm.com>
> Acked-by: Pierre Morel <pmorel@...ux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h      |   1 +
>  drivers/s390/crypto/vfio_ap_ops.c     | 159 ++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_private.h |   5 +
>  3 files changed, 165 insertions(+)
> 

> +static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	int nbytes;
> +	unsigned long *apm, *aqm, *adm;
> +	struct kvm_s390_crypto_cb *crycb = matrix_mdev->kvm->arch.crypto.crycb;
> +
> +	switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> +	case CRYCB_FORMAT2:
> +		apm = (unsigned long *)crycb->apcb1.apm;
> +		aqm = (unsigned long *)crycb->apcb1.aqm;
> +		adm = (unsigned long *)crycb->apcb1.adm;
> +		break;
> +	case CRYCB_FORMAT1:
> +	case CRYCB_FORMAT0:
> +	default:

That one looks a tad odd: the masking above should guarantee that we
hit either the format 2, 1, or 0 cases, so the default case should
never hit. Moreover, if the mask above will ever be changed, we don't
know where we should look for the correct masks (what will a FORMAT<n>
use: apcb1, apcb0, or something else?) Would it be better to moan in
the default case and copy nothing?

> +		apm = (unsigned long *)crycb->apcb0.apm;
> +		aqm = (unsigned long *)crycb->apcb0.aqm;
> +		adm = (unsigned long *)crycb->apcb0.adm;
> +		break;
> +	}
> +
> +	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.apm_max + 1, BITS_PER_BYTE);
> +	memcpy(apm, matrix_mdev->matrix.apm, nbytes);
> +	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.aqm_max + 1, BITS_PER_BYTE);
> +	memcpy(aqm, matrix_mdev->matrix.aqm, nbytes);
> +	nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE);
> +	memcpy(adm, matrix_mdev->matrix.adm, nbytes);
> +}

Anyway, that can be easily changed later on; in general,

Reviewed-by: Cornelia Huck <cohuck@...hat.com>