netdev - Re: [PATCH net-next 2/3] net, neigh: Use NLA_POLICY_MASK helper for NDA_FLAGS

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: <687e2be1-4d16-1f71-bb25-1f27a04d06f0@iogearbox.net>
Date:   Thu, 14 Oct 2021 10:10:18 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     David Ahern <dsahern@...il.com>, davem@...emloft.net,
        kuba@...nel.org
Cc:     roopa@...dia.com, dsahern@...nel.org, m@...bda.lt,
        john.fastabend@...il.com, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH net-next 2/3] net, neigh: Use NLA_POLICY_MASK helper for
 NDA_FLAGS_EXT attribute

On 10/14/21 5:13 AM, David Ahern wrote:
> On 10/13/21 7:21 AM, Daniel Borkmann wrote:
>> Instead of open-coding a check for invalid bits in NTF_EXT_MASK, we can just
>> use the NLA_POLICY_MASK() helper instead, and simplify NDA_FLAGS_EXT sanity
>> check this way.
>>
>> Suggested-by: Jakub Kicinski <kuba@...nel.org>
>> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
>> ---
>>   net/core/neighbour.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> index 4fc601f9cd06..922b9ed0fe76 100644
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -1834,7 +1834,7 @@ const struct nla_policy nda_policy[NDA_MAX+1] = {
>>   	[NDA_MASTER]		= { .type = NLA_U32 },
>>   	[NDA_PROTOCOL]		= { .type = NLA_U8 },
>>   	[NDA_NH_ID]		= { .type = NLA_U32 },
>> -	[NDA_FLAGS_EXT]		= { .type = NLA_U32 },
>> +	[NDA_FLAGS_EXT]		= NLA_POLICY_MASK(NLA_U32, NTF_EXT_MASK),
>>   	[NDA_FDB_EXT_ATTRS]	= { .type = NLA_NESTED },
>>   };
>>   
>> @@ -1936,10 +1936,6 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>>   	if (tb[NDA_FLAGS_EXT]) {
>>   		u32 ext = nla_get_u32(tb[NDA_FLAGS_EXT]);
>>   
>> -		if (ext & ~NTF_EXT_MASK) {
>> -			NL_SET_ERR_MSG(extack, "Invalid extended flags");
>> -			goto out;
>> -		}
>>   		BUILD_BUG_ON(sizeof(neigh->flags) * BITS_PER_BYTE <
>>   			     (sizeof(ndm->ndm_flags) * BITS_PER_BYTE +
>>   			      hweight32(NTF_EXT_MASK)));
>>
> 
> I get that NLA_POLICY_MASK wants to standardize the logic, but the
> generic extack message "reserved bit set" is less useful than the one here.

If the expectation/recommendation is that NLA_POLICY_MASK() should be used, then
it would probably make sense for NLA_POLICY_MASK() itself to improve. For example,
NLA_POLICY_MASK() could perhaps take an optional error string which it should
return via extack rather than the standard "reserved bit set" one or such.. on
the other hand, I see that NL_SET_ERR_MSG_ATTR() already points out the affected
attribute via setting extack->bad_attr, so it be sufficient to figure out that it's
about reserved bits inside NDA_FLAGS_EXT given this is propagated back to user
space via NLMSGERR_ATTR_OFFS.

Thanks,
Daniel