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: <20211014070208.3cd7e679@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 14 Oct 2021 07:02:08 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     David Ahern <dsahern@...il.com>, davem@...emloft.net,
        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 Thu, 14 Oct 2021 10:10:18 +0200 Daniel Borkmann wrote:
> 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.

My larger point is that the ability to dump policy and inspect it in
user space is an important part of the modern netlink paradigm. When
RTNL is extended appropriately it'll be good if the policies are
expressed the right way.

Fingers-on-the-keyboard-eyes-on-the-screen user friendliness is
important but IMHO code that can be built on top of these interfaces 
is more important.

I think the patch is good as is.