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

[Jakub Kicinski <kuba@kernel.org>]

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.

> This needs to be done differently.
> 
> One API "hole" that I see is that payload with size < struct dcb_app
> gets bounced.
> 
> We can pack the new stuff into a smaller payload. The inner attribute
> would not be DCB_ATTR_DCB_APP, but say DCB_ATTR_DCB_PCP, which would
> imply the selector. The payload can be struct { u8 prio; u16 proto; }.
> This would have been bounced by the old UAPI, so we know no userspace
> makes use of that.
> 
> We can treat the output similarly.