Re: [PATCH 0/6] stricter netlink validation

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2019-04-05 at 13:47 +0200, Johannes Berg wrote:
> 
> I've also pushed some very much WIP code to the netlink-policy-export
> branch there that exposes the policies to userspace, there at least for
> generic netlink now.

Seems to more or less work now, userspace gets things like (for
nl80211):

(ID 0x18 is the nl80211 genl family)

	ID: 0x18  policy[0]:attr[1]: type=U32
[...]
	ID: 0x18  policy[0]:attr[87]: type=U32
	ID: 0x18  policy[0]:attr[88]: type=U64
	ID: 0x18  policy[0]:attr[89]: type=U8
	ID: 0x18  policy[0]:attr[90]: type=NESTED
	ID: 0x18  policy[0]:attr[91]: type=BINARY
[...]
	ID: 0x18  policy[0]:attr[270]: type=NESTED policy:1
[...]
	ID: 0x18  policy[0]:attr[273]: type=NESTED policy:2
[...]
	ID: 0x18  policy[1]:attr[1]: type=FLAG
	ID: 0x18  policy[1]:attr[2]: type=BINARY
	ID: 0x18  policy[1]:attr[3]: type=BINARY
[...]
	ID: 0x18  policy[2]:attr[1]: type=REJECT
	ID: 0x18  policy[2]:attr[2]: type=REJECT
	ID: 0x18  policy[2]:attr[3]: type=REJECT
	ID: 0x18  policy[2]:attr[4]: type=REJECT
	ID: 0x18  policy[2]:attr[5]: type=NESTED_ARRAY policy:3
[...]
	ID: 0x18  policy[3]:attr[3]: type=NESTED policy:4

etc.

See net/wireless/nl80211.c nl80211_policy[] for the original data, it's
unchanged over current net-next.


Policy 0 is - by convention - the top-level policy, but once I fix the
recursion issue in validate_nla() it's possible that a nested attribute
refers back to the top-level policy.

There are some bugs, like it generating an almost-empty message for when
the type is NLA_UNSPEC rather than eliding it entirely, and I haven't
implemented a bunch of things yet:

                /* TODO advertise range (min/max) */
                /* TODO advertise min/max len */
                /* TODO show reject string if any */

Also, I haven't hooked it up to anything that's not generic netlink, but
the API should be general enough for anyone:

int netlink_policy_dump_start(const struct nla_policy *policy,
                              unsigned int maxtype,
                              unsigned long *state);
bool netlink_policy_dump_loop(unsigned long *state);
int netlink_policy_dump_write(struct sk_buff *skb, unsigned long state);

(*state/state is &cb->args[n]/cb->args[n] for the netlink dump, it will
generate one message per type. That may be overkill, but it lets us
include the potentially long reject string etc. without worrying about
any message size limitations.)

It feels like it's working, and so I'd like to propose formal patches
soon.

Pablo, what do you think? It seems to me that this type of thing would
address most if not all what you did with the object/bus description
stuff, while not writing any new code, the info is taken straight from
the policy.

johannes