netdev - Re: [PATCH ethtool v2 12/13] ethtool: fix leak of memory after realloc

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]

Date:   Thu, 8 Dec 2022 12:30:03 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v2 12/13] ethtool: fix leak of memory after
 realloc

On Wed, Dec 07, 2022 at 05:11:21PM -0800, Jesse Brandeburg wrote:
> cppcheck finds:
> netlink/msgbuff.c:63:2: error: Memory leak: nbuff [memleak]
>  return 0;
>  ^

This reported error is misleading, the commit message should make it
clear that the leak it addresses is not of nbuff but of the original
msgbuff->buff (on failure).

> This is a pretty common problem with realloc() and just requires handling
> the return code correctly which makes us refactor to reuse the structure
> free/reinit code that already exists in msgbuf_done().
> 
> This fixes the code flow by doing the right thing if realloc() succeeds and
> if it fails then being sure to free the original memory and replicate the
> steps the original code took.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
>  netlink/msgbuff.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/netlink/msgbuff.c b/netlink/msgbuff.c
> index 216f5b946236..a599cab06014 100644
> --- a/netlink/msgbuff.c
> +++ b/netlink/msgbuff.c
[...]
> @@ -43,19 +57,16 @@ int msgbuff_realloc(struct nl_msg_buff *msgbuff, unsigned int new_size)
>  	if (new_size > MAX_MSG_SIZE)
>  		return -EMSGSIZE;
>  	nbuff = realloc(msgbuff->buff, new_size);
> -	if (!nbuff) {
> -		msgbuff->buff = NULL;
> -		msgbuff->size = 0;
> -		msgbuff->left = 0;
> -		return -ENOMEM;
> -	}
> -	if (nbuff != msgbuff->buff) {
> +	if (nbuff) {
>  		if (new_size > old_size)
>  			memset(nbuff + old_size, '\0', new_size - old_size);
>  		msgbuff->nlhdr = (struct nlmsghdr *)(nbuff + nlhdr_off);
>  		msgbuff->genlhdr = (struct genlmsghdr *)(nbuff + genlhdr_off);
>  		msgbuff->payload = nbuff + payload_off;
>  		msgbuff->buff = nbuff;
> +	} else {
> +		msgbuff_done(msgbuff);
> +		return -ENOMEM;
>  	}
>  	msgbuff->size = new_size;
>  	msgbuff->left += (new_size - old_size);

If nbuff is null (reallocation failed), we bail out anyway so there is
no point putting part of the followin code inside an if and leaving the
rest outside. Also, while it makes sense to zero the new part of the
buffer even if the buffer is not moved, there is no reason to
reinitialize all the pointers. How about this:

	if (!nbuff) {
		msgbuff_done(msgbuff);
		return -ENOMEM;
	}
	if (new_size > old_size)
		memset(nbuff + old_size, '\0', new_size - old_size);
	if (nbuff != msgbuff->buff) {
		msgbuff->nlhdr = (struct nlmsghdr *)(nbuff + nlhdr_off);
		msgbuff->genlhdr = (struct genlmsghdr *)(nbuff + genlhdr_off);
		msgbuff->payload = nbuff + payload_off;
		msgbuff->buff = nbuff;
	}
	msgbuff->size = new_size;
	msgbuff->left += (new_size - old_size);

Or we could forget about the "if (nbuff != msgbuff->buff)" test and
assign the four pointers to values they already have in case the block
stays in place. The code would look a bit tidier and the difference
would be negligible.

And looking at the code again, the "new_size > old_size" check can be
omitted too as we already have

	if (new_size <= old_size)
		return 0;

before the realloc() call. This is a relic from a development version
where realloc() was also used to shrink the buffer if new requested size
was smaller than current.


Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)