On Wed, Sep 26, 2018 at 10:35:27PM +0200, Johannes Berg wrote:

On Wed, 2018-09-26 at 22:17 +0200, Johannes Berg wrote:

quoted

On Wed, 2018-09-26 at 22:06 +0200, Johannes Berg wrote:

quoted

From: Johannes Berg <redacted>

Without further bloating the policy structs, we can overload
the `validation_data' pointer with a struct of s16 min, max
and use those to validate ranges in NLA_{U,S}{8,16,32,64}
attributes.

It may sound strange to validate NLA_U32 with a s16 max, but
in many cases NLA_U32 is used for enums etc. since there's no
size benefit in using a smaller attribute width anyway, due
to netlink attribute alignment; in cases like that it's still
useful, particularly when the attribute really transports an
enum value.

That said, I did find a few places where we could benefit from a larger
type here - e.g. having a NLA_U16 that must be non-zero cannot be
represented in the policy as is, since you can't set max to 65535.

We could also fix that, btw, by taking two bits out of the "type" field,
and letting those indicate "check_min" and "check_max". That would also
fix the other thing I noted regarding the union, I suppose.

I didn't really like that too much because it makes the whole thing far
more complex, but perhaps if we hide it behind macros like

#define NLA_POLICY_RANGE(tp, _min, _max) {
	.type = tp,
	.min = _min, .check_min = 1,
	.max = _max, .check_max = 1,
}

#define NLA_POLICY_MIN(tp, _min) {
	.type = tp,
	.min = _min, .check_min = 1,
}

#define NLA_POLICY_MAX(tp, _max) {
	.type = tp,
	.max = _max, .check_max = 1,
}

it becomes more palatable?

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.

Michal Kubecek