Re: [PATCH] checkpolicy: fix the leak memory when uses xperms

[Petr Lautrbach <plautrba@redhat.com>]

liwugang <liwugang@163.com> writes:

> In the define_te_avtab_ioctl function:
> 1. the parameter avrule_template has been copies to

typo? "copied" instead of "copies" ?

> new elements which added to avrule list through
> the function avrule_cpy, so it should free avrule_template.
> 2. And for rangelist, it does not free the allocated memory.
>
> The memory leak can by found by using memory sanitizer:
> =================================================================
> ==20021==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 10336 byte(s) in 76 object(s) allocated from:
>     #0 0x7f8f96d9cb50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
>     #1 0x55c2e9447fb3 in define_te_avtab_xperms_helper /mnt/sources/tools/selinux/checkpolicy/policy_define.c:2046
>     #2 0x55c2e944a6ca in define_te_avtab_extended_perms /mnt/sources/tools/selinux/checkpolicy/policy_define.c:2479
>     #3 0x55c2e943126b in yyparse /mnt/sources/tools/selinux/checkpolicy/policy_parse.y:494
>     #4 0x55c2e9440221 in read_source_policy /mnt/sources/tools/selinux/checkpolicy/parse_util.c:64
>     #5 0x55c2e945a3df in main /mnt/sources/tools/selinux/checkpolicy/checkpolicy.c:619
>     #6 0x7f8f968eeb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
>
> Direct leak of 240 byte(s) in 15 object(s) allocated from:
>     #0 0x7f8f96d9cb50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
>     #1 0x55c2e9446cd9 in avrule_sort_ioctls /mnt/sources/tools/selinux/checkpolicy/policy_define.c:1846
>     #2 0x55c2e9447d8f in avrule_ioctl_ranges /mnt/sources/tools/selinux/checkpolicy/policy_define.c:2020
>     #3 0x55c2e944a0de in define_te_avtab_ioctl /mnt/sources/tools/selinux/checkpolicy/policy_define.c:2409
>     #4 0x55c2e944a744 in define_te_avtab_extended_perms /mnt/sources/tools/selinux/checkpolicy/policy_define.c:2485
>     #5 0x55c2e94312bf in yyparse /mnt/sources/tools/selinux/checkpolicy/policy_parse.y:503
>     #6 0x55c2e9440221 in read_source_policy /mnt/sources/tools/selinux/checkpolicy/parse_util.c:64
>     #7 0x55c2e945a3df in main /mnt/sources/tools/selinux/checkpolicy/checkpolicy.c:619
>     #8 0x7f8f968eeb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
>
> Signed-off-by: liwugang <liwugang@163.com>
> ---
>  checkpolicy/policy_define.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> index 16234f31..04064af2 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -2400,7 +2400,7 @@ int avrule_cpy(avrule_t *dest, avrule_t *src)
>  int define_te_avtab_ioctl(avrule_t *avrule_template)
>  {
>  	avrule_t *avrule;
> -	struct av_ioctl_range_list *rangelist;
> +	struct av_ioctl_range_list *rangelist, *r, *r2;
>  	av_extended_perms_t *complete_driver, *partial_driver, *xperms;
>  	unsigned int i;
>  
> @@ -2458,6 +2458,13 @@ done:
>  	if (partial_driver)
>  		free(partial_driver);
>  
> +	r = rangelist;
> +	while (r != NULL) {


Seems like you could loop using `rangelist` directly only with `r`
instead of `r2`


> +		r2 = r;
> +		r = r->next;
> +		free(r2);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2484,6 +2491,8 @@ int define_te_avtab_extended_perms(int which)
>  		free(id);
>  		if (define_te_avtab_ioctl(avrule_template))
>  			return -1;
> +		avrule_destroy(avrule_template);
> +		free(avrule_template);

avrule_template should be probably free()'d before `return -1`

>  	} else {
>  		yyerror("only ioctl extended permissions are supported");
>  		free(id);
> -- 
> 2.17.1