[PATCH net-next v3 08/13] tools: ynl-gen: only validate nested array payload

Asbjørn Sloth Tønnesen posted 13 patches 2 weeks, 6 days ago
There is a newer version of this series
[PATCH net-next v3 08/13] tools: ynl-gen: only validate nested array payload
Posted by Asbjørn Sloth Tønnesen 2 weeks, 6 days ago
In nested arrays don't require that the intermediate attribute
type should be a valid attribute type, it might just be zero
or an incrementing index, it is often not even used.

See include/net/netlink.h about NLA_NESTED_ARRAY:
> The difference to NLA_NESTED is the structure:
> NLA_NESTED has the nested attributes directly inside
> while an array has the nested attributes at another
> level down and the attribute types directly in the
> nesting don't matter.

Example based on include/uapi/linux/wireguard.h:
 > WGDEVICE_A_PEERS: NLA_NESTED
 >   0: NLA_NESTED
 >     WGPEER_A_PUBLIC_KEY: NLA_EXACT_LEN, len WG_KEY_LEN
 >     [..]
 >   0: NLA_NESTED
 >     ...
 >   ...

Previous the check required that the nested type was valid in
the parent attribute set, which in this case resolves to
WGDEVICE_A_UNSPEC, which is YNL_PT_REJECT, and it took the
early exit and returned YNL_PARSE_CB_ERROR.

This patch adds a new helper, ynl_attr_validate_payload(),
which we can use to validate the payload of the nested
attribute, in the context of the parents attribute type,
and it's policy, which in the above case is generated as:
[WGDEVICE_A_PEERS] = {
  .name = "peers",
  .type = YNL_PT_NEST,
  .nest = &wireguard_wgpeer_nest,
},

Some other examples are NL80211_BAND_ATTR_FREQS (nest) and
NL80211_ATTR_SUPPORTED_COMMANDS (u32).

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 tools/net/ynl/lib/ynl-priv.h     |  2 ++
 tools/net/ynl/lib/ynl.c          | 17 ++++++++++++++---
 tools/net/ynl/pyynl/ynl_gen_c.py |  2 +-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 824777d7e05e..70ea14c0a0e9 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -107,6 +107,8 @@ struct nlmsghdr *
 ynl_gemsg_start_dump(struct ynl_sock *ys, __u32 id, __u8 cmd, __u8 version);
 
 int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr);
+int ynl_attr_validate_payload(struct ynl_parse_arg *yarg,
+			      const struct nlattr *attr, unsigned int type);
 int ynl_submsg_failed(struct ynl_parse_arg *yarg, const char *field_name,
 		      const char *sel_name);
 
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 2a169c3c0797..0daf39229587 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -360,15 +360,15 @@ static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 
 /* Attribute validation */
 
-int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
+static int __ynl_attr_validate(struct ynl_parse_arg *yarg,
+			       const struct nlattr *attr, unsigned int type)
 {
 	const struct ynl_policy_attr *policy;
-	unsigned int type, len;
 	unsigned char *data;
+	unsigned int len;
 
 	data = ynl_attr_data(attr);
 	len = ynl_attr_data_len(attr);
-	type = ynl_attr_type(attr);
 	if (type > yarg->rsp_policy->max_attr) {
 		yerr(yarg->ys, YNL_ERROR_INTERNAL,
 		     "Internal error, validating unknown attribute");
@@ -450,6 +450,17 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
 	return 0;
 }
 
+int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
+{
+	return __ynl_attr_validate(yarg, attr, ynl_attr_type(attr));
+}
+
+int ynl_attr_validate_payload(struct ynl_parse_arg *yarg,
+			      const struct nlattr *attr, unsigned int type)
+{
+	return __ynl_attr_validate(yarg, attr, type);
+}
+
 int ynl_submsg_failed(struct ynl_parse_arg *yarg, const char *field_name,
 		      const char *sel_name)
 {
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index d63b63ac0b8e..ab5b8d98cbda 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -831,7 +831,7 @@ class TypeArrayNest(Type):
         local_vars = ['const struct nlattr *attr2;']
         get_lines = [f'attr_{self.c_name} = attr;',
                      'ynl_attr_for_each_nested(attr2, attr) {',
-                     '\tif (ynl_attr_validate(yarg, attr2))',
+                     '\tif (ynl_attr_validate_payload(yarg, attr2, type))',
                      '\t\treturn YNL_PARSE_CB_ERROR;',
                      f'\tn_{self.c_name}++;',
                      '}']
-- 
2.51.0

Re: [PATCH net-next v3 08/13] tools: ynl-gen: only validate nested array payload
Posted by Jakub Kicinski 2 weeks, 5 days ago
On Thu, 11 Sep 2025 20:05:01 +0000 Asbjørn Sloth Tønnesen wrote:
> +int ynl_attr_validate_payload(struct ynl_parse_arg *yarg,
> +			      const struct nlattr *attr, unsigned int type)
> +{
> +	return __ynl_attr_validate(yarg, attr, type);
> +}

Why not expose __ynl_attr_validate() to the callers?
I don't think the _payload() suffix is crystal clear, we're still
validating attr, _payload() makes it sound like we're validating
what's inside attr?
Re: [PATCH net-next v3 08/13] tools: ynl-gen: only validate nested array payload
Posted by Asbjørn Sloth Tønnesen 2 weeks, 4 days ago
On 9/13/25 12:27 AM, Jakub Kicinski wrote:
> On Thu, 11 Sep 2025 20:05:01 +0000 Asbjørn Sloth Tønnesen wrote:
>> +int ynl_attr_validate_payload(struct ynl_parse_arg *yarg,
>> +			      const struct nlattr *attr, unsigned int type)
>> +{
>> +	return __ynl_attr_validate(yarg, attr, type);
>> +}
> 
> Why not expose __ynl_attr_validate() to the callers?
> I don't think the _payload() suffix is crystal clear, we're still
> validating attr, _payload() makes it sound like we're validating
> what's inside attr?

I didn't wanna call __ynl_attr_validate() directly, as the only __ynl_*
function in ynl-priv.h is __ynl_attr_put_overflow(), and that is only
used in other static functions within that file. I agree, that _payload()
might not be the best given that we currently don't look deeper than
validating that the length a bit, so maybe _length() would have been
better.

In v4, I have changed it to just expose __ynl_attr_validate() in
ynl-priv.h, and changed ynl_attr_validate() to an inline function.