netdev - Re: [PATCHv4 iproute2 2/2] lib/libnetlink: update rtnl

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: <20170929130655.qokb6b55cozj5oor@unicorn.suse.cz>
Date:   Fri, 29 Sep 2017 15:06:55 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Hangbin Liu <haliu@...hat.com>
Cc:     netdev@...r.kernel.org,
        Stephen Hemminger <stephen@...workplumber.org>,
        Phil Sutter <phil@....cc>, Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCHv4 iproute2 2/2] lib/libnetlink: update rtnl_talk to
 support malloc buff at run time

On Thu, Sep 28, 2017 at 09:33:46PM +0800, Hangbin Liu wrote:
> From: Hangbin Liu <liuhangbin@...il.com>
> 
> This is an update for 460c03f3f3cc ("iplink: double the buffer size also in
> iplink_get()"). After update, we will not need to double the buffer size
> every time when VFs number increased.
> 
> With call like rtnl_talk(&rth, &req.n, NULL, 0), we can simply remove the
> length parameter.
> 
> With call like rtnl_talk(&rth, nlh, nlh, sizeof(req), I add a new variable
> answer to avoid overwrite data in nlh, because it may has more info after
> nlh. also this will avoid nlh buffer not enough issue.
> 
> We need to free answer after using.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> Signed-off-by: Phil Sutter <phil@....cc>
> ---

Reviewed-by: Michal Kubecek <mkubecek@...e.cz>

> diff --git a/ip/link_gre.c b/ip/link_gre.c
> index 9ea2970..35782ca 100644
> --- a/ip/link_gre.c
> +++ b/ip/link_gre.c
> @@ -68,7 +68,6 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
>  	struct {
>  		struct nlmsghdr n;
>  		struct ifinfomsg i;
> -		char buf[16384];
>  	} req = {
>  		.n.nlmsg_len = NLMSG_LENGTH(sizeof(*ifi)),
>  		.n.nlmsg_flags = NLM_F_REQUEST,
> @@ -76,6 +75,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
>  		.i.ifi_family = preferred_family,
>  		.i.ifi_index = ifi->ifi_index,
>  	};
> +	struct nlmsghdr *answer = NULL;
>  	struct rtattr *tb[IFLA_MAX + 1];
>  	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
>  	struct rtattr *greinfo[IFLA_GRE_MAX + 1];
> @@ -100,19 +100,20 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
>  	__u32 erspan_idx = 0;
>  
>  	if (!(n->nlmsg_flags & NLM_F_CREATE)) {
> -		if (rtnl_talk(&rth, &req.n, &req.n, sizeof(req)) < 0) {
> +		if (rtnl_talk(&rth, &req.n, &answer) < 0) {
>  get_failed:
>  			fprintf(stderr,
>  				"Failed to get existing tunnel info.\n");
> +			free(answer);
>  			return -1;
>  		}

Took me a moment to realize answer is still NULL if we get here via
failed rtnl_talk() but non-NULL if we get here via "goto get_failed"
later. Nice trick. :-)

Michal Kubecek