Re: [PATCH] netlink: add policy attribute range validation

[Johannes Berg <johannes@xxxxxxxxxxxxxxxx>]

On Thu, 2018-09-27 at 09:16 +0200, Michal Kubecek wrote:

> The overloading still feels a bit complicated. Perhaps we could rather
> use validation_data in the natural way, i.e. as a pointer to validation
> data. That would be a struct (maybe array) of two values of the
> corresponding type. It would mean a bit more data and a bit more writing
> but it would be IMHO more straightforward.

I considered that, but I didn't really like it either. The memory
wasting isn't *that* bad (even if we go to s64 that'd only be ~20x16
bytes for nl80211, eating up 320 out of the 550 saved, but still); I'm
more worried about making this really hard to actually *do*.

Consider

policy[] = {
	...
	[NL80211_ATTR_WIPHY_RETRY_SHORT] =
		NLA_POLICY_RANGE(NLA_U8, 1, 255),
	...
};

vs.

static const struct netlink_policy_range retry_range = {
	.min = 1,
	.max = 255,
};

policy[] = {
	...
	[NL80211_ATTR_WIPHY_RETRY_SHORT] = {
		.type = NLA_U8,
		.validation_data = &retry_range,
	},
	...
};


That's significantly more to type, to the point where I'd seriously
consider doing this only for attributes that are used and checked in
many places - it doesn't feel like a big win over manual range-checking.

But I want it to be a win over manual range-checking so it gets used
more because it's more efficient, less prone to getting messed up if
multiple places use the same attribute and validates attributes even if
they're ignored by an operation.


I'd also say that we're certainly no strangers to union/overloading, so
I don't feel like this is a big argument. One doesn't even really have
to be *aware* of it for the most part: if it were a struct instead of a
union, it'd actually have the same effect since the .type field
indicates which part gets used. That it's overloaded in a union is
basically just a space saving measure, I don't think it makes the
reasoning much more complex?


That said, given that I also later sent that RFC patch for a further
validation function pointer (which is useful e.g. for ensuring certain
binary attributes are well-formed), I think it might in fact be better
to split .type field (it really only needs to be u8, we have ~20) and
adding a "validation" enum to the policy:

enum netlink_policy_validation {
	/* default */
	NLA_VALIDATE_NONE,

	/* valid for NLA_{U,S}{8,16,32,64} */
	NLA_VALIDATE_MIN,
	NLA_VALIDATE_MAX,
	NLA_VALIDATE_RANGE,

	/* valid for any type other than NLA_BITFIELD32/NLA_REJECT */
	NLA_VALIDATE_FUNCTION,
};

Combining that with macros like the ones I wrote in my previous message
in this thread:

#define __NLA_ENSURE(condition) (sizeof(char[1 - 2*!(condition)]) - 1)
#define NLA_ENSURE_INT_TYPE(tp)                         \
        (__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 ||   \
                      tp == NLA_S16 || tp == NLA_U16 || \
                      tp == NLA_S32 || tp == NLA_U32 || \
                      tp == NLA_S64 || tp == NLA_U64) + tp)
#define NLA_ENSURE_NO_VALIDATION_PTR(tp)                \
        (__NLA_ENSURE(tp != NLA_BITFIELD32 &&           \
                      tp != NLA_REJECT &&               \
                      tp != NLA_NESTED &&               \
                      tp != NLA_NESTED_ARRAY) + tp)

#define NLA_POLICY_RANGE(tp, _min, _max) {
        .type = NLA_ENSURE_INT_TYPE(tp),
	.validate_type = NLA_VALIDATE_RANGE,
        .min = _min,
        .max = _max,
}

#define NLA_POLICY_MIN(tp, _min) {
        .type = NLA_ENSURE_INT_TYPE(tp),
	.validate_type = NLA_VALIDATE_MIN,
        .min = _min,
}

#define NLA_POLICY_MAX(tp, _max) {
        .type = NLA_ENSURE_INT_TYPE(tp),
	.validate_type = NLA_VALIDATE_MAX,
        .max = _max,
}

#define NLA_POLICY_FN(tp, fn) {
	.type = NLA_ENSURE_NO_VALIDATION_PTR(tp),
	.validate_type = NLA_VALIDATE_FUNCTION,
	.validate = fn,
}

This would even give us the flexibility to extend the validation type
further in the future, to actually have what you suggested where the
validation_data is a pointer and the valid range is given in a struct
pointed to, to allow larger ranges than s16.

johannes