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.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:
>
> 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 <jgr...@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
