linux-kernel - Re: [PATCH] wimax/i2400m: fix a memory leak bug

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190815184505.o7o2ojt7ag4shh7u@oracle.com>
Date:   Thu, 15 Aug 2019 14:45:05 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Wenwen Wang <wenwen@...uga.edu>
Cc:     Inaky Perez-Gonzalez <inaky.perez-gonzalez@...el.com>,
        "supporter:INTEL WIRELESS WIMAX CONNECTION 2400" 
        <linux-wimax@...el.com>, "David S. Miller" <davem@...emloft.net>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] wimax/i2400m: fix a memory leak bug

* Wenwen Wang <wenwen@...uga.edu> [190815 14:05]:
> In i2400m_barker_db_init(), 'options_orig' is allocated through kstrdup()
> to hold the original command line options. Then, the options are parsed.
> However, if an error occurs during the parsing process, 'options_orig' is
> not deallocated, leading to a memory leak bug. To fix this issue, free
> 'options_orig' before returning the error.
> 
> Signed-off-by: Wenwen Wang <wenwen@...uga.edu>
> ---
>  drivers/net/wimax/i2400m/fw.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
> index e9fc168..6b36f6d 100644
> --- a/drivers/net/wimax/i2400m/fw.c
> +++ b/drivers/net/wimax/i2400m/fw.c
> @@ -342,6 +342,7 @@ int i2400m_barker_db_init(const char *_options)
>  				       "a 32-bit number\n",
>  				       __func__, token);
>  				result = -EINVAL;
> +				kfree(options_orig);
>  				goto error_parse;
>  			}
>  			if (barker == 0) {
> @@ -350,8 +351,10 @@ int i2400m_barker_db_init(const char *_options)
>  				continue;
>  			}
>  			result = i2400m_barker_db_add(barker);
> -			if (result < 0)
> +			if (result < 0) {
> +				kfree(options_orig);
>  				goto error_add;

I know that you didn't add this error_add label, but it seems like the
incorrect goto label.  Although looking at the caller indicates an add
failed, this label is used prior to and after the memory leak you are
trying to fix.  It might be better to change this label to something
like error_parse_add and move the kfree to the unwinding.  If a new
label is used, it becomes more clear as to what is being undone and
there aren't two jumps into an unwind from two very different stages of
the function.  Adding a new label also has the benefit of moving the
kfree to the unwind of error_parse.

Thanks,
Liam


> +			}
>  		}
>  		kfree(options_orig);
>  	}
> -- 
> 2.7.4
>