netdev - Re: [PATCH net-next] rtnetlink: add RTNH_REJECT

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:   Sun, 28 Nov 2021 17:19:38 -0700
From:   David Ahern <dsahern@...il.com>
To:     Ido Schimmel <idosch@...sch.org>,
        Alexander Mikhalitsyn <alexander.mikhalitsyn@...tuozzo.com>
Cc:     netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Ido Schimmel <idosch@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Roopa Prabhu <roopa@...dia.com>,
        Andrei Vagin <avagin@...il.com>,
        Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
        Alexander Mikhalitsyn <alexander@...alicyn.com>
Subject: Re: [PATCH net-next] rtnetlink: add RTNH_REJECT_MASK

On 11/28/21 7:01 AM, Ido Schimmel wrote:
> On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index 5888492a5257..9c065e2fdef9 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -417,6 +417,9 @@ struct rtnexthop {
>>  #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
>>  				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
>>  
>> +/* these flags can't be set by the userspace */
>> +#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
>> +
>>  /* Macros to handle hexthops */
>>  
>>  #define RTNH_ALIGNTO	4
>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>> index 4c0c33e4710d..805f5e05b56d 100644
>> --- a/net/ipv4/fib_semantics.c
>> +++ b/net/ipv4/fib_semantics.c
>> @@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
>>  			return -EINVAL;
>>  		}
>>  
>> -		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
>> +		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
>>  			NL_SET_ERR_MSG(extack,
>>  				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
>>  			return -EINVAL;
>> @@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
>>  		goto err_inval;
>>  	}
>>  
>> -	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
>> +	if (cfg->fc_flags & RTNH_REJECT_MASK) {
>>  		NL_SET_ERR_MSG(extack,
>>  			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");
> 
> Instead of a deny list as in the legacy nexthop code, the new nexthop
> code has an allow list (from rtm_to_nh_config()):
> 
> ```
> 	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
> 		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
> 		goto out;
> 	}
> ```
> 
> Where:
> 
> ```
> #define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK
> ```
> 
> So while the legacy nexthop code allows setting flags such as
> RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
> case for setting these flags from user space so I don't care if we allow
> or deny them, but I believe the legacy and new nexthop code should be
> consistent.
> 
> WDYT? Should we allow these flags in the new nexthop code as well or
> keep denying them?
> 
>>  		goto err_inval;

I like the positive naming - RTNH_VALID_USER_FLAGS.

nexthop API should allow the OFFLOAD flag to be consistent; separate
change though.