diff mbox series

[v2,2/2] KVM: s390: pv: fix index value of replaced ASCE

Message ID 20230705111937.33472-3-imbrenda@linux.ibm.com
State New
Headers
Series KVM: s390: pv: fix two small bugs |

Commit Message

Claudio Imbrenda July 5, 2023, 11:19 a.m. UTC 
  The index field of the struct page corresponding to a guest ASCE should
be 0. When replacing the ASCE in s390_replace_asce(), the index of the
new ASCE should also be set to 0.

Having the wrong index might lead to the wrong addresses being passed
around when notifying pte invalidations, and eventually to validity
intercepts (VM crash) if the prefix gets unmapped and the notifier gets
called with the wrong address.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/mm/gmap.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Philippe Mathieu-Daudé July 11, 2023, 2:12 p.m. UTC | #1 
On 5/7/23 13:19, Claudio Imbrenda wrote:
> The index field of the struct page corresponding to a guest ASCE should
> be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> new ASCE should also be set to 0.
> 
> Having the wrong index might lead to the wrong addresses being passed
> around when notifying pte invalidations, and eventually to validity
> intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> called with the wrong address.

Can that also happen in crst_table_alloc()?

> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   arch/s390/mm/gmap.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Claudio Imbrenda July 11, 2023, 2:31 p.m. UTC | #2 
On Tue, 11 Jul 2023 16:12:54 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 5/7/23 13:19, Claudio Imbrenda wrote:
> > The index field of the struct page corresponding to a guest ASCE should
> > be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> > new ASCE should also be set to 0.
> > 
> > Having the wrong index might lead to the wrong addresses being passed
> > around when notifying pte invalidations, and eventually to validity
> > intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> > called with the wrong address.  
> 
> Can that also happen in crst_table_alloc()?

no. ->index is only used for gmap page tables (guest mapping), from
the root to pmd tables. the last level (ptes) is shared between guest
mapping and QEMU address space. i.e. the ptes are the ones from QEMU.

the last level of page tables is not expected to have ->index set
(since that one actually belongs to QEMU and not to the guest mapping)

guest page tables (all levels, except ptes) are allocated with
gmap_alloc_table(), which correctly sets ->index

the top level of a guest mapping is allocated in gmap_alloc(), which
also correctly sets ->index to 0.

> 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >   arch/s390/mm/gmap.c | 1 +
> >   1 file changed, 1 insertion(+)  
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

thank you!
Janosch Frank July 12, 2023, 11:58 a.m. UTC | #3 
On 7/5/23 13:19, Claudio Imbrenda wrote:
> The index field of the struct page corresponding to a guest ASCE should
> be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> new ASCE should also be set to 0.
> 
> Having the wrong index might lead to the wrong addresses being passed
> around when notifying pte invalidations, and eventually to validity
> intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> called with the wrong address.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

No fixes tag?

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Claudio Imbrenda July 12, 2023, 12:06 p.m. UTC | #4 
On Wed, 12 Jul 2023 13:58:49 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/5/23 13:19, Claudio Imbrenda wrote:
> > The index field of the struct page corresponding to a guest ASCE should
> > be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> > new ASCE should also be set to 0.
> > 
> > Having the wrong index might lead to the wrong addresses being passed
> > around when notifying pte invalidations, and eventually to validity
> > intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> > called with the wrong address.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>  
> 
> No fixes tag?

oops, you're right

Fixes: faa2f72cb356 ("KVM: s390: pv: leak the topmost page table when
destroy fails")


> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>
diff mbox series

Patch

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index f4b6fc746fce..7c77f246e101 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2853,6 +2853,7 @@  int s390_replace_asce(struct gmap *gmap)
 	page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER);
 	if (!page)
 		return -ENOMEM;
+	page->index = 0;
 	table = page_to_virt(page);
 	memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT));