Three follow-up patches for recent strict netlink validation series.
Patch 1 fixes dump handling for genetlink families which validate and parse
messages themselves (e.g. because they need different policies for diferent
commands).
Patch 2 sets bad_attr in extack in one place where this was omitted.
Patch 3 adds new NL_VALIDATE_NESTED flags for strict validation to enable
checking that NLA_F_NESTED value in received messages matches expectations
and includes this flag in NL_VALIDATE_STRICT. This would change userspace
visible behavior but the previous switching to NL_VALIDATE_STRICT for new
code is still only in net-next at the moment.
Michal Kubecek (3):
genetlink: do not validate dump requests if there is no policy
netlink: set bad attribute also on maxtype check
netlink: add validation of NLA_F_NESTED flag
include/net/netlink.h | 10 +++++++++-
lib/nlattr.c | 18 +++++++++++++++++-
net/netlink/genetlink.c | 24 ++++++++++++++----------
3 files changed, 40 insertions(+), 12 deletions(-)
--
2.21.0
Add new validation flag NL_VALIDATE_NESTED which adds three consistency
checks of NLA_F_NESTED_FLAG:
- the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
- the flag is not set on attributes with other policies except NLA_UNSPEC
- the flag is set on attribute passed to nla_parse_nested()
Signed-off-by: Michal Kubecek <[email protected]>
---
include/net/netlink.h | 10 +++++++++-
lib/nlattr.c | 15 +++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 679f649748d4..55f68e00fb6e 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -401,6 +401,8 @@ struct nl_info {
* are enforced going forward.
* @NL_VALIDATE_STRICT_ATTRS: strict attribute policy parsing (e.g.
* U8, U16, U32 must have exact size, etc.)
+ * @NL_VALIDATE_NESTED: Check that NLA_F_NESTED is set for NLA_NESTED(_ARRAY)
+ * and unset for other policies.
*/
enum netlink_validation {
NL_VALIDATE_LIBERAL = 0,
@@ -408,6 +410,7 @@ enum netlink_validation {
NL_VALIDATE_MAXTYPE = BIT(1),
NL_VALIDATE_UNSPEC = BIT(2),
NL_VALIDATE_STRICT_ATTRS = BIT(3),
+ NL_VALIDATE_NESTED = BIT(4),
};
#define NL_VALIDATE_DEPRECATED_STRICT (NL_VALIDATE_TRAILING |\
@@ -415,7 +418,8 @@ enum netlink_validation {
#define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
NL_VALIDATE_MAXTYPE |\
NL_VALIDATE_UNSPEC |\
- NL_VALIDATE_STRICT_ATTRS)
+ NL_VALIDATE_STRICT_ATTRS |\
+ NL_VALIDATE_NESTED)
int netlink_rcv_skb(struct sk_buff *skb,
int (*cb)(struct sk_buff *, struct nlmsghdr *,
@@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack)
{
+ if (!(nla->nla_type & NLA_F_NESTED)) {
+ NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
+ return -EINVAL;
+ }
return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
NL_VALIDATE_STRICT, extack);
}
diff --git a/lib/nlattr.c b/lib/nlattr.c
index adc919b32bf9..92da65cb6637 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
}
}
+ if (validate & NL_VALIDATE_NESTED) {
+ if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
+ !(nla->nla_type & NLA_F_NESTED)) {
+ NL_SET_ERR_MSG_ATTR(extack, nla,
+ "nested attribute expected");
+ return -EINVAL;
+ }
+ if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
+ pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
+ NL_SET_ERR_MSG_ATTR(extack, nla,
+ "nested attribute not expected");
+ return -EINVAL;
+ }
+ }
+
switch (pt->type) {
case NLA_EXACT_LEN:
if (attrlen != pt->len)
--
2.21.0
On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> Add new validation flag NL_VALIDATE_NESTED which adds three consistency
> checks of NLA_F_NESTED_FLAG:
>
> - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
> - the flag is not set on attributes with other policies except NLA_UNSPEC
> - the flag is set on attribute passed to nla_parse_nested()
Looks good to me!
> @@ -415,7 +418,8 @@ enum netlink_validation {
> #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
> NL_VALIDATE_MAXTYPE |\
> NL_VALIDATE_UNSPEC |\
> - NL_VALIDATE_STRICT_ATTRS)
> + NL_VALIDATE_STRICT_ATTRS |\
> + NL_VALIDATE_NESTED)
This is fine _right now_, but in general we cannot keep adding here
after the next release :-)
> int netlink_rcv_skb(struct sk_buff *skb,
> int (*cb)(struct sk_buff *, struct nlmsghdr *,
> @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
> const struct nla_policy *policy,
> struct netlink_ext_ack *extack)
> {
> + if (!(nla->nla_type & NLA_F_NESTED)) {
> + NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
attribute expected" could result in a lot of headscratching (without
looking at the code) because it looks nested if you do nla_nest_start()
etc.
> + return -EINVAL;
> + }
> return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
> NL_VALIDATE_STRICT, extack);
I'd probably put a blank line there but ymmv.
> }
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index adc919b32bf9..92da65cb6637 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> }
> }
>
> + if (validate & NL_VALIDATE_NESTED) {
> + if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
> + !(nla->nla_type & NLA_F_NESTED)) {
> + NL_SET_ERR_MSG_ATTR(extack, nla,
> + "nested attribute expected");
> + return -EINVAL;
> + }
> + if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
> + pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
> + NL_SET_ERR_MSG_ATTR(extack, nla,
> + "nested attribute not expected");
> + return -EINVAL;
Same comment here wrt. the messages, I think they should more explicitly
refer to the flag.
johannes
(PS: if you CC me on this address I generally can respond quicker)
On Thu, May 02, 2019 at 02:54:56PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > Add new validation flag NL_VALIDATE_NESTED which adds three consistency
> > checks of NLA_F_NESTED_FLAG:
> >
> > - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
> > - the flag is not set on attributes with other policies except NLA_UNSPEC
> > - the flag is set on attribute passed to nla_parse_nested()
>
> Looks good to me!
>
> > @@ -415,7 +418,8 @@ enum netlink_validation {
> > #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
> > NL_VALIDATE_MAXTYPE |\
> > NL_VALIDATE_UNSPEC |\
> > - NL_VALIDATE_STRICT_ATTRS)
> > + NL_VALIDATE_STRICT_ATTRS |\
> > + NL_VALIDATE_NESTED)
>
> This is fine _right now_, but in general we cannot keep adding here
> after the next release :-)
Right, that's why I would like to get this into the same cycle as your
series.
> > int netlink_rcv_skb(struct sk_buff *skb,
> > int (*cb)(struct sk_buff *, struct nlmsghdr *,
> > @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
> > const struct nla_policy *policy,
> > struct netlink_ext_ack *extack)
> > {
> > + if (!(nla->nla_type & NLA_F_NESTED)) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
>
> Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
> attribute expected" could result in a lot of headscratching (without
> looking at the code) because it looks nested if you do nla_nest_start()
> etc.
How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?
>
> > + return -EINVAL;
> > + }
> > return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
> > NL_VALIDATE_STRICT, extack);
>
> I'd probably put a blank line there but ymmv.
OK
> > }
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index adc919b32bf9..92da65cb6637 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> > }
> > }
> >
> > + if (validate & NL_VALIDATE_NESTED) {
> > + if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
> > + !(nla->nla_type & NLA_F_NESTED)) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla,
> > + "nested attribute expected");
> > + return -EINVAL;
> > + }
> > + if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
> > + pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla,
> > + "nested attribute not expected");
> > + return -EINVAL;
>
> Same comment here wrt. the messages, I think they should more explicitly
> refer to the flag.
>
> johannes
>
> (PS: if you CC me on this address I generally can respond quicker)
I'll try to keep that in mind.
Michal
On 5/2/19 7:14 AM, Michal Kubecek wrote:
>>> @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
>>> const struct nla_policy *policy,
>>> struct netlink_ext_ack *extack)
>>> {
>>> + if (!(nla->nla_type & NLA_F_NESTED)) {
>>> + NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
>>
>> Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
>> attribute expected" could result in a lot of headscratching (without
>> looking at the code) because it looks nested if you do nla_nest_start()
>> etc.
>
> How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?
>
That is much better to me.
On Thu, 2019-05-02 at 15:14 +0200, Michal Kubecek wrote:
>
> > > @@ -415,7 +418,8 @@ enum netlink_validation {
> > > #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
> > > NL_VALIDATE_MAXTYPE |\
> > > NL_VALIDATE_UNSPEC |\
> > > - NL_VALIDATE_STRICT_ATTRS)
> > > + NL_VALIDATE_STRICT_ATTRS |\
> > > + NL_VALIDATE_NESTED)
> >
> > This is fine _right now_, but in general we cannot keep adding here
> > after the next release :-)
>
> Right, that's why I would like to get this into the same cycle as your
> series.
Yeah, I know you know, just wanted state it again :-)
> How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?
Looks good to me.
johannes