[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 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: ~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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |