> On 24 Feb 2023, at 14:56, Andrew Cooper <andrew.coop...@citrix.com> wrote:
>
> 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 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
>
