| Message ID | 20230705111937.33472-3-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: pv: fix two small bugs | expand |
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>
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!
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>
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 --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));
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(+)