linux-kernel - Re: [PATCH] s390/vfio-ap: fix unregister GISC when KVM is already gone results in OOPS

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]

Message-ID: <20200921135655.152c77c6.pasic@linux.ibm.com>
Date:   Mon, 21 Sep 2020 13:56:55 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Christian Borntraeger <borntraeger@...ibm.com>
Cc:     Tony Krowiak <akrowiak@...ux.ibm.com>, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        pmorel@...ux.ibm.com, alex.williamson@...hat.com,
        cohuck@...hat.com, kwankhede@...dia.com
Subject: Re: [PATCH] s390/vfio-ap: fix unregister GISC when KVM is already
 gone results in OOPS

On Mon, 21 Sep 2020 07:48:58 +0200
Christian Borntraeger <borntraeger@...ibm.com> wrote:

> 
> 
> On 18.09.20 19:02, Tony Krowiak wrote:
> > Attempting to unregister Guest Interruption Subclass (GISC) when the
> > link between the matrix mdev and KVM has been removed results in the
> > following:
> > 
> >    "Kernel panic -not syncing: Fatal exception: panic_on_oops"
> 
> I think the full backtrace would be better in case someone runs into this
> and needs to compare this patch to its oops. This also makes it easier to
> understand the fix. 
> > 
> > This patch fixes this bug by verifying the matrix mdev and KVM are still
> > linked prior to unregistering the GISC.
> > 
> > Signed-off-by: Tony Krowiak <akrowiak@...ux.ibm.com>
> 
> Do we need a Fixes tag and cc stable?
> 

I believe we do!

> 
> > ---
> >  drivers/s390/crypto/vfio_ap_ops.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > index e0bde8518745..847a88642644 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -119,11 +119,15 @@ static void vfio_ap_wait_for_irqclear(int apqn)
> >   */
> >  static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
> >  {
> > -	if (q->saved_isc != VFIO_AP_ISC_INVALID && q->matrix_mdev)
> 
> So we already check for q->matrix_mdev here
> 
> > -		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
> > -	if (q->saved_pfn && q->matrix_mdev)
> 
> and here
> > -		vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
> > -				 &q->saved_pfn, 1);
> > +	if (q->matrix_mdev) {
> > +		if (q->saved_isc != VFIO_AP_ISC_INVALID && q->matrix_mdev->kvm)
>                                                            ^^^^ and this is the only
> 		new check? Cant we just add this condition to the first if?

You are technically right, but I'm not comfortable with my level of
understanding of this logic regardless of the coding style. Will ask
some questions directly.

Regards,
Halil

> 
> > +			kvm_s390_gisc_unregister(q->matrix_mdev->kvm,
> > +						 q->saved_isc);
> > +		if (q->saved_pfn)
> > +			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
> > +					 &q->saved_pfn, 1);
> > +	}
> > +
> >  	q->saved_pfn = 0;
> >  	q->saved_isc = VFIO_AP_ISC_INVALID;
> >  }
> >