[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
On 27/02/2023 2:42 pm, Juergen Gross wrote: > On 24.02.23 15:56, Andrew Cooper wrote: >> On 24/02/2023 1:36 pm, Edwin Török wrote: >>> From: Edwin Török <edwin.torok@xxxxxxxxx> >>> >>> Prior to bd7a29c3d0 'out' would've always been executed and memory >>> freed, but that commit changed it such that it returns early and leaks. >>> >>> Found using gcc 12.2.1 `-fanalyzer`: >>> ``` >>> xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’: >>> xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401] >>> [-Werror=analyzer-malloc-leak] >>> 300 | return p2m_frame_list; >>> | ^~~~~~ >>> ‘xc_core_arch_map_p2m_writable’: events 1-2 >>> | >>> | 378 | xc_core_arch_map_p2m_writable(xc_interface *xch, >>> struct domain_info_context *dinfo, xc_dominfo_t *info, >>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> | | | >>> | | (1) entry to ‘xc_core_arch_map_p2m_writable’ >>> |...... >>> | 381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info, >>> live_shinfo, live_p2m, 1); >>> | | >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> | | | >>> | | (2) calling ‘xc_core_arch_map_p2m_rw’ from >>> ‘xc_core_arch_map_p2m_writable’ >>> | >>> +--> ‘xc_core_arch_map_p2m_rw’: events 3-10 >>> | >>> | 319 | xc_core_arch_map_p2m_rw(xc_interface *xch, >>> struct domain_info_context *dinfo, xc_dominfo_t *info, >>> | | ^~~~~~~~~~~~~~~~~~~~~~~ >>> | | | >>> | | (3) entry to ‘xc_core_arch_map_p2m_rw’ >>> |...... >>> | 328 | if ( xc_domain_nr_gpfns(xch, info->domid, >>> &dinfo->p2m_size) < 0 ) >>> | | ~ >>> | | | >>> | | (4) following ‘false’ branch... >>> |...... >>> | 334 | if ( dinfo->p2m_size < info->nr_pages ) >>> | | ~~ ~ >>> | | | | >>> | | | (6) following ‘false’ branch... >>> | | (5) ...to here >>> |...... >>> | 340 | p2m_cr3 = GET_FIELD(live_shinfo, >>> arch.p2m_cr3, dinfo->guest_width); >>> | | ~~~~~~~ >>> | | | >>> | | (7) ...to here >>> | 341 | >>> | 342 | p2m_frame_list = p2m_cr3 ? >>> xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3) >>> | | >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> | 343 | : >>> xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo); >>> | | >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> | | | | >>> | | | (9) ...to here >>> | | | (10) calling >>> ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’ >>> | | (8) following >>> ‘false’ branch... >>> | >>> +--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24 >>> | >>> | 228 | >>> xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct >>> domain_info_context *dinfo, >>> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> | | | >>> | | (11) entry to >>> ‘xc_core_arch_map_p2m_tree_rw’ >>> |...... >>> | 245 | if ( !live_p2m_frame_list_list ) >>> | | ~ >>> | | | >>> | | (12) following ‘false’ branch >>> (when ‘live_p2m_frame_list_list’ is non-NULL)... >>> |...... >>> | 252 | if ( !(p2m_frame_list_list = >>> malloc(PAGE_SIZE)) ) >>> | | ~~ ~ >>> ~~~~~~~~~~~~~~~~~ >>> | | | | | >>> | | | | (14) >>> allocated here >>> | | | (15) assuming >>> ‘p2m_frame_list_list’ is non-NULL >>> | | | (16) following ‘false’ branch >>> (when ‘p2m_frame_list_list’ is non-NULL)... >>> | | (13) ...to here >>> |...... >>> | 257 | memcpy(p2m_frame_list_list, >>> live_p2m_frame_list_list, PAGE_SIZE); >>> | | ~~~~~~ >>> | | | >>> | | (17) ...to here >>> |...... >>> | 266 | else if ( dinfo->guest_width < >>> sizeof(unsigned long) ) >>> | | ~ >>> | | | >>> | | (18) following ‘false’ >>> branch... >>> |...... >>> | 270 | live_p2m_frame_list = >>> | | ~~~~~~~~~~~~~~~~~~~ >>> | | | >>> | | (19) ...to here >>> |...... >>> | 275 | if ( !live_p2m_frame_list ) >>> | | ~ >>> | | | >>> | | (20) following ‘false’ branch >>> (when ‘live_p2m_frame_list’ is non-NULL)... >>> |...... >>> | 282 | if ( !(p2m_frame_list = >>> malloc(P2M_TOOLS_FL_SIZE)) ) >>> | | ~~ ~ >>> | | | | >>> | | | (22) following ‘false’ branch >>> (when ‘p2m_frame_list’ is non-NULL)... >>> | | (21) ...to here >>> |...... >>> | 287 | memset(p2m_frame_list, 0, >>> P2M_TOOLS_FL_SIZE); >>> | | ~~~~~~ >>> | | | >>> | | (23) ...to here >>> |...... >>> | 300 | return p2m_frame_list; >>> | | ~~~~~~ >>> | | | >>> | | (24) ‘p2m_frame_list_list’ leaks >>> here; was allocated at (14) >>> | >>> ``` >>> Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to >>> support linear p2m table") >>> >>> Signed-off-by: Edwin Török <edwin.torok@xxxxxxxxx> >>> --- >>> tools/libs/guest/xg_core_x86.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/tools/libs/guest/xg_core_x86.c >>> b/tools/libs/guest/xg_core_x86.c >>> index 61106b98b8..69929879d7 100644 >>> --- a/tools/libs/guest/xg_core_x86.c >>> +++ b/tools/libs/guest/xg_core_x86.c >>> @@ -297,6 +297,8 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, >>> struct domain_info_context *dinf >>> dinfo->p2m_frames = P2M_FL_ENTRIES; >>> + free(p2m_frame_list_list); >>> + >>> return p2m_frame_list; >>> out: >> >> I agree there are problems here, but I think they're larger still. The >> live_p2m_frame_list_list and live_p2m_frame_list foreign mappings are >> leaked too on the success path. >> >> I think this is the necessary fix: > > Yes, I agree. > >> >> ~Andrew >> >> ----8<---- >> >> diff --git a/tools/libs/guest/xg_core_x86.c >> b/tools/libs/guest/xg_core_x86.c >> index 61106b98b877..c5e4542ccccc 100644 >> --- a/tools/libs/guest/xg_core_x86.c >> +++ b/tools/libs/guest/xg_core_x86.c >> @@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, >> struct domain_info_context *dinf >> uint32_t dom, shared_info_any_t >> *live_shinfo) >> { >> /* Double and single indirect references to the live P2M table */ >> - xen_pfn_t *live_p2m_frame_list_list; >> + xen_pfn_t *live_p2m_frame_list_list = NULL; >> xen_pfn_t *live_p2m_frame_list = NULL; >> /* Copies of the above. */ >> xen_pfn_t *p2m_frame_list_list = NULL; >> - xen_pfn_t *p2m_frame_list; >> + xen_pfn_t *p2m_frame_list = NULL; >> int err; >> int i; >> @@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, >> struct domain_info_context *dinf >> dinfo->p2m_frames = P2M_FL_ENTRIES; >> - return p2m_frame_list; >> - >> out: >> err = errno; >> @@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, >> struct domain_info_context *dinf >> errno = err; >> - return NULL; >> + return p2m_frame_list; >> } >> static int >> > > In case this fix is taken, my > > Reviewed-by: Juergen Gross <jgross@xxxxxxxx> > > can be added. Thanks. I'll write a full patch and post it, with appropriate tags, and also include it in my commit sweep. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |