netdev - Re: [PATCH] tc: Fix descriptor leak in get_qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240208090434.2291c0af@hermes.local>
Date: Thu, 8 Feb 2024 09:04:34 -0800
From: Stephen Hemminger <stephen@...workplumber.org>
To: Maks Mishin <maks.mishinfz@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] tc: Fix descriptor leak in get_qdisc_kind()

On Thu,  8 Feb 2024 00:00:06 +0300
Maks Mishin <maks.mishinfz@...il.com> wrote:

> Add closure of fd `dlh` and fix incorrect comparison of `dlh` with NULL.
> 
> Found by RASU JSC
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@...il.com>
> ---
>  tc/tc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tc/tc.c b/tc/tc.c
> index 575157a8..162700b2 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -112,7 +112,7 @@ struct qdisc_util *get_qdisc_kind(const char *str)
>  
>  	snprintf(buf, sizeof(buf), "%s/q_%s.so", get_tc_lib(), str);
>  	dlh = dlopen(buf, RTLD_LAZY);
> -	if (!dlh) {
> +	if (dlh != NULL) {
>  		/* look in current binary, only open once */
>  		dlh = BODY;
>  		if (dlh == NULL) {
> @@ -124,6 +124,9 @@ struct qdisc_util *get_qdisc_kind(const char *str)
>  
>  	snprintf(buf, sizeof(buf), "%s_qdisc_util", str);
>  	q = dlsym(dlh, buf);
> +	if (dlh != NULL)
> +		dlclose(dlh);
> +
>  	if (q == NULL)
>  		goto noexist;
>  

This does make sense, but the patch method is messy.
Please don't modify code outside scope of the fix being addressed.
And since the check for dlh being NULL is already done above,
the conditional there is irrelevant.

Also, this change will break the code. If you read carefully
in the comments, there is a hint.  The idea is that if the
qdisc is not in the external file, then it does an internal
dlopen() and it wants to keep it open in the variable BODY.

Your change would close dlh which since it same as BODY would
close the internal handle.  That would cause next call to get_qdisc_kind()
to look at an invalid handle (BODY).

I appreciate your efforts, but you need to read deeper.
This is 30 year old code and the leak may not be important
or be intentional.