[PATCH v2 0/1] fix race in gmap_make_secure

[PATCH v2 0/1] fix race in gmap_make_secure

[Claudio Imbrenda]

This fixes a potential race and, most importantly, removes the last
user of follow_page without FOLL_GET.

This removes the last user of follow_page without FOLL_GET. Once this
patch is merged I'll send a separate patch to perform FOLL_GET
automatically with follow_page, as suggested by Jason Gunthorpe.

Claudio Imbrenda (1):
  KVM: s390: fix race in gmap_make_secure

 arch/s390/kernel/uv.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

-- 
2.40.0

[PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure

[Claudio Imbrenda]

This patch fixes a potential race in gmap_make_secure and removes the
last user of follow_page without FOLL_GET.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
---
 arch/s390/kernel/uv.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 9f18a4af9c13..cb2ee06df286 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -192,21 +192,10 @@ static int expected_page_refs(struct page *page)
 	return res;
 }
 
-static int make_secure_pte(pte_t *ptep, unsigned long addr,
-			   struct page *exp_page, struct uv_cb_header *uvcb)
+static int make_page_secure(struct page *page, struct uv_cb_header *uvcb)
 {
-	pte_t entry = READ_ONCE(*ptep);
-	struct page *page;
 	int expected, cc = 0;
 
-	if (!pte_present(entry))
-		return -ENXIO;
-	if (pte_val(entry) & _PAGE_INVALID)
-		return -ENXIO;
-
-	page = pte_page(entry);
-	if (page != exp_page)
-		return -ENXIO;
 	if (PageWriteback(page))
 		return -EAGAIN;
 	expected = expected_page_refs(page);
@@ -304,17 +293,18 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
 		goto out;
 
 	rc = -ENXIO;
-	page = follow_page(vma, uaddr, FOLL_WRITE);
-	if (IS_ERR_OR_NULL(page))
-		goto out;
-
-	lock_page(page);
 	ptep = get_locked_pte(gmap->mm, uaddr, &ptelock);
-	if (should_export_before_import(uvcb, gmap->mm))
-		uv_convert_from_secure(page_to_phys(page));
-	rc = make_secure_pte(ptep, uaddr, page, uvcb);
+	if (pte_present(*ptep) && !(pte_val(*ptep) & _PAGE_INVALID) && pte_write(*ptep)) {
+		page = pte_page(*ptep);
+		rc = -EAGAIN;
+		if (trylock_page(page)) {
+			if (should_export_before_import(uvcb, gmap->mm))
+				uv_convert_from_secure(page_to_phys(page));
+			rc = make_page_secure(page, uvcb);
+			unlock_page(page);
+		}
+	}
 	pte_unmap_unlock(ptep, ptelock);
-	unlock_page(page);
 out:
 	mmap_read_unlock(gmap->mm);
 
-- 
2.40.0

Re: [PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure

[Heiko Carstens]

On Wed, Apr 26, 2023 at 03:48:34PM +0200, Claudio Imbrenda wrote:
> This patch fixes a potential race in gmap_make_secure and removes the
> last user of follow_page without FOLL_GET.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
> ---
>  arch/s390/kernel/uv.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)

It would be helpful if this would be a bit more descriptive. "Fix
race" is not very helpful :)

What race does this fix?
When can this happen?
What are the consequences if the race window is being hit?

Re: [PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure

[Claudio Imbrenda]

On Thu, 27 Apr 2023 12:53:04 +0200
Heiko Carstens <hca@linux.ibm.com> wrote:

> On Wed, Apr 26, 2023 at 03:48:34PM +0200, Claudio Imbrenda wrote:
> > This patch fixes a potential race in gmap_make_secure and removes the
> > last user of follow_page without FOLL_GET.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
> > ---
> >  arch/s390/kernel/uv.c | 32 +++++++++++---------------------
> >  1 file changed, 11 insertions(+), 21 deletions(-)  
> 
> It would be helpful if this would be a bit more descriptive. "Fix
> race" is not very helpful :)
> 
> What race does this fix?
> When can this happen?
> What are the consequences if the race window is being hit?

We are locking something we don't have a reference to, and as explained
by Jason and David in this thread <Y9J4P/RNvY1Ztn0Q@nvidia.com> it can
lead to all kind of bad things, including the page getting
unmapped (MADV_DONTNEED), freed, reallocated as a larger folio and the
unlock_page() would target the wrong bit.

Also there is another race with the FOLL_WRITE, which could race
between the follow_page and the get_locked_pte.

The main point of the patch is to remove the last follow_page without
FOLL_GET or FOLL_PIN, removing the races can be considered a nice bonus.

Re: [PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure

[Heiko Carstens]

On Thu, Apr 27, 2023 at 01:46:49PM +0200, Claudio Imbrenda wrote:
> On Thu, 27 Apr 2023 12:53:04 +0200
> Heiko Carstens <hca@linux.ibm.com> wrote:
> 
> > On Wed, Apr 26, 2023 at 03:48:34PM +0200, Claudio Imbrenda wrote:
> > > This patch fixes a potential race in gmap_make_secure and removes the
> > > last user of follow_page without FOLL_GET.
> > > 
> > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
> > > ---
> > >  arch/s390/kernel/uv.c | 32 +++++++++++---------------------
> > >  1 file changed, 11 insertions(+), 21 deletions(-)  
> > 
> > It would be helpful if this would be a bit more descriptive. "Fix
> > race" is not very helpful :)
> > 
> > What race does this fix?
> > When can this happen?
> > What are the consequences if the race window is being hit?
> 
> We are locking something we don't have a reference to, and as explained
> by Jason and David in this thread <Y9J4P/RNvY1Ztn0Q@nvidia.com> it can
> lead to all kind of bad things, including the page getting
> unmapped (MADV_DONTNEED), freed, reallocated as a larger folio and the
> unlock_page() would target the wrong bit.
> 
> Also there is another race with the FOLL_WRITE, which could race
> between the follow_page and the get_locked_pte.
> 
> The main point of the patch is to remove the last follow_page without
> FOLL_GET or FOLL_PIN, removing the races can be considered a nice bonus.

I've seen that discussion. What I'm actually asking for is that all of
this information should be added to the commit description. Nobody
will remember any of the details in one year.

Re: [PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure

[Claudio Imbrenda]

On Thu, 27 Apr 2023 14:01:27 +0200
Heiko Carstens <hca@linux.ibm.com> wrote:

> On Thu, Apr 27, 2023 at 01:46:49PM +0200, Claudio Imbrenda wrote:
> > On Thu, 27 Apr 2023 12:53:04 +0200
> > Heiko Carstens <hca@linux.ibm.com> wrote:
> >   
> > > On Wed, Apr 26, 2023 at 03:48:34PM +0200, Claudio Imbrenda wrote:  
> > > > This patch fixes a potential race in gmap_make_secure and removes the
> > > > last user of follow_page without FOLL_GET.
> > > > 
> > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
> > > > ---
> > > >  arch/s390/kernel/uv.c | 32 +++++++++++---------------------
> > > >  1 file changed, 11 insertions(+), 21 deletions(-)    
> > > 
> > > It would be helpful if this would be a bit more descriptive. "Fix
> > > race" is not very helpful :)
> > > 
> > > What race does this fix?
> > > When can this happen?
> > > What are the consequences if the race window is being hit?  
> > 
> > We are locking something we don't have a reference to, and as explained
> > by Jason and David in this thread <Y9J4P/RNvY1Ztn0Q@nvidia.com> it can
> > lead to all kind of bad things, including the page getting
> > unmapped (MADV_DONTNEED), freed, reallocated as a larger folio and the
> > unlock_page() would target the wrong bit.
> > 
> > Also there is another race with the FOLL_WRITE, which could race
> > between the follow_page and the get_locked_pte.
> > 
> > The main point of the patch is to remove the last follow_page without
> > FOLL_GET or FOLL_PIN, removing the races can be considered a nice bonus.  
> 
> I've seen that discussion. What I'm actually asking for is that all of
> this information should be added to the commit description. Nobody
> will remember any of the details in one year.

I will put it in the patch description.

do you think the text above is enough?

Re: [PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure

[Heiko Carstens]

On Thu, Apr 27, 2023 at 02:17:11PM +0200, Claudio Imbrenda wrote:
> > > > On Wed, Apr 26, 2023 at 03:48:34PM +0200, Claudio Imbrenda wrote:  
> > > > > This patch fixes a potential race in gmap_make_secure and removes the
> > > > > last user of follow_page without FOLL_GET.
> > > > > 
> > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > > Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
> > > > > ---
> > > > >  arch/s390/kernel/uv.c | 32 +++++++++++---------------------
> > > > >  1 file changed, 11 insertions(+), 21 deletions(-)    
> > > > 
> > > > It would be helpful if this would be a bit more descriptive. "Fix
> > > > race" is not very helpful :)
> > > > 
> > > > What race does this fix?
> > > > When can this happen?
> > > > What are the consequences if the race window is being hit?  
> > > 
> > > We are locking something we don't have a reference to, and as explained
> > > by Jason and David in this thread <Y9J4P/RNvY1Ztn0Q@nvidia.com> it can
> > > lead to all kind of bad things, including the page getting
> > > unmapped (MADV_DONTNEED), freed, reallocated as a larger folio and the
> > > unlock_page() would target the wrong bit.
> > > 
> > > Also there is another race with the FOLL_WRITE, which could race
> > > between the follow_page and the get_locked_pte.
> > > 
> > > The main point of the patch is to remove the last follow_page without
> > > FOLL_GET or FOLL_PIN, removing the races can be considered a nice bonus.  
> > 
> > I've seen that discussion. What I'm actually asking for is that all of
> > this information should be added to the commit description. Nobody
> > will remember any of the details in one year.
> 
> I will put it in the patch description.
> 
> do you think the text above is enough?

Fine with me. With a proper Link: tag this is much better than before.
Thanks!