linux-kernel - Re: [PATCH 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks

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: <20210211174750.24d91a65.pasic@linux.ibm.com>
Date:   Thu, 11 Feb 2021 17:47:50 +0100
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>
Cc:     Cornelia Huck <cohuck@...hat.com>, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        stable@...r.kernel.org, borntraeger@...ibm.com,
        kwankhede@...dia.com, pbonzini@...hat.com,
        alex.williamson@...hat.com, pasic@...ux.vnet.ibm.com
Subject: Re: [PATCH 1/1] s390/vfio-ap: fix circular lockdep when
 setting/clearing crypto masks

On Thu, 11 Feb 2021 09:21:26 -0500
Tony Krowiak <akrowiak@...ux.ibm.com> wrote:

> Yes, it makes sense. I guess I didn't look closely at your
> suggestion when I said it was exactly what I implemented
> after agreeing with Connie. I had a slight difference in
> my implementation:
> 
> static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> {
>      struct kvm *kvm;
> 
>      mutex_lock(&matrix_dev->lock);
> 
>      if (matrix_mdev->kvm) {
>          kvm = matrix_mdev->kvm;
>          mutex_unlock(&matrix_dev->lock);

The problem with this one is that as soon as we drop
the lock here, another thread can in theory execute
the critical section below, which drops our reference
to kvm via kvm_put_kvm(kvm). Thus when we enter
kvm_arch_crypto_clear_mask(), even if we are guaranteed
to have a non-null pointer, the pointee is not guaranteed
to be around. So like Connie suggested, you better take
another reference to kvm in the first critical section.

Regards,
Halil

>          kvm_arch_crypto_clear_masks(kvm);
>          mutex_lock(&matrix_dev->lock);
>          kvm->arch.crypto.pqap_hook = NULL;
>          vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
>          matrix_mdev->kvm = NULL;
>          kvm_put_kvm(kvm);
>      }
> 
>      mutex_unlock(&matrix_dev->lock);
> }