On 24/02/2023 1:36 pm, Edwin Török wrote:
> From: Edwin Török <edwin.to...@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.to...@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
