[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak
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@xxxxxxxxx>
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@xxxxxxxxx>
---
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@xxxxxxxx>
can be added.
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
