| Message ID | 20201005155753.2333882-6-kuba@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ethtool: allow dumping policies to user space | expand |
On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote: > We don't have good validation policy for existing unsigned int attrs > which serve as flags (for new ones we could use NLA_BITFIELD32). > With increased use of policy dumping having the validation be > expressed as part of the policy is important. Add validation > policy in form of a mask of supported/valid bits. Nice, I'll surely use this as well somewhere :) > #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition)) > +#define NLA_ENSURE_UINT_TYPE(tp) \ > + (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \ > + tp == NLA_U32 || tp == NLA_U64) + tp) > #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \ nit: maybe express this (_OR_BINARY_TYPE) in terms of UINT_TYPE() || tp==NLA_BINARY? Doesn't matter much though. > +static int nla_validate_mask(const struct nla_policy *pt, > + const struct nlattr *nla, > + struct netlink_ext_ack *extack) > +{ > + u64 value; > + > + switch (pt->type) { > + case NLA_U8: > + value = nla_get_u8(nla); > + break; > + case NLA_U16: > + value = nla_get_u16(nla); > + break; > + case NLA_U32: > + value = nla_get_u32(nla); > + break; > + case NLA_U64: > + value = nla_get_u64(nla); > + break; > + default: > + return -EINVAL; > + } > + > + if (value & ~(u64)pt->mask) { > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > + return -EINVAL; You had an export of the valid bits there in ethtool, using the cookie. Just pointing out you lost it now. I'm not sure I like using the cookie, that seems a bit strange, but we could easily define a different attr? OTOH, one can always query the policy export too (which hopefully got wired up) so it wouldn't really matter much. Either way is fine with me on both of these points. Reviewed-by: Johannes Berg <johannes@sipsolutions.net> Thanks! johannes
On Mon, 05 Oct 2020 21:05:23 +0200 Johannes Berg wrote: > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote: > > We don't have good validation policy for existing unsigned int attrs > > which serve as flags (for new ones we could use NLA_BITFIELD32). > > With increased use of policy dumping having the validation be > > expressed as part of the policy is important. Add validation > > policy in form of a mask of supported/valid bits. > > Nice, I'll surely use this as well somewhere :) > > > #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition)) > > +#define NLA_ENSURE_UINT_TYPE(tp) \ > > + (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \ > > + tp == NLA_U32 || tp == NLA_U64) + tp) > > #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \ > > nit: maybe express this (_OR_BINARY_TYPE) in terms of UINT_TYPE() || > tp==NLA_BINARY? Doesn't matter much though. Will do! > > +static int nla_validate_mask(const struct nla_policy *pt, > > + const struct nlattr *nla, > > + struct netlink_ext_ack *extack) > > +{ > > + u64 value; > > + > > + switch (pt->type) { > > + case NLA_U8: > > + value = nla_get_u8(nla); > > + break; > > + case NLA_U16: > > + value = nla_get_u16(nla); > > + break; > > + case NLA_U32: > > + value = nla_get_u32(nla); > > + break; > > + case NLA_U64: > > + value = nla_get_u64(nla); > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + if (value & ~(u64)pt->mask) { > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > + return -EINVAL; > > You had an export of the valid bits there in ethtool, using the cookie. > Just pointing out you lost it now. I'm not sure I like using the cookie, > that seems a bit strange, but we could easily define a different attr? > > OTOH, one can always query the policy export too (which hopefully got > wired up) so it wouldn't really matter much. My thinking is that there are no known uses of the cookie, it'd only have practical use to test for new flags - and we're adding first new flag in 5.10. > Either way is fine with me on both of these points. > > Reviewed-by: Johannes Berg <johannes@sipsolutions.net> Thanks!
On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote: > > > + if (value & ~(u64)pt->mask) { > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > > + return -EINVAL; > > > > You had an export of the valid bits there in ethtool, using the cookie. > > Just pointing out you lost it now. I'm not sure I like using the cookie, > > that seems a bit strange, but we could easily define a different attr? > > > > OTOH, one can always query the policy export too (which hopefully got > > wired up) so it wouldn't really matter much. > > My thinking is that there are no known uses of the cookie, it'd only > have practical use to test for new flags - and we're adding first new > flag in 5.10. Hm, wait, not sure I understand? You _had_ this in ethtool, but you removed it now. And you're not keeping it here, afaict. I can't disagree on the "no known uses of the cookie" part, but it feels odd to me anyway - since that is just another netlink message (*), you could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the data into the cookie? But then are you saying the new flags are only in 5.10 so the policy export will be sufficient, since that's also wired up now? johannes (*) in a way - the ack message has the "legacy" fixed part before the attrs, of course
On Mon, Oct 05, 2020 at 09:05:23PM +0200, Johannes Berg wrote: > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote: > > > +static int nla_validate_mask(const struct nla_policy *pt, > > + const struct nlattr *nla, > > + struct netlink_ext_ack *extack) > > +{ > > + u64 value; > > + > > + switch (pt->type) { > > + case NLA_U8: > > + value = nla_get_u8(nla); > > + break; > > + case NLA_U16: > > + value = nla_get_u16(nla); > > + break; > > + case NLA_U32: > > + value = nla_get_u32(nla); > > + break; > > + case NLA_U64: > > + value = nla_get_u64(nla); > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + if (value & ~(u64)pt->mask) { > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > + return -EINVAL; > > You had an export of the valid bits there in ethtool, using the cookie. > Just pointing out you lost it now. I'm not sure I like using the cookie, > that seems a bit strange, but we could easily define a different attr? The idea behind the cookie was that if new userspace sends a request with multiple flags which may not be supported by an old kernel, getting only -EOPNOTSUPP (and badattr pointing to the flags) would not be very helpful as multiple iteration would be necessary to find out which flags are supported and which not. > OTOH, one can always query the policy export too (which hopefully got > wired up) so it wouldn't really matter much. But yes, if userspace can get supported flags from policy dump, it can check them in advance and either bail out (if one of essential flags is unsupported) or send only supported flags. I'm not exactly happy with the prospect of having to do a full policy dump before each such request, thought. Michal
On Mon, 2020-10-05 at 21:28 +0200, Michal Kubecek wrote: > > > + if (value & ~(u64)pt->mask) { > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > > + return -EINVAL; > > > > You had an export of the valid bits there in ethtool, using the cookie. > > Just pointing out you lost it now. I'm not sure I like using the cookie, > > that seems a bit strange, but we could easily define a different attr? > > The idea behind the cookie was that if new userspace sends a request > with multiple flags which may not be supported by an old kernel, getting > only -EOPNOTSUPP (and badattr pointing to the flags) would not be very > helpful as multiple iteration would be necessary to find out which flags > are supported and which not. Message crossing, I guess. I completely agree. I just don't like using the (somewhat vague) _cookie_ for that vs. adding a new explicit NLMSGERR_ATTR_SOMETHING :) I would totally support doing that here in the general validation code, but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate attribute for it. johannes
On Mon, 05 Oct 2020 21:25:57 +0200 Johannes Berg wrote: > On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote: > > > > > + if (value & ~(u64)pt->mask) { > > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > > > + return -EINVAL; > > > > > > You had an export of the valid bits there in ethtool, using the cookie. > > > Just pointing out you lost it now. I'm not sure I like using the cookie, > > > that seems a bit strange, but we could easily define a different attr? > > > > > > OTOH, one can always query the policy export too (which hopefully got > > > wired up) so it wouldn't really matter much. > > > > My thinking is that there are no known uses of the cookie, it'd only > > have practical use to test for new flags - and we're adding first new > > flag in 5.10. > > Hm, wait, not sure I understand? > > You _had_ this in ethtool, but you removed it now. And you're not > keeping it here, afaict. > > I can't disagree on the "no known uses of the cookie" part, but it feels > odd to me anyway - since that is just another netlink message (*), you > could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the > data into the cookie? > > But then are you saying the new flags are only in 5.10 so the policy > export will be sufficient, since that's also wired up now? Right, I was commenting on the need to keep the cookie for backward compat. My preference is to do a policy dump to check the capabilities of the kernel rather than shoot messages at it and then try to work backward based on the info returned in extack. > johannes > > (*) in a way - the ack message has the "legacy" fixed part before the > attrs, of course >
On Mon, 2020-10-05 at 12:34 -0700, Jakub Kicinski wrote: > > > My thinking is that there are no known uses of the cookie, it'd only Ahh. I completely misinterpreted the word "uses" here - you meant, I think (now), "uses of the cookie in the way that it was done in ethtool before". I read over this because it seemed in a way obviously right and also obviously wrong (no other uses of the cookie in ethtool and clearly uses of the cookie elsewhere, respectively)... > Right, I was commenting on the need to keep the cookie for backward > compat. Right ... > My preference is to do a policy dump to check the capabilities of the > kernel rather than shoot messages at it and then try to work backward > based on the info returned in extack. I guess Michal disagrees ;-) I can see both points of view though - if you have just a single attribute it's basically the same, but once you have two or more it's way less complex to just query before, I'd think. johannes
On Mon, 05 Oct 2020 21:31:13 +0200 Johannes Berg wrote: > On Mon, 2020-10-05 at 21:28 +0200, Michal Kubecek wrote: > > > > > + if (value & ~(u64)pt->mask) { > > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > > > + return -EINVAL; > > > > > > You had an export of the valid bits there in ethtool, using the cookie. > > > Just pointing out you lost it now. I'm not sure I like using the cookie, > > > that seems a bit strange, but we could easily define a different attr? > > > > The idea behind the cookie was that if new userspace sends a request > > with multiple flags which may not be supported by an old kernel, getting > > only -EOPNOTSUPP (and badattr pointing to the flags) would not be very > > helpful as multiple iteration would be necessary to find out which flags > > are supported and which not. > > Message crossing, I guess. > > I completely agree. I just don't like using the (somewhat vague) > _cookie_ for that vs. adding a new explicit NLMSGERR_ATTR_SOMETHING :) > > I would totally support doing that here in the general validation code, > but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate > attribute for it. Hm. Perhaps we can do a partial policy dump into the extack? NL_POLICY_TYPE_ATTR_TYPE etc.? Either way, I don't feel like this series needs it. > > I'm not exactly happy with the prospect of having to do a full policy > > dump before each such request, thought. I tried to code it up and it gets rather ugly. One has to selectively suppress error messages for stuff that's known to be optional, and keep retrying. The policy dump is much, much cleaner (and the policy for an op is rather tiny - one nested policy of the header with 3? attrs in it).
On Mon, Oct 05, 2020 at 12:34:14PM -0700, Jakub Kicinski wrote: > On Mon, 05 Oct 2020 21:25:57 +0200 Johannes Berg wrote: > > On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote: > > > > > > > + if (value & ~(u64)pt->mask) { > > > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); > > > > > + return -EINVAL; > > > > > > > > You had an export of the valid bits there in ethtool, using the cookie. > > > > Just pointing out you lost it now. I'm not sure I like using the cookie, > > > > that seems a bit strange, but we could easily define a different attr? > > > > > > > > OTOH, one can always query the policy export too (which hopefully got > > > > wired up) so it wouldn't really matter much. > > > > > > My thinking is that there are no known uses of the cookie, it'd only > > > have practical use to test for new flags - and we're adding first new > > > flag in 5.10. > > > > Hm, wait, not sure I understand? > > > > You _had_ this in ethtool, but you removed it now. And you're not > > keeping it here, afaict. > > > > I can't disagree on the "no known uses of the cookie" part, but it feels > > odd to me anyway - since that is just another netlink message (*), you > > could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the > > data into the cookie? > > > > But then are you saying the new flags are only in 5.10 so the policy > > export will be sufficient, since that's also wired up now? > > Right, I was commenting on the need to keep the cookie for backward > compat. > > My preference is to do a policy dump to check the capabilities of the > kernel rather than shoot messages at it and then try to work backward > based on the info returned in extack. The reason I used the extack was that that way, the "standard" case of kernel and ethtool in sync (or even old ethtool with new kernel) would still use one request and only new ethtool against old kernel would end up doing two. Also, the extra request would be relatively short and so would be the reply to it. On the other hand, using the policy dump would allow checking not only support for new flags but also support for new request attributes so in the long term, it could actually make things simpler. Michal
On Mon, 2020-10-05 at 12:40 -0700, Jakub Kicinski wrote: > > I would totally support doing that here in the general validation code, > > but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate > > attribute for it. > > Hm. Perhaps we can do a partial policy dump into the extack? Hm. I like that idea. If we have NLMSGERR_ATTR_OFFS we could accompany that with the sub- policy for that particular attribute, something like [NLMSGERR_ATTR_POLICY] = nested { [NL_POLICY_TYPE_ATTR_TYPE] = ... [NL_POLICY_TYPE_ATTR_MASK] = ... } which we could basically do by factoring out the inner portion of netlink_policy_dump_write(): attr = nla_nest_start(skb, state->attr_idx); if (!attr) goto nla_put_failure; ... nla_nest_end(skb, attr); from there into a separate function, give it the pt and the nested attribute (what's "state->attr_idx" here) as arguments, and then we call it with NLMSGERR_ATTR_POLICY from here, and with "state->attr_idx" from netlink_policy_dump_write() :-) Nice, easy & useful, maybe I'll code it up tomorrow. > Either way, I don't feel like this series needs it. Fair enough. johannes
On Mon, 2020-10-05 at 21:53 +0200, Johannes Berg wrote: > On Mon, 2020-10-05 at 12:40 -0700, Jakub Kicinski wrote: > > > > I would totally support doing that here in the general validation code, > > > but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate > > > attribute for it. > > > > Hm. Perhaps we can do a partial policy dump into the extack? > > Hm. I like that idea. > > If we have NLMSGERR_ATTR_OFFS we could accompany that with the sub- > policy for that particular attribute, something like > > [NLMSGERR_ATTR_POLICY] = nested { > [NL_POLICY_TYPE_ATTR_TYPE] = ... > [NL_POLICY_TYPE_ATTR_MASK] = ... > } > > which we could basically do by factoring out the inner portion of > netlink_policy_dump_write(): > > attr = nla_nest_start(skb, state->attr_idx); > if (!attr) > goto nla_put_failure; > ... > nla_nest_end(skb, attr); > > from there into a separate function, give it the pt and the nested > attribute (what's "state->attr_idx" here) as arguments, and then we call > it with NLMSGERR_ATTR_POLICY from here, and with "state->attr_idx" from > netlink_policy_dump_write() :-) > > Nice, easy & useful, maybe I'll code it up tomorrow. OK I thought about it a bit more and looked at the code, and it's not actually possible to do easily right now, because we can't actually point to the bad attribute from the general lib/nlattr.c code ... Why? Because we don't know right now, e.g. for nla_validate(), where in the message we started validation, i.e. the offset of the "head" inside the particular message. For nlmsg_parse() and friends that's a bit easier, but it needs more rejiggering than I'm willing to do tonight ;) johannes
On Mon, 05 Oct 2020 22:12:25 +0200 Johannes Berg wrote: > On Mon, 2020-10-05 at 21:53 +0200, Johannes Berg wrote: > > Hm. I like that idea. > > > > If we have NLMSGERR_ATTR_OFFS we could accompany that with the sub- > > policy for that particular attribute, something like > > > > [NLMSGERR_ATTR_POLICY] = nested { > > [NL_POLICY_TYPE_ATTR_TYPE] = ... > > [NL_POLICY_TYPE_ATTR_MASK] = ... > > } > > > > which we could basically do by factoring out the inner portion of > > netlink_policy_dump_write(): > > > > attr = nla_nest_start(skb, state->attr_idx); > > if (!attr) > > goto nla_put_failure; > > ... > > nla_nest_end(skb, attr); > > > > from there into a separate function, give it the pt and the nested > > attribute (what's "state->attr_idx" here) as arguments, and then we call > > it with NLMSGERR_ATTR_POLICY from here, and with "state->attr_idx" from > > netlink_policy_dump_write() :-) > > > > Nice, easy & useful, maybe I'll code it up tomorrow. > > OK I thought about it a bit more and looked at the code, and it's not > actually possible to do easily right now, because we can't actually > point to the bad attribute from the general lib/nlattr.c code ... > > Why? Because we don't know right now, e.g. for nla_validate(), where in > the message we started validation, i.e. the offset of the "head" inside > the particular message. > > For nlmsg_parse() and friends that's a bit easier, but it needs more > rejiggering than I'm willing to do tonight ;) I thought we'd record the const struct nla_policy *tp for the failing attr in struct netlink_ext_ack and output based on that.
On Mon, 2020-10-05 at 15:21 -0700, Jakub Kicinski wrote: > > > Nice, easy & useful, maybe I'll code it up tomorrow. > > > > OK I thought about it a bit more and looked at the code, and it's not > > actually possible to do easily right now, because we can't actually > > point to the bad attribute from the general lib/nlattr.c code ... > > > > Why? Because we don't know right now, e.g. for nla_validate(), where in > > the message we started validation, i.e. the offset of the "head" inside > > the particular message. > > > > For nlmsg_parse() and friends that's a bit easier, but it needs more > > rejiggering than I'm willing to do tonight ;) > > I thought we'd record the const struct nla_policy *tp for the failing > attr in struct netlink_ext_ack and output based on that. We could, but it's a bit useless if you know "which" attribute caused the issue, but you don't know where it was in the message? That way you wouldn't know the nesting level etc. I mean, we actually have that problem today - the generic lib/nlattr.c policy violation doesn't tell you where exactly the problem occurred, so it'd be good to fix that regardless. johannes
On Tue, 2020-10-06 at 08:37 +0200, Johannes Berg wrote: > On Mon, 2020-10-05 at 15:21 -0700, Jakub Kicinski wrote: > > > > > Nice, easy & useful, maybe I'll code it up tomorrow. > > > > > > OK I thought about it a bit more and looked at the code, and it's not > > > actually possible to do easily right now, because we can't actually > > > point to the bad attribute from the general lib/nlattr.c code ... > > > > > > Why? Because we don't know right now, e.g. for nla_validate(), where in > > > the message we started validation, i.e. the offset of the "head" inside > > > the particular message. > > > > > > For nlmsg_parse() and friends that's a bit easier, but it needs more > > > rejiggering than I'm willing to do tonight ;) > > > > I thought we'd record the const struct nla_policy *tp for the failing > > attr in struct netlink_ext_ack and output based on that. > > We could, but it's a bit useless if you know "which" attribute caused > the issue, but you don't know where it was in the message? That way you > wouldn't know the nesting level etc. > > I mean, we actually have that problem today - the generic lib/nlattr.c > policy violation doesn't tell you where exactly the problem occurred, so > it'd be good to fix that regardless. Just for the record: I'm obviously *completely* confused, of course we do say which attribute was bad, I was just looking for the offset, but we carry around a pointer and calculate the offset later, with NL_SET_ERR_MSG_ATTR() or NL_SET_BAD_ATTR(). Which means this isn't so hard ... johannes
diff --git a/include/net/netlink.h b/include/net/netlink.h index 5a5ff97cc596..844b53dbba68 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -200,6 +200,7 @@ enum nla_policy_validation { NLA_VALIDATE_RANGE_WARN_TOO_LONG, NLA_VALIDATE_MIN, NLA_VALIDATE_MAX, + NLA_VALIDATE_MASK, NLA_VALIDATE_RANGE_PTR, NLA_VALIDATE_FUNCTION, }; @@ -317,6 +318,7 @@ struct nla_policy { u16 len; union { const u32 bitfield32_valid; + const u32 mask; const char *reject_message; const struct nla_policy *nested_policy; struct netlink_range_validation *range; @@ -363,6 +365,9 @@ struct nla_policy { { .type = NLA_BITFIELD32, .bitfield32_valid = valid } #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition)) +#define NLA_ENSURE_UINT_TYPE(tp) \ + (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \ + tp == NLA_U32 || tp == NLA_U64) + tp) #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \ (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \ tp == NLA_U32 || tp == NLA_U64 || \ @@ -415,6 +420,12 @@ struct nla_policy { .max = _max, \ } +#define NLA_POLICY_MASK(tp, _mask) { \ + .type = NLA_ENSURE_UINT_TYPE(tp), \ + .validation_type = NLA_VALIDATE_MASK, \ + .mask = _mask, \ +} + #define NLA_POLICY_VALIDATE_FN(tp, fn, ...) { \ .type = NLA_ENSURE_NO_VALIDATION_PTR(tp), \ .validation_type = NLA_VALIDATE_FUNCTION, \ diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h index eac8a6a648ea..d02e472ba54c 100644 --- a/include/uapi/linux/netlink.h +++ b/include/uapi/linux/netlink.h @@ -331,6 +331,7 @@ enum netlink_attribute_type { * the index, if limited inside the nesting (U32) * @NL_POLICY_TYPE_ATTR_BITFIELD32_MASK: valid mask for the * bitfield32 type (U32) + * @NL_POLICY_TYPE_ATTR_MASK: mask of valid bits for unsigned integers (U64) * @NL_POLICY_TYPE_ATTR_PAD: pad attribute for 64-bit alignment */ enum netlink_policy_type_attr { @@ -346,6 +347,7 @@ enum netlink_policy_type_attr { NL_POLICY_TYPE_ATTR_POLICY_MAXTYPE, NL_POLICY_TYPE_ATTR_BITFIELD32_MASK, NL_POLICY_TYPE_ATTR_PAD, + NL_POLICY_TYPE_ATTR_MASK, /* keep last */ __NL_POLICY_TYPE_ATTR_MAX, diff --git a/lib/nlattr.c b/lib/nlattr.c index 80ff9fe83696..9c99f5daa4d2 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -323,6 +323,37 @@ static int nla_validate_int_range(const struct nla_policy *pt, } } +static int nla_validate_mask(const struct nla_policy *pt, + const struct nlattr *nla, + struct netlink_ext_ack *extack) +{ + u64 value; + + switch (pt->type) { + case NLA_U8: + value = nla_get_u8(nla); + break; + case NLA_U16: + value = nla_get_u16(nla); + break; + case NLA_U32: + value = nla_get_u32(nla); + break; + case NLA_U64: + value = nla_get_u64(nla); + break; + default: + return -EINVAL; + } + + if (value & ~(u64)pt->mask) { + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set"); + return -EINVAL; + } + + return 0; +} + static int validate_nla(const struct nlattr *nla, int maxtype, const struct nla_policy *policy, unsigned int validate, struct netlink_ext_ack *extack, unsigned int depth) @@ -503,6 +534,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype, if (err) return err; break; + case NLA_VALIDATE_MASK: + err = nla_validate_mask(pt, nla, extack); + if (err) + return err; + break; case NLA_VALIDATE_FUNCTION: if (pt->validate) { err = pt->validate(nla, extack); diff --git a/net/netlink/policy.c b/net/netlink/policy.c index cf23c0151721..ee26d01328ee 100644 --- a/net/netlink/policy.c +++ b/net/netlink/policy.c @@ -263,6 +263,14 @@ int netlink_policy_dump_write(struct sk_buff *skb, else type = NL_ATTR_TYPE_U64; + if (pt->validation_type == NLA_VALIDATE_MASK) { + if (nla_put_u64_64bit(skb, NL_POLICY_TYPE_ATTR_MASK, + pt->mask, + NL_POLICY_TYPE_ATTR_PAD)) + goto nla_put_failure; + break; + } + nla_get_range_unsigned(pt, &range); if (nla_put_u64_64bit(skb, NL_POLICY_TYPE_ATTR_MIN_VALUE_U,
We don't have good validation policy for existing unsigned int attrs which serve as flags (for new ones we could use NLA_BITFIELD32). With increased use of policy dumping having the validation be expressed as part of the policy is important. Add validation policy in form of a mask of supported/valid bits. Support u64 in the uAPI to be future-proof, but really for now the embedded mask member can only hold 32 bits, so anything with bit 32+ set will always fail validation. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: jiri@resnulli.us CC: dsahern@gmail.com CC: pablo@netfilter.org --- include/net/netlink.h | 11 +++++++++++ include/uapi/linux/netlink.h | 2 ++ lib/nlattr.c | 36 ++++++++++++++++++++++++++++++++++++ net/netlink/policy.c | 8 ++++++++ 4 files changed, 57 insertions(+)