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:
--
2.39.1
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.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
> 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
>
Edwin, with the help of GCC's -fanalyzer, identified that p2m_frame_list_list
gets leaked. What fanalyzer can't see is that the live_p2m_frame_list_list
and live_p2m_frame_list foreign mappings are leaked too.
Rework the logic so the out path is executed unconditionally, which cleans up
all the intermediate allocations/mappings appropriately.
Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
Reported-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
tools/libs/guest/xg_core_x86.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
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
--
2.30.2
© 2016 - 2026 Red Hat, Inc.