netdev - Re: [PATCH] netlink: add policy attribute range 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]

Date:   Thu, 27 Sep 2018 09:16:21 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] netlink: add policy attribute range validation

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:
> > On Wed, 2018-09-26 at 22:06 +0200, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@...el.com>
> > > 
> > > 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