On Thu, Oct 04, 2018 at 02:33:49PM -0700, David Ahern wrote:
> From: David Ahern <dsah...@gmail.com>
>
> Update neigh_dump_info for strict data checking. If the flag is set,
> the dump request is expected to have an ndmsg struct as the header
> potentially followed by one or more attributes. Any data passed in the
> header or as an attribute is taken as a request to influence the data
> returned. Only values supported by the dump handler are allowed to be
> non-0 or set in the request. At the moment only the NDA_IFINDEX and
> NDA_MASTER attributes are supported.
>
> Existing code does not fail the dump if nlmsg_parse fails. That behavior
> is kept for non-strict checking.
>
> Signed-off-by: David Ahern <dsah...@gmail.com>
> ---
> net/core/neighbour.c | 59
> ++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index b06f794bf91e..3130d010b7ad 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2428,13 +2428,14 @@ static int pneigh_dump_table(struct neigh_table *tbl,
> struct sk_buff *skb,
>
> static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> const struct nlmsghdr *nlh = cb->nlh;
> struct neigh_dump_filter filter = {};
> struct nlattr *tb[NDA_MAX + 1];
> struct neigh_table *tbl;
> int t, family, s_t;
> int proxy = 0;
> - int err;
> + int err, i;
>
> family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
>
> @@ -2445,20 +2446,56 @@ static int neigh_dump_info(struct sk_buff *skb,
> struct netlink_callback *cb)
> ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY)
> proxy = 1;
>
> - err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb, NDA_MAX, NULL,
> - cb->extack);
So right, this seems like there was precedent in the kernel where an
nlmsg_parse() would fail and a warning as generated in the logs.
Anyway, as we have decided to not cause such warnings to be printed it
would make sense to make the whole nlmsg_parse() conditional and move it
under the if (strict) branch.
> - if (!err) {
> - if (tb[NDA_IFINDEX]) {
> - if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
> - return -EINVAL;
> - filter.dev_idx = nla_get_u32(tb[NDA_IFINDEX]);
> + if (cb->strict_check) {
> + struct ndmsg *ndm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ndm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + ndm = nlmsg_data(nlh);
> + if (ndm->ndm_pad1 || ndm->ndm_pad2 || ndm->ndm_ifindex ||
> + ndm->ndm_state || ndm->ndm_flags || ndm->ndm_type) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for
> dump request");
> + return -EINVAL;
> }
> - if (tb[NDA_MASTER]) {
> - if (nla_len(tb[NDA_MASTER]) != sizeof(u32))
> + }
> +
> + err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb, NDA_MAX, NULL, extack);
> + if (err < 0) {
> + if (cb->strict_check)
> + return -EINVAL;
> + goto walk_entries;
> + }
> +
> + for (i = 0; i <= NDA_MAX; ++i) {
> + if (!tb[i])
> + continue;
> + switch (i) {
> + case NDA_IFINDEX:
> + if (nla_len(tb[i]) != sizeof(u32)) {
> + NL_SET_ERR_MSG(extack, "Invalid IFINDEX
> attribute");
> return -EINVAL;
> - filter.master_idx = nla_get_u32(tb[NDA_MASTER]);
> + }
> + filter.dev_idx = nla_get_u32(tb[i]);
> + break;
> + case NDA_MASTER:
> + if (nla_len(tb[i]) != sizeof(u32)) {
> + NL_SET_ERR_MSG(extack, "Invalid MASTER
> attribute");
> + return -EINVAL;
> + }
> + filter.master_idx = nla_get_u32(tb[i]);
> + break;
> + default:
> + if (cb->strict_check) {
> + NL_SET_ERR_MSG(extack, "Unsupported attribute
> in dump request");
> + return -EINVAL;
> + }
> }
> }
> +
> +walk_entries:
> s_t = cb->args[0];
>
> for (t = 0; t < NEIGH_NR_TABLES; t++) {
> --
> 2.11.0
>
