| Message ID | 20190321220504.7642-2-johannes@sipsolutions.net |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | netlink/genetlink: stricter parsing | expand |
Johannes Berg <johannes@sipsolutions.net> wrote: > + * nla_validate_strict - Strictly validate a stream of attributes > + * @head: head of attribute stream > + * @len: length of attribute stream > + * @maxtype: maximum attribute type to be expected > + * @policy: validation policy > + * @extack: extended ACK report struct > + * > + * Validates all attributes in the specified attribute stream against the > + * specified policy. Attributes with a type exceeding maxtype will be > + * ignored. ^^^^^^^^ rejected? > +int nla_validate_strict(const struct nlattr *head, int len, int maxtype, > + const struct nla_policy *policy, > + struct netlink_ext_ack *extack) > +{ > + const struct nlattr *nla; > + int rem; > + > + nla_for_each_attr(nla, head, len, rem) { > + u16 type = nla_type(nla); > + int err; > + > + if (type == 0 || type > maxtype) { > + NL_SET_ERR_MSG(extack, "Unknown attribute type"); Sure looks like it, and thats good.
On Thu, 2019-03-21 at 23:50 +0100, Florian Westphal wrote: > Johannes Berg <johannes@sipsolutions.net> wrote: > > + * nla_validate_strict - Strictly validate a stream of attributes > > + * @head: head of attribute stream > > + * @len: length of attribute stream > > + * @maxtype: maximum attribute type to be expected > > + * @policy: validation policy > > + * @extack: extended ACK report struct > > + * > > + * Validates all attributes in the specified attribute stream against the > > + * specified policy. Attributes with a type exceeding maxtype will be > > + * ignored. > > ^^^^^^^^ > rejected? Oops, right, I didn't pay attentino to the docs at all. But anyway, I don't think I want to do this. I'm tempted to do the following: * add an enum netlink_validation { NETLINK_VALIDATION_LIBERAL, // old behaviour NETLINK_VALIDATION_STRICT_MSG, // current strict NETLINK_VALIDATION_STRICT, // strict message & attribute }; * add __*_parse()/__*_validate() that get a new argument from this enum * for all existing callers of *_parse()/*_validate() add a new inline *_parse_liberal()/*_validate_liberal() and replace all calls, using _LIBERAL * change all existing *_parse_strict() to a new *_parse_strict_msg() inline using _STRICT_MSG * re-introduce *_parse()/*_validate() as being fully _STRICT Also, do this before the generic netlink changes, so generic netlink never gets the intermediate "STRICT_MSG" level. That addresses two things: 1) my table from the cover letter would be - at least for genl - what I want it to be, for some rtnetlink commands we'd have "strict_msg" semantics 2) Default of *_parse()/*_validate() becomes to be strict for new code, so we don't need to pay as much attention to it - it'll be easier to see if somebody adds a call explicitly calling the more liberal versions. I'm tempted to not even add inline wrappers for this reason but to just open-code the __*() versions with the enum value instead (it's spatch, after all). PS: STRICT_MSG (currently _strict()) semantics are a bit strange because an attribute type that's out of range is rejected, while one that's in range but has no policy is accepted; yet the range is prone to change all the time... The "strict_start_type" fixes that though, if applied. johannes
diff --git a/include/net/netlink.h b/include/net/netlink.h index 23f27b0b3cef..e211b745008a 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -66,6 +66,7 @@ * nlmsg_find_attr() find an attribute in a message * nlmsg_for_each_msg() loop over all messages * nlmsg_validate() validate netlink message incl. attrs + * nlmsg_validate_strict() strictly validate netlink message/attrs * nlmsg_for_each_attr() loop over all attributes * * Misc: @@ -149,6 +150,7 @@ * nla_ok(nla, remaining) does nla fit into remaining bytes? * nla_next(nla, remaining) get next netlink attribute * nla_validate() validate a stream of attributes + * nla_validate_strict() strictly validate a stream of attributes * nla_validate_nested() validate a stream of nested attributes * nla_find() find attribute in stream of attributes * nla_find_nested() find attribute in nested attributes @@ -374,6 +376,9 @@ int nlmsg_notify(struct sock *sk, struct sk_buff *skb, u32 portid, int nla_validate(const struct nlattr *head, int len, int maxtype, const struct nla_policy *policy, struct netlink_ext_ack *extack); +int nla_validate_strict(const struct nlattr *head, int len, int maxtype, + const struct nla_policy *policy, + struct netlink_ext_ack *extack); int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, int len, const struct nla_policy *policy, struct netlink_ext_ack *extack); @@ -582,6 +587,27 @@ static inline int nlmsg_validate(const struct nlmsghdr *nlh, extack); } +/** + * nlmsg_validate_strict - strictly validate a netlink message including attrs + * @nlh: netlinket message header + * @hdrlen: length of familiy specific header + * @maxtype: maximum attribute type to be expected + * @policy: validation policy + * @extack: extended ACK report struct + */ +static inline int nlmsg_validate_strict(const struct nlmsghdr *nlh, + int hdrlen, int maxtype, + const struct nla_policy *policy, + struct netlink_ext_ack *extack) +{ + if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) + return -EINVAL; + + return nla_validate_strict(nlmsg_attrdata(nlh, hdrlen), + nlmsg_attrlen(nlh, hdrlen), maxtype, + policy, extack); +} + /** * nlmsg_report - need to report back to application? * @nlh: netlink message header diff --git a/lib/nlattr.c b/lib/nlattr.c index d26de6156b97..3ccef39b89de 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -347,6 +347,51 @@ int nla_validate(const struct nlattr *head, int len, int maxtype, } EXPORT_SYMBOL(nla_validate); +/** + * nla_validate_strict - Strictly validate a stream of attributes + * @head: head of attribute stream + * @len: length of attribute stream + * @maxtype: maximum attribute type to be expected + * @policy: validation policy + * @extack: extended ACK report struct + * + * Validates all attributes in the specified attribute stream against the + * specified policy. Attributes with a type exceeding maxtype will be + * ignored. See documenation of struct nla_policy for more details. + * + * Returns 0 on success or a negative error code. + */ +int nla_validate_strict(const struct nlattr *head, int len, int maxtype, + const struct nla_policy *policy, + struct netlink_ext_ack *extack) +{ + const struct nlattr *nla; + int rem; + + nla_for_each_attr(nla, head, len, rem) { + u16 type = nla_type(nla); + int err; + + if (type == 0 || type > maxtype) { + NL_SET_ERR_MSG(extack, "Unknown attribute type"); + return -EINVAL; + } + + err = validate_nla(nla, maxtype, policy, extack); + + if (err < 0) + return err; + } + + if (unlikely(rem > 0)) { + NL_SET_ERR_MSG(extack, "bytes leftover after parsing attributes"); + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(nla_validate_strict); + /** * nla_policy_len - Determin the max. length of a policy * @policy: policy to use