linux-kernel - Re: [PATCH net-next 06/11] tools: ynl-gen: don't validate nested array attribute types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250906122938.30566de3@kernel.org>
Date: Sat, 6 Sep 2025 12:29:38 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Asbjørn Sloth Tønnesen
 <ast@...erby.net>
Cc: "Jason A. Donenfeld" <Jason@...c4.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>, Simon Horman
 <horms@...nel.org>, Jacob Keller <jacob.e.keller@...el.com>, Andrew Lunn
 <andrew+netdev@...n.ch>, wireguard@...ts.zx2c4.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 06/11] tools: ynl-gen: don't validate nested
 array attribute types

On Sat, 6 Sep 2025 13:22:01 +0000 Asbjørn Sloth Tønnesen wrote:
> > I don't understand, please provide more details.
> > This is an ArrayNest, right?
> > 
> > [ARRAY-ATTR]
> >    [ENTRY]
> >      [MEMBER1]
> >      [MEMBER2]
> >    [ENTRY]
> >      [MEMBER1]
> >      [MEMBER2]
> > 
> > Which level are you saying doesn't matter?
> > If entry is a nest it must be a valid nest.
> > What the comment you're quoting is saying is that the nla_type of ENTRY
> > doesn't matter.  
> 
> I will expand this in v2, but the gist of it is that this is part of the
> "split attribute counting, and later allocating an array to hold them" code.
> 
> The check that I remove for nested arrays, is an early exit during the
> counting phase. Later in the allocation and parse phase it validates the
> nested payload.
> 
> In 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
>  >     ...
>  >   ...  
> 
> The current check requires that the nested type is valid in the nested
> attribute set, which in this case resolves to WGDEVICE_A_UNSPEC, which is
> YNL_PT_REJECT, and it takes the early exit and returns YNL_PARSE_CB_ERROR.

I see your point now. We're validating ENTRY as an attribute in the
parent attribute set, but it's just a meaningless id.

I think we need more fixing here. The real parsing loop will only
validate what's _inside_ the [MEMBER]. Which doesn't matter all
that much to nests, but look at what happens if subtype is a scalar.
We'll just call ynl_attr_get_u32(), type is never really validate.

I think we need this, and make the codegen feed in the ARRAY-ATTR type
to validate ENTRY?

diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 2a169c3c0797..e43167398c69 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)
+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,11 @@ 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_submsg_failed(struct ynl_parse_arg *yarg, const char *field_name,
 		      const char *sel_name)
 {