[PATCH v1 0/2] KVM: s390: Fix two bugs

[PATCH v1 0/2] KVM: s390: Fix two bugs

[Claudio Imbrenda]

This small series fixes two bugs in s390 KVM. One is small and trivial,
the other is pretty bad.

* The wrong type of flag was being passed to vcpu_dat_fault_handler();
  it expects a FOLL_* flag, but a FAULT_FLAG_* was passed instead.
* Due to incorrect usage of mmu_notifier_register(), in some rare cases
  when running a secure guest, the struct mm for the userspace process
  would get freed too early and cause a use-after-free.

Claudio Imbrenda (2):
  KVM: s390: Fix incorrect usage of mmu_notifier_register()
  KVM: s390: Fix FOLL_*/FAULT_FLAG_* confusion

 arch/s390/kvm/kvm-s390.c |  8 ++++----
 arch/s390/kvm/pv.c       | 14 +++++++++-----
 2 files changed, 13 insertions(+), 9 deletions(-)

-- 
2.50.1

[PATCH v1 1/2] KVM: s390: Fix incorrect usage of mmu_notifier_register()

[Claudio Imbrenda]

If mmu_notifier_register() fails, for example because a signal was
pending, the mmu_notifier will not be registered. But when the VM gets
destroyed, it will get unregistered anyway and that will cause one
extra mmdrop(), which will eventually cause the mm of the process to
be freed too early, and cause a use-after free.

This bug happens rarely, and only when secure guests are involved.

The solution is to check the return value of mmu_notifier_register()
and return it to the caller (ultimately it will be propagated all the
way to userspace). In case of -EINTR, userspace will try again.

Fixes: ca2fd0609b5d ("KVM: s390: pv: add mmu_notifier")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/pv.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index 14c330ec8ceb..e85fb3247b0e 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -622,6 +622,15 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
 	int cc, ret;
 	u16 dummy;
 
+	/* Add the notifier only once. No races because we hold kvm->lock */
+	if (kvm->arch.pv.mmu_notifier.ops != &kvm_s390_pv_mmu_notifier_ops) {
+		ret = mmu_notifier_register(&kvm->arch.pv.mmu_notifier, kvm->mm);
+		if (ret)
+			return ret;
+		/* The notifier will be unregistered when the VM is destroyed */
+		kvm->arch.pv.mmu_notifier.ops = &kvm_s390_pv_mmu_notifier_ops;
+	}
+
 	ret = kvm_s390_pv_alloc_vm(kvm);
 	if (ret)
 		return ret;
@@ -657,11 +666,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
 		return -EIO;
 	}
 	kvm->arch.gmap->guest_handle = uvcb.guest_handle;
-	/* Add the notifier only once. No races because we hold kvm->lock */
-	if (kvm->arch.pv.mmu_notifier.ops != &kvm_s390_pv_mmu_notifier_ops) {
-		kvm->arch.pv.mmu_notifier.ops = &kvm_s390_pv_mmu_notifier_ops;
-		mmu_notifier_register(&kvm->arch.pv.mmu_notifier, kvm->mm);
-	}
 	return 0;
 }
 
-- 
2.50.1

Re: [PATCH v1 1/2] KVM: s390: Fix incorrect usage of mmu_notifier_register()

[Christian Borntraeger]

Am 05.08.25 um 13:14 schrieb Claudio Imbrenda:
> If mmu_notifier_register() fails, for example because a signal was
> pending, the mmu_notifier will not be registered. But when the VM gets
> destroyed, it will get unregistered anyway and that will cause one
> extra mmdrop(), which will eventually cause the mm of the process to
> be freed too early, and cause a use-after free.
> 
> This bug happens rarely, and only when secure guests are involved.
> 
> The solution is to check the return value of mmu_notifier_register()
> and return it to the caller (ultimately it will be propagated all the
> way to userspace). In case of -EINTR, userspace will try again.
> 
> Fixes: ca2fd0609b5d ("KVM: s390: pv: add mmu_notifier")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   arch/s390/kvm/pv.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> index 14c330ec8ceb..e85fb3247b0e 100644
> --- a/arch/s390/kvm/pv.c
> +++ b/arch/s390/kvm/pv.c
> @@ -622,6 +622,15 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
>   	int cc, ret;
>   	u16 dummy;
>   
> +	/* Add the notifier only once. No races because we hold kvm->lock */
> +	if (kvm->arch.pv.mmu_notifier.ops != &kvm_s390_pv_mmu_notifier_ops) {
> +		ret = mmu_notifier_register(&kvm->arch.pv.mmu_notifier, kvm->mm);
> +		if (ret)
> +			return ret;
> +		/* The notifier will be unregistered when the VM is destroyed */
> +		kvm->arch.pv.mmu_notifier.ops = &kvm_s390_pv_mmu_notifier_ops;
> +	}
> +
>   	ret = kvm_s390_pv_alloc_vm(kvm);
>   	if (ret)
>   		return ret;
> @@ -657,11 +666,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
>   		return -EIO;
>   	}
>   	kvm->arch.gmap->guest_handle = uvcb.guest_handle;
> -	/* Add the notifier only once. No races because we hold kvm->lock */
> -	if (kvm->arch.pv.mmu_notifier.ops != &kvm_s390_pv_mmu_notifier_ops) {
> -		kvm->arch.pv.mmu_notifier.ops = &kvm_s390_pv_mmu_notifier_ops;
> -		mmu_notifier_register(&kvm->arch.pv.mmu_notifier, kvm->mm);
> -	}
>   	return 0;
>   }
>   

Re: [PATCH v1 1/2] KVM: s390: Fix incorrect usage of mmu_notifier_register()

[David Hildenbrand]

On 05.08.25 13:14, Claudio Imbrenda wrote:
> If mmu_notifier_register() fails, for example because a signal was
> pending, the mmu_notifier will not be registered. But when the VM gets
> destroyed, it will get unregistered anyway and that will cause one
> extra mmdrop(), which will eventually cause the mm of the process to
> be freed too early, and cause a use-after free.
> 
> This bug happens rarely, and only when secure guests are involved.
> 
> The solution is to check the return value of mmu_notifier_register()
> and return it to the caller (ultimately it will be propagated all the
> way to userspace). In case of -EINTR, userspace will try again.
> 
> Fixes: ca2fd0609b5d ("KVM: s390: pv: add mmu_notifier")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] KVM: s390: Fix incorrect usage of mmu_notifier_register()

[Steffen Eiden]

On Tue, Aug 05, 2025 at 01:14:45PM +0200, Claudio Imbrenda wrote:
> If mmu_notifier_register() fails, for example because a signal was
> pending, the mmu_notifier will not be registered. But when the VM gets
> destroyed, it will get unregistered anyway and that will cause one
> extra mmdrop(), which will eventually cause the mm of the process to
> be freed too early, and cause a use-after free.
> 
> This bug happens rarely, and only when secure guests are involved.
> 
> The solution is to check the return value of mmu_notifier_register()
> and return it to the caller (ultimately it will be propagated all the
> way to userspace). In case of -EINTR, userspace will try again.
> 
> Fixes: ca2fd0609b5d ("KVM: s390: pv: add mmu_notifier")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [PATCH v1 1/2] KVM: s390: Fix incorrect usage of mmu_notifier_register()

[Christoph Schlameuss]

On Tue Aug 5, 2025 at 1:14 PM CEST, Claudio Imbrenda wrote:
> If mmu_notifier_register() fails, for example because a signal was
> pending, the mmu_notifier will not be registered. But when the VM gets
> destroyed, it will get unregistered anyway and that will cause one
> extra mmdrop(), which will eventually cause the mm of the process to
> be freed too early, and cause a use-after free.
>
> This bug happens rarely, and only when secure guests are involved.
>
> The solution is to check the return value of mmu_notifier_register()
> and return it to the caller (ultimately it will be propagated all the
> way to userspace). In case of -EINTR, userspace will try again.
>
> Fixes: ca2fd0609b5d ("KVM: s390: pv: add mmu_notifier")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/kvm/pv.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> index 14c330ec8ceb..e85fb3247b0e 100644
> --- a/arch/s390/kvm/pv.c
> +++ b/arch/s390/kvm/pv.c
> @@ -622,6 +622,15 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
>  	int cc, ret;
>  	u16 dummy;
>  
> +	/* Add the notifier only once. No races because we hold kvm->lock */
> +	if (kvm->arch.pv.mmu_notifier.ops != &kvm_s390_pv_mmu_notifier_ops) {
> +		ret = mmu_notifier_register(&kvm->arch.pv.mmu_notifier, kvm->mm);
> +		if (ret)
> +			return ret;
> +		/* The notifier will be unregistered when the VM is destroyed */
> +		kvm->arch.pv.mmu_notifier.ops = &kvm_s390_pv_mmu_notifier_ops;
> +	}
> +
>  	ret = kvm_s390_pv_alloc_vm(kvm);
>  	if (ret)
>  		return ret;
> @@ -657,11 +666,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
>  		return -EIO;
>  	}
>  	kvm->arch.gmap->guest_handle = uvcb.guest_handle;
> -	/* Add the notifier only once. No races because we hold kvm->lock */
> -	if (kvm->arch.pv.mmu_notifier.ops != &kvm_s390_pv_mmu_notifier_ops) {
> -		kvm->arch.pv.mmu_notifier.ops = &kvm_s390_pv_mmu_notifier_ops;
> -		mmu_notifier_register(&kvm->arch.pv.mmu_notifier, kvm->mm);
> -	}
>  	return 0;
>  }
>  

[PATCH v1 2/2] KVM: s390: Fix FOLL_*/FAULT_FLAG_* confusion

[Claudio Imbrenda]

Pass the right type of flag to vcpu_dat_fault_handler(); it expects a
FOLL_* flag (in particular FOLL_WRITE), but FAULT_FLAG_WRITE is passed
instead.

This still works because they happen to have the same integer value,
but it's a mistake, thus the fix.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: 05066cafa925 ("s390/mm/fault: Handle guest-related program interrupts in KVM")
---
 arch/s390/kvm/kvm-s390.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d5ad10791c25..d41d77f2c7cd 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4954,13 +4954,13 @@ static int vcpu_dat_fault_handler(struct kvm_vcpu *vcpu, unsigned long gaddr, un
 
 static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
 {
-	unsigned int flags = 0;
+	unsigned int foll = 0;
 	unsigned long gaddr;
 	int rc;
 
 	gaddr = current->thread.gmap_teid.addr * PAGE_SIZE;
 	if (kvm_s390_cur_gmap_fault_is_write())
-		flags = FAULT_FLAG_WRITE;
+		foll = FOLL_WRITE;
 
 	switch (current->thread.gmap_int_code & PGM_INT_CODE_MASK) {
 	case 0:
@@ -5002,7 +5002,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
 			send_sig(SIGSEGV, current, 0);
 		if (rc != -ENXIO)
 			break;
-		flags = FAULT_FLAG_WRITE;
+		foll = FOLL_WRITE;
 		fallthrough;
 	case PGM_PROTECTION:
 	case PGM_SEGMENT_TRANSLATION:
@@ -5012,7 +5012,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
 	case PGM_REGION_SECOND_TRANS:
 	case PGM_REGION_THIRD_TRANS:
 		kvm_s390_assert_primary_as(vcpu);
-		return vcpu_dat_fault_handler(vcpu, gaddr, flags);
+		return vcpu_dat_fault_handler(vcpu, gaddr, foll);
 	default:
 		KVM_BUG(1, vcpu->kvm, "Unexpected program interrupt 0x%x, TEID 0x%016lx",
 			current->thread.gmap_int_code, current->thread.gmap_teid.val);
-- 
2.50.1

Re: [PATCH v1 2/2] KVM: s390: Fix FOLL_*/FAULT_FLAG_* confusion

[Christian Borntraeger]

Am 05.08.25 um 13:14 schrieb Claudio Imbrenda:
> Pass the right type of flag to vcpu_dat_fault_handler(); it expects a
> FOLL_* flag (in particular FOLL_WRITE), but FAULT_FLAG_WRITE is passed
> instead.
> 
> This still works because they happen to have the same integer value,
> but it's a mistake, thus the fix.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Fixes: 05066cafa925 ("s390/mm/fault: Handle guest-related program interrupts in KVM")

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Shouldnt we rename the parameter to __kvm_s390_handle_dat_fault and
vcpu_dat_fault_handler from flags to foll as well in their
implementation and prototypes to keep this consistent?

> ---
>   arch/s390/kvm/kvm-s390.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d5ad10791c25..d41d77f2c7cd 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4954,13 +4954,13 @@ static int vcpu_dat_fault_handler(struct kvm_vcpu *vcpu, unsigned long gaddr, un
>   
>   static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
>   {
> -	unsigned int flags = 0;
> +	unsigned int foll = 0;
>   	unsigned long gaddr;
>   	int rc;
>   
>   	gaddr = current->thread.gmap_teid.addr * PAGE_SIZE;
>   	if (kvm_s390_cur_gmap_fault_is_write())
> -		flags = FAULT_FLAG_WRITE;
> +		foll = FOLL_WRITE;
>   
>   	switch (current->thread.gmap_int_code & PGM_INT_CODE_MASK) {
>   	case 0:
> @@ -5002,7 +5002,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
>   			send_sig(SIGSEGV, current, 0);
>   		if (rc != -ENXIO)
>   			break;
> -		flags = FAULT_FLAG_WRITE;
> +		foll = FOLL_WRITE;
>   		fallthrough;
>   	case PGM_PROTECTION:
>   	case PGM_SEGMENT_TRANSLATION:
> @@ -5012,7 +5012,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
>   	case PGM_REGION_SECOND_TRANS:
>   	case PGM_REGION_THIRD_TRANS:
>   		kvm_s390_assert_primary_as(vcpu);
> -		return vcpu_dat_fault_handler(vcpu, gaddr, flags);
> +		return vcpu_dat_fault_handler(vcpu, gaddr, foll);
>   	default:
>   		KVM_BUG(1, vcpu->kvm, "Unexpected program interrupt 0x%x, TEID 0x%016lx",
>   			current->thread.gmap_int_code, current->thread.gmap_teid.val);

Re: [PATCH v1 2/2] KVM: s390: Fix FOLL_*/FAULT_FLAG_* confusion

[Claudio Imbrenda]

On Tue, 5 Aug 2025 13:44:04 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 05.08.25 um 13:14 schrieb Claudio Imbrenda:
> > Pass the right type of flag to vcpu_dat_fault_handler(); it expects a
> > FOLL_* flag (in particular FOLL_WRITE), but FAULT_FLAG_WRITE is passed
> > instead.
> > 
> > This still works because they happen to have the same integer value,
> > but it's a mistake, thus the fix.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > Fixes: 05066cafa925 ("s390/mm/fault: Handle guest-related program interrupts in KVM")  
> 
> Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> 
> Shouldnt we rename the parameter to __kvm_s390_handle_dat_fault and
> vcpu_dat_fault_handler from flags to foll as well in their
> implementation and prototypes to keep this consistent?

that's a fair point

a patch in an upcoming series will do that, but I guess I can move that
change here instead.

I'll send a v2 later on today

> 
> > ---
> >   arch/s390/kvm/kvm-s390.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index d5ad10791c25..d41d77f2c7cd 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -4954,13 +4954,13 @@ static int vcpu_dat_fault_handler(struct kvm_vcpu *vcpu, unsigned long gaddr, un
> >   
> >   static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
> >   {
> > -	unsigned int flags = 0;
> > +	unsigned int foll = 0;
> >   	unsigned long gaddr;
> >   	int rc;
> >   
> >   	gaddr = current->thread.gmap_teid.addr * PAGE_SIZE;
> >   	if (kvm_s390_cur_gmap_fault_is_write())
> > -		flags = FAULT_FLAG_WRITE;
> > +		foll = FOLL_WRITE;
> >   
> >   	switch (current->thread.gmap_int_code & PGM_INT_CODE_MASK) {
> >   	case 0:
> > @@ -5002,7 +5002,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
> >   			send_sig(SIGSEGV, current, 0);
> >   		if (rc != -ENXIO)
> >   			break;
> > -		flags = FAULT_FLAG_WRITE;
> > +		foll = FOLL_WRITE;
> >   		fallthrough;
> >   	case PGM_PROTECTION:
> >   	case PGM_SEGMENT_TRANSLATION:
> > @@ -5012,7 +5012,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
> >   	case PGM_REGION_SECOND_TRANS:
> >   	case PGM_REGION_THIRD_TRANS:
> >   		kvm_s390_assert_primary_as(vcpu);
> > -		return vcpu_dat_fault_handler(vcpu, gaddr, flags);
> > +		return vcpu_dat_fault_handler(vcpu, gaddr, foll);
> >   	default:
> >   		KVM_BUG(1, vcpu->kvm, "Unexpected program interrupt 0x%x, TEID 0x%016lx",
> >   			current->thread.gmap_int_code, current->thread.gmap_teid.val);  
> 

Re: [PATCH v1 2/2] KVM: s390: Fix FOLL_*/FAULT_FLAG_* confusion

[David Hildenbrand]

On 05.08.25 13:14, Claudio Imbrenda wrote:
> Pass the right type of flag to vcpu_dat_fault_handler(); it expects a
> FOLL_* flag (in particular FOLL_WRITE), but FAULT_FLAG_WRITE is passed
> instead.
> 
> This still works because they happen to have the same integer value,
> but it's a mistake, thus the fix.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Fixes: 05066cafa925 ("s390/mm/fault: Handle guest-related program interrupts in KVM")
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb