| Message ID | 20230718075234.3863-2-fw@strlen.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: nf_tables: use NLA_POLICY_MASK instead of manual checks | expand |
On Tue, 18 Jul 2023 09:52:29 +0200 Florian Westphal wrote: > __NLA_IS_BEINT_TYPE(tp) isn't useful. NLA_BE16/32 are identical to > NLA_U16/32, the only difference is that it tells the netlink validation > functions that byteorder conversion might be needed before comparing > the value to the policy min/max ones. > > After this change all policy macros that can be used with UINT types, > such as NLA_POLICY_MASK() can also be used with NLA_BE16/32. > > This will be used to validate nf_tables flag attributes which > are in bigendian byte order. Semi-related, how well do we do with NLA_F_NET_BYTEORDER? On a quick grep we were using it in the kernel -> user direction but not validating on input. Is that right?
Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 18 Jul 2023 09:52:29 +0200 Florian Westphal wrote: > > __NLA_IS_BEINT_TYPE(tp) isn't useful. NLA_BE16/32 are identical to > > NLA_U16/32, the only difference is that it tells the netlink validation > > functions that byteorder conversion might be needed before comparing > > the value to the policy min/max ones. > > > > After this change all policy macros that can be used with UINT types, > > such as NLA_POLICY_MASK() can also be used with NLA_BE16/32. > > > > This will be used to validate nf_tables flag attributes which > > are in bigendian byte order. > > Semi-related, how well do we do with NLA_F_NET_BYTEORDER? Looks incomplete at best. > On a quick grep we were using it in the kernel -> user > direction but not validating on input. Is that right? Looks like ipset is the only user, it sets it for kernel->user dir. I see ipset userspace even sets it on user -> kernel dir but like you say, its not checked and BE encoding is assumed on kernel side. From a quick glance in ipset all Uxx types are always treated as bigendian, which would mean things should not fall apart if ipset stops announcing NLA_F_NET_BYTEORDER. Not sure its worth risking any breakage though. I suspect that in practice, given both producer and consumer need to agree of the meaning of type "12345" anyway its easier to just agree on the byte ordering as well. Was there a specific reason for the question?
On Wed, 19 Jul 2023 04:53:23 +0200 Florian Westphal wrote: > > On a quick grep we were using it in the kernel -> user > > direction but not validating on input. Is that right? > > Looks like ipset is the only user, it sets it for kernel->user > dir. > > I see ipset userspace even sets it on user -> kernel dir but > like you say, its not checked and BE encoding is assumed on > kernel side. > > From a quick glance in ipset all Uxx types are always treated as > bigendian, which would mean things should not fall apart if ipset > stops announcing NLA_F_NET_BYTEORDER. Not sure its worth risking > any breakage though. > > I suspect that in practice, given both producer and consumer need > to agree of the meaning of type "12345" anyway its easier to just > agree on the byte ordering as well. > > Was there a specific reason for the question? I was wondering what we should do with it going forward. Looks like it was added by 8f4c1f9b049d ("[NETLINK]: Introduce nested and byteorder flag to netlink attribute") which says: The byte-order flag is yet unused, it's intended use is to allow automatic byte order convertions for all atomic types. That idea clearly never gained traction. If nobody knows of any use of the flag outside of ipset I'm tempted to send a patch to officially deprecate it and possibly reuse that bit for something else later on?
On Wed, 19 Jul 2023, Florian Westphal wrote: > Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 18 Jul 2023 09:52:29 +0200 Florian Westphal wrote: > > > __NLA_IS_BEINT_TYPE(tp) isn't useful. NLA_BE16/32 are identical to > > > NLA_U16/32, the only difference is that it tells the netlink validation > > > functions that byteorder conversion might be needed before comparing > > > the value to the policy min/max ones. > > > > > > After this change all policy macros that can be used with UINT types, > > > such as NLA_POLICY_MASK() can also be used with NLA_BE16/32. > > > > > > This will be used to validate nf_tables flag attributes which > > > are in bigendian byte order. > > > > Semi-related, how well do we do with NLA_F_NET_BYTEORDER? > > Looks incomplete at best. > > > On a quick grep we were using it in the kernel -> user > > direction but not validating on input. Is that right? > > Looks like ipset is the only user, it sets it for kernel->user > dir. > > I see ipset userspace even sets it on user -> kernel dir but > like you say, its not checked and BE encoding is assumed on > kernel side. > > From a quick glance in ipset all Uxx types are always treated as > bigendian, which would mean things should not fall apart if ipset > stops announcing NLA_F_NET_BYTEORDER. Not sure its worth risking > any breakage though. Yes, ipset treats all uxx types as netorder. It checks the presence of the NLA_F_NET_BYTEORDER, see the ip_set_attr_netorder() and ip_set_optattr_netorder() functions in include/linux/netfilter/ipset/ip_set.h which are then used at input validation. The userspace tool also uses and checks the flag in lib/session.c, but it accepts hostorder as well. > I suspect that in practice, given both producer and consumer need > to agree of the meaning of type "12345" anyway its easier to just > agree on the byte ordering as well. > > Was there a specific reason for the question? Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics H-1525 Budapest 114, POB. 49, Hungary
diff --git a/include/net/netlink.h b/include/net/netlink.h index b12cd957abb4..8a7cd1170e1f 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -375,12 +375,11 @@ struct nla_policy { #define NLA_POLICY_BITFIELD32(valid) \ { .type = NLA_BITFIELD32, .bitfield32_valid = valid } -#define __NLA_IS_UINT_TYPE(tp) \ - (tp == NLA_U8 || tp == NLA_U16 || tp == NLA_U32 || tp == NLA_U64) +#define __NLA_IS_UINT_TYPE(tp) \ + (tp == NLA_U8 || tp == NLA_U16 || tp == NLA_U32 || \ + tp == NLA_U64 || tp == NLA_BE16 || tp == NLA_BE32) #define __NLA_IS_SINT_TYPE(tp) \ (tp == NLA_S8 || tp == NLA_S16 || tp == NLA_S32 || tp == NLA_S64) -#define __NLA_IS_BEINT_TYPE(tp) \ - (tp == NLA_BE16 || tp == NLA_BE32) #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition)) #define NLA_ENSURE_UINT_TYPE(tp) \ @@ -394,7 +393,6 @@ struct nla_policy { #define NLA_ENSURE_INT_OR_BINARY_TYPE(tp) \ (__NLA_ENSURE(__NLA_IS_UINT_TYPE(tp) || \ __NLA_IS_SINT_TYPE(tp) || \ - __NLA_IS_BEINT_TYPE(tp) || \ tp == NLA_MSECS || \ tp == NLA_BINARY) + tp) #define NLA_ENSURE_NO_VALIDATION_PTR(tp) \ @@ -402,8 +400,6 @@ struct nla_policy { tp != NLA_REJECT && \ tp != NLA_NESTED && \ tp != NLA_NESTED_ARRAY) + tp) -#define NLA_ENSURE_BEINT_TYPE(tp) \ - (__NLA_ENSURE(__NLA_IS_BEINT_TYPE(tp)) + tp) #define NLA_POLICY_RANGE(tp, _min, _max) { \ .type = NLA_ENSURE_INT_OR_BINARY_TYPE(tp), \ diff --git a/lib/nlattr.c b/lib/nlattr.c index 489e15bde5c1..7a2b6c38fd59 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -355,6 +355,12 @@ static int nla_validate_mask(const struct nla_policy *pt, case NLA_U64: value = nla_get_u64(nla); break; + case NLA_BE16: + value = ntohs(nla_get_be16(nla)); + break; + case NLA_BE32: + value = ntohl(nla_get_be32(nla)); + break; default: return -EINVAL; }
__NLA_IS_BEINT_TYPE(tp) isn't useful. NLA_BE16/32 are identical to NLA_U16/32, the only difference is that it tells the netlink validation functions that byteorder conversion might be needed before comparing the value to the policy min/max ones. After this change all policy macros that can be used with UINT types, such as NLA_POLICY_MASK() can also be used with NLA_BE16/32. This will be used to validate nf_tables flag attributes which are in bigendian byte order. Signed-off-by: Florian Westphal <fw@strlen.de> --- include/net/netlink.h | 10 +++------- lib/nlattr.c | 6 ++++++ 2 files changed, 9 insertions(+), 7 deletions(-)