| Message ID | 20240216015257.10020-1-stephen@networkplumber.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] netlink: check for NULL attribute in nla_parse_nested_deprecated | expand |
On Thu, Feb 15, 2024 at 05:52:48PM -0800, Stephen Hemminger wrote: > Lots of code in network schedulers has the pattern: > if (!nla) { > NL_SET_ERR_MSG_MOD(extack, "missing attributes"); > return -EINVAL; > } > > err = nla_parse_nested_deprecated(tb, TCA_CSUM_MAX, nla, csum_policy, > NULL); > if (err < 0) > return err; > > The check for nla being NULL can be moved into nla_parse_nested_deprecated(). > Which simplifies lots of places. > > This is safer and won't break other places since: > err = nla_parse_nested_deprecated(tb, TCA_XXXX, NULL, some_policy, NULL); > would have crashed kernel because nla_parse_deprecated derefences the nla > argument before calling __nla_parse. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Hi Stephen, this seems like a good approach to me. Would you also plan to update the schedules at some point?
On Thu, 15 Feb 2024 17:52:48 -0800 Stephen Hemminger wrote: > Lots of code in network schedulers has the pattern: > if (!nla) { > NL_SET_ERR_MSG_MOD(extack, "missing attributes"); > return -EINVAL; > } > > err = nla_parse_nested_deprecated(tb, TCA_CSUM_MAX, nla, csum_policy, > NULL); > if (err < 0) > return err; > > The check for nla being NULL can be moved into nla_parse_nested_deprecated(). > Which simplifies lots of places. If it's mainly TC which has this problem the fix belongs in TC, not in the generic code.
diff --git a/include/net/netlink.h b/include/net/netlink.h index c19ff921b661..05d137283ab0 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -1322,6 +1322,11 @@ static inline int nla_parse_nested_deprecated(struct nlattr *tb[], int maxtype, const struct nla_policy *policy, struct netlink_ext_ack *extack) { + if (!nla) { + NL_SET_ERR_MSG_MOD(extack, "missing attributes"); + return -EINVAL; + } + return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy, NL_VALIDATE_LIBERAL, extack); }
Lots of code in network schedulers has the pattern: if (!nla) { NL_SET_ERR_MSG_MOD(extack, "missing attributes"); return -EINVAL; } err = nla_parse_nested_deprecated(tb, TCA_CSUM_MAX, nla, csum_policy, NULL); if (err < 0) return err; The check for nla being NULL can be moved into nla_parse_nested_deprecated(). Which simplifies lots of places. This is safer and won't break other places since: err = nla_parse_nested_deprecated(tb, TCA_XXXX, NULL, some_policy, NULL); would have crashed kernel because nla_parse_deprecated derefences the nla argument before calling __nla_parse. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- include/net/netlink.h | 5 +++++ 1 file changed, 5 insertions(+)