Re: [PATCH net-next v2 1/6] net: dcb: add new pcp selector to app object

[Petr Machata]

Jakub Kicinski <kuba@xxxxxxxxxx> writes:

> On Fri, 30 Sep 2022 14:20:50 +0200 Petr Machata wrote:
>> > @@ -1495,7 +1536,7 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
>> >  		nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
>> >  			struct dcb_app *app_data;
>> >
>> > -			if (nla_type(attr) != DCB_ATTR_IEEE_APP)
>> > +			if (!dcbnl_app_attr_type_validate(nla_type(attr)))  
>> 
>> Oh no! It wasn't validating the DCB_ATTR_IEEE_APP_TABLE nest against a
>> policy! Instead it was just skipping whatever is not DCB_ATTR_IEEE_APP.
>> 
>> So userspace was permitted to shove random crap down here, and it would
>> just quietly be ignored. We can't start reinterpreting some of that crap
>> as information. We also can't start bouncing it.
>
> Are you saying that we can't start interpreting new attr types?
>
> "Traditionally" netlink ignored new attr types so from that perspective
> starting to interpret new types is pretty "run of the mill" for netlink.
> IOW *_deprecated() parsing routines do not use NL_VALIDATE_MAXTYPE.
>
> That does put netlink in a bit of a special category when it comes to
> input validation, but really putting in a random but valid attr is much
> harder than not initializing a struct member. Is there user space which
> does that?
>
> Sorry if I'm misinterpreting the situation.

I assumed the policy is much more strict with changes like this. If you
think it's OK, I'm fine with it as well.

The userspace (lldpad in particular) is doing the opposite thing BTW:
assuming everything in the nest is a DCB_ATTR_IEEE_APP. When we start
emitting the new attribute, it will get confused.