netdev - Re: [PATCH net-next 5/6] netlink: add mask validation

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]

Message-ID: <c28aa386c1a998c1bc1a35580f016e129f58a5e3.camel@sipsolutions.net>
Date:   Mon, 05 Oct 2020 21:05:23 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, kernel-team@...com, jiri@...nulli.us,
        andrew@...n.ch, mkubecek@...e.cz, dsahern@...il.com,
        pablo@...filter.org
Subject: Re: [PATCH net-next 5/6] netlink: add mask validation

On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:

> We don't have good validation policy for existing unsigned int attrs
> which serve as flags (for new ones we could use NLA_BITFIELD32).
> With increased use of policy dumping having the validation be
> expressed as part of the policy is important. Add validation
> policy in form of a mask of supported/valid bits.

Nice, I'll surely use this as well somewhere :)

>  #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
> +#define NLA_ENSURE_UINT_TYPE(tp)			\
> +	(__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 ||	\
> +		      tp == NLA_U32 || tp == NLA_U64) + tp)
>  #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp)		\

nit: maybe express this (_OR_BINARY_TYPE) in terms of UINT_TYPE() ||
tp==NLA_BINARY? Doesn't matter much though.

> +static int nla_validate_mask(const struct nla_policy *pt,
> +			     const struct nlattr *nla,
> +			     struct netlink_ext_ack *extack)
> +{
> +	u64 value;
> +
> +	switch (pt->type) {
> +	case NLA_U8:
> +		value = nla_get_u8(nla);
> +		break;
> +	case NLA_U16:
> +		value = nla_get_u16(nla);
> +		break;
> +	case NLA_U32:
> +		value = nla_get_u32(nla);
> +		break;
> +	case NLA_U64:
> +		value = nla_get_u64(nla);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (value & ~(u64)pt->mask) {
> +		NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> +		return -EINVAL;

You had an export of the valid bits there in ethtool, using the cookie.
Just pointing out you lost it now. I'm not sure I like using the cookie,
that seems a bit strange, but we could easily define a different attr?

OTOH, one can always query the policy export too (which hopefully got
wired up) so it wouldn't really matter much.


Either way is fine with me on both of these points.

Reviewed-by: Johannes Berg <johannes@...solutions.net>

Thanks!

johannes