Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Lin Ma <linma@zju.edu.cn>
To:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, fw@strlen.de, yang.lee@linux.alibaba.com,
        jgg@ziepe.ca, markzhang@nvidia.com, phaddad@nvidia.com,
        yuancan@huawei.com, linma@zju.edu.cn, ohartoov@nvidia.com,
        chenzhongjin@huawei.com, aharonl@nvidia.com, leon@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-rdma@vger.kernel.org
Subject: [PATCH net v1 1/2] netlink: let len field used to parse type-not-care nested attrs
Date:   Mon, 31 Jul 2023 20:12:47 +0800
Message-Id: <20230731121247.3972783-1-linma@zju.edu.cn>
Precedence: bulk

Recently I found several manual parsing cases for nested attributes
whose fix is rather trivial. The pattern for those like below

const struct nla_policy y[...] = {
  ...
  X	= { .type = NLA_NESTED },
  ...
}

nla_for_each_nested/attr(nla, tb[X], ...) {
   // nla_type never used
   ...
   x = nla_data(nla) // directly access nla without length checking
   ....
}

One example can be found in discussion at:
https://lore.kernel.org/all/20230723074504.3706691-1-linma@zju.edu.cn/

In short, the very direct idea to fix such lengh-check-forgotten bug is
add nla_len() checks like

  if (nla_len(nla) < SOME_LEN)
    return -EINVAL;

However, this is tedious and just like Leon said: add another layer of
cabal knowledge. The better solution should leverage the nla_policy and
discard nlattr whose length is invalid when doing parsing. That is, we
should defined a nested_policy for the X above like

  X      = { NLA_POLICY_NESTED(Z) },

But unfortunately, as said above, the nla_type is never used in such
manual parsing cases, which means is difficult to defined a nested
policy Z without breaking user space (they may put weird value in type
of these nlattrs, we have no idea).

To this end, this commit uses the len field in nla_policy crafty and
allow the existing validate_nla checks such type-not-care nested attrs.
In current implementation, for attribute with type NLA_NESTED, the len
field used as the length of the nested_policy:

	{ .type = NLA_NESTED, .nested_policy = policy, .len = maxattr }

	_NLA_POLICY_NESTED(ARRAY_SIZE(policy) - 1, policy)

If one nlattr does not provide policy, like the example X, this len
field is not used. This means we can leverage this field for our end.
This commit introduces one new macro named NLA_POLICY_NESTED_NO_TYPE
and let validate_nla() to use the len field as a hint to check the
nested attributes. Therefore, such manual parsing code can also define
a nla_policy and take advantage of the validation within the existing
parsers like nla_parse_deprecated..

Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 include/net/netlink.h |  6 ++++++
 lib/nlattr.c          | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b12cd957abb4..d825a5672161 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -229,6 +229,9 @@ enum nla_policy_validation {
  *                         nested header (or empty); len field is used if
  *                         nested_policy is also used, for the max attr
  *                         number in the nested policy.
+ *                         For NLA_NESTED whose nested nlattr is not necessary,
+ *                         the len field will indicate the exptected length of
+ *                         them for checking.
  *    NLA_U8, NLA_U16,
  *    NLA_U32, NLA_U64,
  *    NLA_S8, NLA_S16,
@@ -372,6 +375,9 @@ struct nla_policy {
 	_NLA_POLICY_NESTED(ARRAY_SIZE(policy) - 1, policy)
 #define NLA_POLICY_NESTED_ARRAY(policy) \
 	_NLA_POLICY_NESTED_ARRAY(ARRAY_SIZE(policy) - 1, policy)
+/* not care about the nested attributes, just do length check */
+#define NLA_POLICY_NESTED_NO_TYPE(length) \
+	_NLA_POLICY_NESTED(length, NULL)
 #define NLA_POLICY_BITFIELD32(valid) \
 	{ .type = NLA_BITFIELD32, .bitfield32_valid = valid }
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 489e15bde5c1..29a412b41d28 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -488,6 +488,18 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 				 */
 				return err;
 			}
+		} else if (pt->len) {
+			/* length set without nested_policy, the len field will
+			 * be used to check those nested attributes here,
+			 * we will not do parse here but just validation as the
+			 * consumers will do manual parsing.
+			 */
+			const struct nlattr *nla_nested;
+			int rem;
+
+			nla_for_each_attr(nla_nested, nla_data(nla), nla_len(nla), rem)
+				if (nla_len(nla_nested) < pt->len)
+					return -EINVAL;
 		}
 		break;
 	case NLA_NESTED_ARRAY:
-- 
2.17.1