[PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[Jakub Kicinski]

To keep backward compatibility we used to leave attribute parsing
to the family if no policy is specified. This becomes tedious as
we move to more strict validation. Families must define reject all
policies if they don't want any attributes accepted.

Piggy back on the resv_start_op field as the switchover point.
AFAICT only ethtool has added new commands since the resv_start_op
was defined, and it has per-op policies so this should be a no-op.

Nonetheless the patch should still go into v6.1 for consistency.

Link: https://lore.kernel.org/all/20221019125745.3f2e7659@kernel.org/
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@nvidia.com
---
 include/net/genetlink.h | 10 +++++++++-
 net/netlink/genetlink.c | 23 +++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 3d08e67b3cfc..9f97f73615b6 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -41,13 +41,21 @@ struct genl_info;
  * @mcgrps: multicast groups used by this family
  * @n_mcgrps: number of multicast groups
  * @resv_start_op: first operation for which reserved fields of the header
- *	can be validated, new families should leave this field at zero
+ *	can be validated and policies are required (see below);
+ *	new families should leave this field at zero
  * @mcgrp_offset: starting number of multicast group IDs in this family
  *	(private)
  * @ops: the operations supported by this family
  * @n_ops: number of operations supported by this family
  * @small_ops: the small-struct operations supported by this family
  * @n_small_ops: number of small-struct operations supported by this family
+ *
+ * Attribute policies (the combination of @policy and @maxattr fields)
+ * can be attached at the family level or at the operation level.
+ * If both are present the per-operation policy takes precedence.
+ * For operations before @resv_start_op lack of policy means that the core
+ * will perform no attribute parsing or validation. For newer operations
+ * if policy is not provided core will reject all TLV attributes.
  */
 struct genl_family {
 	int			id;		/* private */
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 39b7c00e4cef..b1fd059c9992 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -78,10 +78,29 @@ static unsigned long mc_group_start = 0x3 | BIT(GENL_ID_CTRL) |
 static unsigned long *mc_groups = &mc_group_start;
 static unsigned long mc_groups_longs = 1;
 
+/* We need the last attribute with non-zero ID therefore a 2-entry array */
+static struct nla_policy genl_policy_reject_all[] = {
+	{ .type = NLA_REJECT },
+	{ .type = NLA_REJECT },
+};
+
 static int genl_ctrl_event(int event, const struct genl_family *family,
 			   const struct genl_multicast_group *grp,
 			   int grp_id);
 
+static void
+genl_op_fill_in_reject_policy(const struct genl_family *family,
+			      struct genl_ops *op)
+{
+	BUILD_BUG_ON(ARRAY_SIZE(genl_policy_reject_all) - 1 != 1);
+
+	if (op->policy || op->cmd < family->resv_start_op)
+		return;
+
+	op->policy = genl_policy_reject_all;
+	op->maxattr = 1;
+}
+
 static const struct genl_family *genl_family_find_byid(unsigned int id)
 {
 	return idr_find(&genl_fam_idr, id);
@@ -113,6 +132,8 @@ static void genl_op_from_full(const struct genl_family *family,
 		op->maxattr = family->maxattr;
 	if (!op->policy)
 		op->policy = family->policy;
+
+	genl_op_fill_in_reject_policy(family, op);
 }
 
 static int genl_get_cmd_full(u32 cmd, const struct genl_family *family,
@@ -142,6 +163,8 @@ static void genl_op_from_small(const struct genl_family *family,
 
 	op->maxattr = family->maxattr;
 	op->policy = family->policy;
+
+	genl_op_fill_in_reject_policy(family, op);
 }
 
 static int genl_get_cmd_small(u32 cmd, const struct genl_family *family,
-- 
2.37.3

Re: [PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[Johannes Berg]

On Fri, 2022-10-21 at 12:35 -0700, Jakub Kicinski wrote:
> 
> +/* We need the last attribute with non-zero ID therefore a 2-entry array */
> +static struct nla_policy genl_policy_reject_all[] = {
> +	{ .type = NLA_REJECT },
> +	{ .type = NLA_REJECT },
> +};
> +
>  static int genl_ctrl_event(int event, const struct genl_family *family,
>  			   const struct genl_multicast_group *grp,
>  			   int grp_id);
>  
> +static void
> +genl_op_fill_in_reject_policy(const struct genl_family *family,
> +			      struct genl_ops *op)
> +{
> +	BUILD_BUG_ON(ARRAY_SIZE(genl_policy_reject_all) - 1 != 1);
> +
> +	if (op->policy || op->cmd < family->resv_start_op)
> +		return;
> +
> +	op->policy = genl_policy_reject_all;
> +	op->maxattr = 1;
> +}

It feels it might've been easier to implement as simply, apart from the
doc changes:

--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -529,6 +529,10 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
 	struct nlattr **attrbuf;
 	int err;
 
+	if (ops->cmd >= family->resv_start_op && !ops->maxattr &&
+	    nlmsg_attrlen(nlh, hdrlen))
+		return ERR_PTR(-EINVAL);
+
 	if (!ops->maxattr)
 		return NULL;
 


But maybe I'm missing something in the relation with the new split ops
etc.

Also, technically, you could now have an op that is >= resv_start_op,
but sets one of GENL_DONT_VALIDATE{_DUMP,}_STRICT and then gets the old
behaviour except that attributes 0 and 1 are rejected?

Any particular reason you chose this implementation here? I can
understand having chosen it with the yaml things since then you can be
sure you're not setting GENL_DONT_VALIDATE{_DUMP,}_STRICT and you don't
have another choice anyway, but here?

Hmm.

Then again, maybe anyway we should make sure that
GENL_DONT_VALIDATE{_DUMP,}_STRICT aren't set for ops >= resv_start_op?


Anyway, for the intended use it works, and I guess it'd be a stupid
family that makes sure to set this but then still uses non-strict
validation, though I've seen people (try to) copy/paste non-strict
validation into new ops ...

johannes

Re: [PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[Jakub Kicinski]

On Fri, 21 Oct 2022 21:57:53 +0200 Johannes Berg wrote:
> It feels it might've been easier to implement as simply, apart from the
> doc changes:
> 
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -529,6 +529,10 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
>  	struct nlattr **attrbuf;
>  	int err;
>  
> +	if (ops->cmd >= family->resv_start_op && !ops->maxattr &&
> +	    nlmsg_attrlen(nlh, hdrlen))
> +		return ERR_PTR(-EINVAL);
> +
>  	if (!ops->maxattr)
>  		return NULL;
>
> But maybe I'm missing something in the relation with the new split ops
> etc.

The reason was that payload length check is... "unintrospectable"?
The reject all policy shows up in GETPOLICY. Dunno how much it matters
in practice but that was the motivation. LMK which way you prefer.

> Also, technically, you could now have an op that is >= resv_start_op,
> but sets one of GENL_DONT_VALIDATE{_DUMP,}_STRICT and then gets the old
> behaviour except that attributes 0 and 1 are rejected?
> 
> Any particular reason you chose this implementation here? I can
> understand having chosen it with the yaml things since then you can be
> sure you're not setting GENL_DONT_VALIDATE{_DUMP,}_STRICT and you don't
> have another choice anyway, but here?
> 
> Hmm.
> 
> Then again, maybe anyway we should make sure that
> GENL_DONT_VALIDATE{_DUMP,}_STRICT aren't set for ops >= resv_start_op?
> 
> 
> Anyway, for the intended use it works, and I guess it'd be a stupid
> family that makes sure to set this but then still uses non-strict
> validation, though I've seen people (try to) copy/paste non-strict
> validation into new ops ...

Hm, yeah, adding DONT*_STRICT for new commands would be pretty odd as
you say. Someone may copy & paste an existing command, tho, without
understanding what this flag does. 

I can add a check separately I reckon. It's more of a "no new command
should set this flag" thing rather than inherently related to the
reject-all policy, right?

Re: [PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[Johannes Berg]

On Fri, 2022-10-21 at 21:08 -0700, Jakub Kicinski wrote:
> On Fri, 21 Oct 2022 21:57:53 +0200 Johannes Berg wrote:
> > It feels it might've been easier to implement as simply, apart from the
> > doc changes:
> > 
> > --- a/net/netlink/genetlink.c
> > +++ b/net/netlink/genetlink.c
> > @@ -529,6 +529,10 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
> >  	struct nlattr **attrbuf;
> >  	int err;
> >  
> > +	if (ops->cmd >= family->resv_start_op && !ops->maxattr &&
> > +	    nlmsg_attrlen(nlh, hdrlen))
> > +		return ERR_PTR(-EINVAL);
> > +
> >  	if (!ops->maxattr)
> >  		return NULL;
> > 
> > But maybe I'm missing something in the relation with the new split ops
> > etc.
> 
> The reason was that payload length check is... "unintrospectable"?

Fair enough.

> The reject all policy shows up in GETPOLICY. Dunno how much it matters
> in practice but that was the motivation. LMK which way you prefer.

No I guess it's fine, it just felt a lot of overhead for what could've
been a one-line check. Having it introspectable is a nice benefit I
didn't think about :)

> > Anyway, for the intended use it works, and I guess it'd be a stupid
> > family that makes sure to set this but then still uses non-strict
> > validation, though I've seen people (try to) copy/paste non-strict
> > validation into new ops ...
> 
> Hm, yeah, adding DONT*_STRICT for new commands would be pretty odd as
> you say. Someone may copy & paste an existing command, tho, without
> understanding what this flag does. 
> 
> I can add a check separately I reckon. It's more of a "no new command
> should set this flag" thing rather than inherently related to the
> reject-all policy, right?

Yes. In fact there's also the strict_start_type in the policy[0] entry
too, which was kind of similar.

johannes

Re: [PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[Jacob Keller]

On 10/23/2022 9:19 AM, Johannes Berg wrote:
> On Fri, 2022-10-21 at 21:08 -0700, Jakub Kicinski wrote:
>> On Fri, 21 Oct 2022 21:57:53 +0200 Johannes Berg wrote:
>>> It feels it might've been easier to implement as simply, apart from the
>>> doc changes:
>>>
>>> --- a/net/netlink/genetlink.c
>>> +++ b/net/netlink/genetlink.c
>>> @@ -529,6 +529,10 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
>>>   	struct nlattr **attrbuf;
>>>   	int err;
>>>   
>>> +	if (ops->cmd >= family->resv_start_op && !ops->maxattr &&
>>> +	    nlmsg_attrlen(nlh, hdrlen))
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>>   	if (!ops->maxattr)
>>>   		return NULL;
>>>
>>> But maybe I'm missing something in the relation with the new split ops
>>> etc.
>>
>> The reason was that payload length check is... "unintrospectable"?
> 
> Fair enough.
> 
>> The reject all policy shows up in GETPOLICY. Dunno how much it matters
>> in practice but that was the motivation. LMK which way you prefer.
> 
> No I guess it's fine, it just felt a lot of overhead for what could've
> been a one-line check. Having it introspectable is a nice benefit I
> didn't think about :)
> 

I agree with reporting and making it possible to introspect. Thanks!

>>> Anyway, for the intended use it works, and I guess it'd be a stupid
>>> family that makes sure to set this but then still uses non-strict
>>> validation, though I've seen people (try to) copy/paste non-strict
>>> validation into new ops ...
>>
>> Hm, yeah, adding DONT*_STRICT for new commands would be pretty odd as
>> you say. Someone may copy & paste an existing command, tho, without
>> understanding what this flag does.
>>
>> I can add a check separately I reckon. It's more of a "no new command
>> should set this flag" thing rather than inherently related to the
>> reject-all policy, right?
> 

I agree here. Non-strict commands are very difficult if not impossible 
to extend or modify. Making it difficult to add new ones (whether by 
accident or not) is good in my book.

> Yes. In fact there's also the strict_start_type in the policy[0] entry
> too, which was kind of similar.
> 
> johannes

Re: [PATCH net] genetlink: piggy back on resv_op to default to a reject policy

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 21 Oct 2022 12:35:32 -0700 you wrote:
> To keep backward compatibility we used to leave attribute parsing
> to the family if no policy is specified. This becomes tedious as
> we move to more strict validation. Families must define reject all
> policies if they don't want any attributes accepted.
> 
> Piggy back on the resv_start_op field as the switchover point.
> AFAICT only ethtool has added new commands since the resv_start_op
> was defined, and it has per-op policies so this should be a no-op.
> 
> [...]

Here is the summary with links:
  - [net] genetlink: piggy back on resv_op to default to a reject policy
    https://git.kernel.org/netdev/net/c/4fa86555d1cd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html