netdev - Re: Genetlink per cmd policies

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]

Date:   Wed, 30 Sep 2020 22:13:07 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Jiri Pirko <jiri@...nulli.us>, Michal Kubecek <mkubecek@...e.cz>,
        dsahern@...nel.org, pablo@...filter.org, netdev@...r.kernel.org
Subject: Re: Genetlink per cmd policies

On Wed, 2020-09-30 at 12:46 -0700, Jakub Kicinski wrote:

> This builds (I think) - around 100 extra LoC:

Looks good to me, couple of comments below.

> +/**
> + * struct genl_light_ops - generic netlink operations (small version)
> + * @cmd: command identifier
> + * @internal_flags: flags used by the family
> + * @flags: flags
> + * @validate: validation flags from enum genl_validate_flags
> + * @doit: standard command callback
> + * @dumpit: callback for dumpers
> + *
> + * This is a cut-down version of struct genl_ops for users who don't need
> + * most of the ancillary infra and want to save space.
> + */
> +struct genl_light_ops {
> +	int	(*doit)(struct sk_buff *skb, struct genl_info *info);
> +	int	(*dumpit)(struct sk_buff *skb, struct netlink_callback *cb);

Even dumpit is pretty rare (e.g. 10 out of 107 in nl80211) - maybe
remove that even? It's a bit more juggling in nl80211 to actually use
it, but I'm certainly happy to do that myself.

> +static void genl_op_from_full(const struct genl_family *family,
> +			      unsigned int i, struct genl_ops *op)
> +{
> +	memcpy(op, &family->ops[i], sizeof(*op));

What's wrong with struct assignment? :)

	*op = family->ops[i];


> +	if (!op->maxattr)
> +		op->maxattr = family->maxattr;
> +	if (!op->policy)
> +		op->policy = family->policy;

That doesn't build as is, I think? Or did you have some other patch
below it?

>  static int genl_validate_ops(const struct genl_family *family)
>  {
[...]
> +	n_ops = genl_get_cmd_cnt(family);
>  	if (!n_ops)
>  		return 0;

Come to think of it, that check is kinda pointless, the loop won't run
if it's 0 and then we return 0 immediately anyway... whatever :)

>  	for (i = 0; i < n_ops; i++) {
> -		if (ops[i].dumpit == NULL && ops[i].doit == NULL)
> +		struct genl_ops op;
> +
> +		if (genl_get_cmd_by_index(i, family, &op))
>  			return -EINVAL;

Maybe WARN_ON() or something? It really ought to not be possible for
that to fail, since you're only iterating to n_ops, so you'd have to
have some consistency issues if that happens.

> -		for (j = i + 1; j < n_ops; j++)
> -			if (ops[i].cmd == ops[j].cmd)
> +		if (op.dumpit == NULL && op.doit == NULL)
> +			return -EINVAL;
> +		for (j = i + 1; j < n_ops; j++) {
> +			struct genl_ops op2;
> +
> +			if (genl_get_cmd_by_index(j, family, &op2))
>  				return -EINVAL;

same here

> +		for (i = 0; i < genl_get_cmd_cnt(family); i++) {
>  			struct nlattr *nest;
> -			const struct genl_ops *ops = &family->ops[i];
> -			u32 op_flags = ops->flags;
> +			struct genl_ops op;
> +			u32 op_flags;
> +
> +			if (genl_get_cmd_by_index(i, family, &op))
> +				goto nla_put_failure;

but actually, same here, so maybe it should just not even be able to
return an error but WARN_ON instead and clear the op, so you have
everything NULL in that case?

I don't really see a case where you'd have the index coming from
userspace and would have to protect against it being bad, or something?

johannes