Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <faf336e3ba515d41211910f3d8d207e693434cb9.camel@sipsolutions.net>
Subject: Re: [PATCH 4/7] netlink: extend policy range validation
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     netdev@vger.kernel.org, Antonio Quartulli <ordex@autistici.org>,
        linux-wireless@vger.kernel.org
Date:   Wed, 29 Apr 2020 21:21:15 +0200
In-Reply-To: <20200429111034.71ab2443@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
References: <20200429134843.42224-1-johannes@sipsolutions.net>
         <20200429154836.b86f45043a5e.I7b46d9c85e4d7a99c0b5e0c2f54bb89b5750e6dc@changeid>
         <20200429111034.71ab2443@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 (3.34.4-1.fc31) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Wed, 2020-04-29 at 11:10 -0700, Jakub Kicinski wrote:

> > +static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
> > +					   const struct nlattr *nla,
> > +					   struct netlink_ext_ack *extack)
> >  {
> > -	bool validate_min, validate_max;
> > -	s64 value;
> > +	struct netlink_range_validation _range = {
> > +		.min = 0,
> > +		.max = U64_MAX,
> > +	}, *range = &_range;
> > +	u64 value;
> >  
> > -	validate_min = pt->validation_type == NLA_VALIDATE_RANGE ||
> > -		       pt->validation_type == NLA_VALIDATE_MIN;
> > -	validate_max = pt->validation_type == NLA_VALIDATE_RANGE ||
> > -		       pt->validation_type == NLA_VALIDATE_MAX;
> > +	WARN_ON_ONCE(pt->min < 0 || pt->max < 0);
> 
> I'm probably missing something, but in case of NLA_VALIDATE_RANGE_PTR
> aren't min and max invalid (union has the range pointer set, so this
> will read 2 bytes of the pointer).

No, you're right of course. It's reading 4 bytes, actually, they're both
s16. Which I did because that's the maximum range that doesn't increase
the size on 32-bit.

I could move it into the switch, but, hm.. the unused ones (min/max if
only one is used) should be 0, so I guess just

	WARN_ON_ONCE(pt->validation_type != NLA_VALIDATE_RANGE_PTR &&
                     (pt->min < 0 || pt->max < 0));

will be fine.

Thanks!

johannes