netdev - Re: [RFC 2] Validate required parameters in inet6_validate_link

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: <08069f49-58b0-4b83-eab8-97326e7e5080@mellanox.com>
Date:   Wed, 15 May 2019 08:14:37 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Leon Romanovsky <leonro@...lanox.com>
Subject: Re: [RFC 2] Validate required parameters in inet6_validate_link_af

On 2019-05-15 02:32, Jakub Kicinski wrote:
> On Mon, 13 May 2019 15:05:30 +0000, Maxim Mikityanskiy wrote:
>> +	err = -EINVAL;
>> +
>> +	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
>> +		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
>> +
>> +		if (check_addr_gen_mode(mode) < 0)
>> +			return -EINVAL;
>> +		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
>> +			return -EINVAL;
>> +
>> +		err = 0;
>> +	}
>> +
>> +	if (tb[IFLA_INET6_TOKEN])
>> +		err = 0;
>> +
>> +	return err;
> 
> While at it could you forgo the retval optimization?  Most of the time
> it just leads to less readable code for no gain.

OK, I'll make this change in a respin.

> The normal way to write this code would be:
> 
> 	if (!tb[IFLA_INET6_ADDR_GEN_MODE] && !tb[IFLA_INET6_TOKEN])
> 		return -EINVAL;

Yeah, that's how I wrote this check in RFC 1, but here in this patch I 
decided to preserve the pattern that was used in inet6_set_link_af 
before my change, to minimize the changes. I agree it's less readable (I 
didn't like the error handling flow in inet6_set_link_af either), so 
I'll fix it. Thanks for reviewing!

> 	if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
> 		u8 mode = nla_get_u8(tb[IFLA_INET6_ADDR_GEN_MODE]);
> 
> 		if (check_addr_gen_mode(mode) < 0)
> 			return -EINVAL;
> 		if (dev && check_stable_privacy(idev, dev_net(dev), mode) < 0)
> 			return -EINVAL;
> 	}
> 
> 	return 0;
>