linux-kernel - Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

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:   Tue, 30 Apr 2019 10:18:20 +0200
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     borntraeger@...ibm.com, alex.williamson@...hat.com,
        cohuck@...hat.com, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, kvm@...r.kernel.org,
        frankja@...ux.ibm.com, akrowiak@...ux.ibm.com, david@...hat.com,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        freude@...ux.ibm.com, mimu@...ux.ibm.com
Subject: Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in
 kernel

On 29/04/2019 18:50, Halil Pasic wrote:
> On Fri, 26 Apr 2019 15:01:27 +0200
> Pierre Morel <pmorel@...ux.ibm.com> wrote:
> 
>> +static struct ap_queue_status vfio_ap_setirq(struct vfio_ap_queue *q)
>> +{
>> +	struct ap_qirq_ctrl aqic_gisa = {};
>> +	struct ap_queue_status status = {};
>> +	struct kvm_s390_gisa *gisa;
>> +	struct kvm *kvm;
>> +	unsigned long h_nib, h_pfn;
>> +	int ret;
>> +
>> +	q->a_pfn = q->a_nib >> PAGE_SHIFT;
>> +	ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1,
>> +			     IOMMU_READ | IOMMU_WRITE, &h_pfn);
>> +	switch (ret) {
>> +	case 1:
>> +		break;
>> +	case -EINVAL:
>> +	case -E2BIG:
>> +		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
>> +		/* Fallthrough */
>> +	default:
>> +		return status;
> 
> Can we actually hit the default label? AFICT you would return an
> all-zero status, i.e. status.response_code == 0 'Normal completion'.

hum right, the setting of AP_INVALID_ADDRESS should be in the default 
and there is no need for the two error cases, they will match the default.


> 
>> +	}
>> +
>> +	kvm = q->matrix_mdev->kvm;
>> +	gisa = kvm->arch.gisa_int.origin;
>> +
>> +	h_nib = (h_pfn << PAGE_SHIFT) | (q->a_nib & ~PAGE_MASK);
>> +	aqic_gisa.gisc = q->a_isc;
>> +	aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->a_isc);
>> +	aqic_gisa.ir = 1;
>> +	aqic_gisa.gisa = gisa->next_alert >> 4;
> 
> Why gisa->next_alert? Isn't this supposed to get set to gisa origin
> (without some bits on the left)?

Someone already asked this question.
The answer is: look at the ap_qirq_ctrl structure, you will see that the 
gisa field is 27 bits wide.

> 
>> +
>> +	status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
>> +	switch (status.response_code) {
>> +	case AP_RESPONSE_NORMAL:
>> +		/* See if we did clear older IRQ configuration */
>> +		if (q->p_pfn)
>> +			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
>> +					 &q->p_pfn, 1);
>> +		if (q->p_isc != VFIO_AP_ISC_INVALID)
>> +			kvm_s390_gisc_unregister(kvm, q->p_isc);
>> +		q->p_pfn = q->a_pfn;
>> +		q->p_isc = q->a_isc;
>> +		break;
>> +	case AP_RESPONSE_OTHERWISE_CHANGED:
>> +		/* We could not modify IRQ setings: clear new configuration */
>> +		vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1);
>> +		kvm_s390_gisc_unregister(kvm, q->a_isc);
> 
> Hm, see below. Wouldn't you want to set a_isc to VFIO_AP_ISC_INVALID?

grrr!!! when did I insert these 3 lines, it was OK in previous series!
all 3 lines, vfio_unpin() / gisc_unregister / break must go away.

> 
>> +		break;
>> +	default:	/* Fall Through */
> 
> Is it 'break' or is it 'Fall Through'?

it is a fall through

> 
>> +		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>> +			status.response_code);
>> +		vfio_ap_free_irq_data(q);
>> +		break;
>> +	}
>> +
>> +	return status;
>> +}


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany