From: Jan Beulich Subject: gnttab/ARM: don't corrupt shared GFN array ... by writing status GFNs to it. Introduce a second array instead. Also implement gnttab_status_gmfn() properly now that the information is suitably being tracked. While touching it anyway, remove a misguided (but luckily benign) upper bound check from gnttab_shared_gmfn(): We should never access beyond the bounds of that array. This is part of XSA-255. Signed-off-by: Jan Beulich Reviewed-by: Stefano Stabellini Reviewed-by: Andrew Cooper --- v3: Don't init the ARM GFN arrays to zero anymore, use INVALID_GFN. v2: New. --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -3775,6 +3775,7 @@ int gnttab_map_frame(struct domain *d, u { int rc = 0; struct grant_table *gt = d->grant_table; + bool status = false; grant_write_lock(gt); @@ -3785,6 +3786,7 @@ int gnttab_map_frame(struct domain *d, u (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; + status = true; if ( idx < nr_status_frames(gt) ) *mfn = _mfn(virt_to_mfn(gt->status[idx])); else @@ -3802,7 +3804,7 @@ int gnttab_map_frame(struct domain *d, u } if ( !rc ) - gnttab_set_frame_gfn(gt, idx, gfn); + gnttab_set_frame_gfn(gt, status, idx, gfn); grant_write_unlock(gt); --- a/xen/include/asm-arm/grant_table.h +++ b/xen/include/asm-arm/grant_table.h @@ -9,7 +9,8 @@ #define INITIAL_NR_GRANT_FRAMES 1U struct grant_table_arch { - gfn_t *gfn; + gfn_t *shared_gfn; + gfn_t *status_gfn; }; void gnttab_clear_flag(unsigned long nr, uint16_t *addr); @@ -21,7 +22,6 @@ int replace_grant_host_mapping(unsigned unsigned long new_gpaddr, unsigned int flags); void gnttab_mark_dirty(struct domain *d, unsigned long l); #define gnttab_create_status_page(d, t, i) do {} while (0) -#define gnttab_status_gmfn(d, t, i) (0) #define gnttab_release_host_mappings(domain) 1 static inline int replace_grant_supported(void) { @@ -42,19 +42,35 @@ static inline unsigned int gnttab_dom0_m #define gnttab_init_arch(gt) \ ({ \ - (gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames); \ - ( (gt)->arch.gfn ? 0 : -ENOMEM ); \ + unsigned int ngf_ = (gt)->max_grant_frames; \ + unsigned int nsf_ = grant_to_status_frames(ngf_); \ + \ + (gt)->arch.shared_gfn = xmalloc_array(gfn_t, ngf_); \ + (gt)->arch.status_gfn = xmalloc_array(gfn_t, nsf_); \ + if ( (gt)->arch.shared_gfn && (gt)->arch.status_gfn ) \ + { \ + while ( ngf_-- ) \ + (gt)->arch.shared_gfn[ngf_] = INVALID_GFN; \ + while ( nsf_-- ) \ + (gt)->arch.status_gfn[nsf_] = INVALID_GFN; \ + } \ + else \ + gnttab_destroy_arch(gt); \ + (gt)->arch.shared_gfn ? 0 : -ENOMEM; \ }) #define gnttab_destroy_arch(gt) \ do { \ - xfree((gt)->arch.gfn); \ - (gt)->arch.gfn = NULL; \ + xfree((gt)->arch.shared_gfn); \ + (gt)->arch.shared_gfn = NULL; \ + xfree((gt)->arch.status_gfn); \ + (gt)->arch.status_gfn = NULL; \ } while ( 0 ) -#define gnttab_set_frame_gfn(gt, idx, gfn) \ +#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ do { \ - (gt)->arch.gfn[idx] = gfn; \ + ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ + (gfn); \ } while ( 0 ) #define gnttab_create_shared_page(d, t, i) \ @@ -65,8 +81,10 @@ static inline unsigned int gnttab_dom0_m } while ( 0 ) #define gnttab_shared_gmfn(d, t, i) \ - ( ((i >= nr_grant_frames(t)) && \ - (i < (t)->max_grant_frames))? 0 : gfn_x((t)->arch.gfn[i])) + gfn_x(((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) + +#define gnttab_status_gmfn(d, t, i) \ + gfn_x(((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) #define gnttab_need_iommu_mapping(d) \ (is_domain_direct_mapped(d) && need_iommu(d)) --- a/xen/include/asm-x86/grant_table.h +++ b/xen/include/asm-x86/grant_table.h @@ -46,7 +46,7 @@ static inline unsigned int gnttab_dom0_m #define gnttab_init_arch(gt) 0 #define gnttab_destroy_arch(gt) do {} while ( 0 ) -#define gnttab_set_frame_gfn(gt, idx, gfn) do {} while ( 0 ) +#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 ) #define gnttab_create_shared_page(d, t, i) \ do { \