From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-10-13 13:21:53
Minor follow-up series to address prior feedback from David and Jakub.
Patch 1 adds a build time assertion to prevent overflows when shifting
in extended flags, patch 2 is a cleanup to use NLA_POLICY_MASK instead
of open-coding invalid flags rejection and patch 3 rejects creating new
neighbors with NUD_PERMANENT & NTF_MANAGED. For details, see individual
patches. Will push out iproute2 series after that. Thanks!
Daniel Borkmann (3):
net, neigh: Add build-time assertion to avoid neigh->flags overflow
net, neigh: Use NLA_POLICY_MASK helper for NDA_FLAGS_EXT attribute
net, neigh: Reject creating NUD_PERMANENT with NTF_MANAGED entries
net/core/neighbour.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
--
2.27.0
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-10-13 13:21:53
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@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-10-13 13:21:53
Currently, NDA_FLAGS_EXT flags allow a maximum of 24 bits to be used for
extended neighbor flags. These are eventually fed into neigh->flags by
shifting with NTF_EXT_SHIFT as per commit 2c611ad97a82 ("net, neigh:
Extend neigh->flags to 32 bit to allow for extensions").
If really ever needed in future, the full 32 bits from NDA_FLAGS_EXT can
be used, it would only require to move neigh->flags from u32 to u64 inside
the kernel.
Add a build-time assertion such that when extending the NTF_EXT_MASK with
new bits, we'll trigger an error once we surpass the 24th bit. This assumes
that no bit holes in new NTF_EXT_* flags will slip in from UAPI, but I
think this is reasonable to assume.
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 3 +++
1 file changed, 3 insertions(+)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-10-13 13:21:56
The combination of NUD_PERMANENT + NTF_MANAGED is not supported and does
not make sense either given the former indicates a static/fixed neighbor
entry whereas the latter a dynamically resolved one. While it is possible
to transition from one over to the other, we should however reject such
creation attempts.
Fixes: 7482e3841d52 ("net, neigh: Add NTF_MANAGED flag for managed neighbor entries")
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
From: David Ahern <hidden> Date: 2021-10-14 03:10:54
On 10/13/21 7:21 AM, Daniel Borkmann wrote:
quoted hunk
Currently, NDA_FLAGS_EXT flags allow a maximum of 24 bits to be used for
extended neighbor flags. These are eventually fed into neigh->flags by
shifting with NTF_EXT_SHIFT as per commit 2c611ad97a82 ("net, neigh:
Extend neigh->flags to 32 bit to allow for extensions").
If really ever needed in future, the full 32 bits from NDA_FLAGS_EXT can
be used, it would only require to move neigh->flags from u32 to u64 inside
the kernel.
Add a build-time assertion such that when extending the NTF_EXT_MASK with
new bits, we'll trigger an error once we surpass the 24th bit. This assumes
that no bit holes in new NTF_EXT_* flags will slip in from UAPI, but I
think this is reasonable to assume.
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 3 +++
1 file changed, 3 insertions(+)
From: David Ahern <hidden> Date: 2021-10-14 03:11:17
On 10/13/21 7:21 AM, Daniel Borkmann wrote:
The combination of NUD_PERMANENT + NTF_MANAGED is not supported and does
not make sense either given the former indicates a static/fixed neighbor
entry whereas the latter a dynamically resolved one. While it is possible
to transition from one over to the other, we should however reject such
creation attempts.
Fixes: 7482e3841d52 ("net, neigh: Add NTF_MANAGED flag for managed neighbor entries")
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
From: David Ahern <hidden> Date: 2021-10-14 03:13:55
On 10/13/21 7:21 AM, Daniel Borkmann wrote:
quoted hunk
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@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2021-10-14 08:10:29
On 10/14/21 5:13 AM, David Ahern wrote:
On 10/13/21 7:21 AM, Daniel Borkmann wrote:
quoted
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@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-14 14:02:11
On Thu, 14 Oct 2021 10:10:18 +0200 Daniel Borkmann wrote:
On 10/14/21 5:13 AM, David Ahern wrote:
quoted
On 10/13/21 7:21 AM, Daniel Borkmann wrote:
quoted
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@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/neighbour.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
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.
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski [off-list ref]:
On Wed, 13 Oct 2021 15:21:37 +0200 you wrote:
Minor follow-up series to address prior feedback from David and Jakub.
Patch 1 adds a build time assertion to prevent overflows when shifting
in extended flags, patch 2 is a cleanup to use NLA_POLICY_MASK instead
of open-coding invalid flags rejection and patch 3 rejects creating new
neighbors with NUD_PERMANENT & NTF_MANAGED. For details, see individual
patches. Will push out iproute2 series after that. Thanks!
[...]