| Message ID | c721e627bc74047c0c5e55c0aeae7d72bbc3ce0e.1677245356.git.edwin.torok@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix memory leaks reported by GCC -fanalyzer | expand |
On 24/02/2023 1:36 pm, Edwin Török wrote: > From: Edwin Török <edwin.torok@cloud.com> > > 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@cloud.com> > --- > 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
> On 24 Feb 2023, at 14:56, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 24/02/2023 1:36 pm, Edwin Török wrote: >> From: Edwin Török <edwin.torok@cloud.com> >> >> 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@cloud.com> >> --- >> 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 thought the goal of that function was to create the mapping (judging by its name), but looking at its caller there is another map_foreign_pages there, so there is indeed a leak (-fanalyzer won't be able to spot these unless we figure out a way to put some attributs on these map and unmap to teach it that they are alloc/free pairs). Pushed updated commits here (top 2): leak-fixes <https://github.com/edwintorok/xen/commits/leak-fixes> Before posting a V2 I'll try it out on an actual machine though, just to check that we don't have a double-free instead now. Thanks, --Edwin > > 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 >
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@cloud.com> >> >> 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@cloud.com> >> --- >> 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@suse.com> can be added. Juergen
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@cloud.com> >>> >>> 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@cloud.com> >>> --- >>> 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@suse.com> > > 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
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: