netdev - Re: [PATCH net v3] gen_stats.c: Duplicate xstats buffer for later use

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: <CAHA+R7OpbP_+iwh6X=sh4SsXJ4VreBjze7dPrV6Xh8RHEe+cGw@mail.gmail.com>
Date:	Fri, 9 Jan 2015 14:35:41 -0800
From:	Cong Wang <cwang@...pensource.com>
To:	Ignacy Gawędzki 
	<ignacy.gawedzki@...en-communications.fr>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net v3] gen_stats.c: Duplicate xstats buffer for later use

On Thu, Jan 8, 2015 at 5:30 AM, Ignacy Gawędzki
<ignacy.gawedzki@...en-communications.fr> wrote:
> The gnet_stats_copy_app() function gets called, more often than not, with its
> second argument a pointer to an automatic variable in the caller's stack.
> Therefore, to avoid copying garbage afterwards when calling
> gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
> memory that gets freed after use.

Good catch!

>
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@...en-communications.fr>
> ---
>  net/core/gen_stats.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..c9f1fa8 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -32,6 +32,9 @@ gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
>         return 0;
>
>  nla_put_failure:
> +       kfree(d->xstats);
> +       d->xstats = NULL;
> +       d->xstats_len = 0;
>         spin_unlock_bh(d->lock);
>         return -1;
>  }

Although it is not your fault, the API sucks, gnet_stats_copy()
should not need to care about the error path of its caller.
I will cook a patch for it.

> @@ -305,7 +308,10 @@ int
>  gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
>  {
>         if (d->compat_xstats) {
> -               d->xstats = st;
> +               kfree(d->xstats);
> +               d->xstats = kmemdup(st, len, GFP_ATOMIC);
> +               if (!d->xstats)
> +                       goto kmalloc_failure;

Do we really need to call kfree() before kmemdup()?
I don't think gnet_stats_copy_app() is supposed to be called
twice before gnet_stats_copy_finish()?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html