From: Johannes Berg <[email protected]>
In some situations some netlink attributes may be used for output
only (kernel->userspace) or may be reserved for future use. It's
then helpful to be able to prevent userspace from using them in
messages sent to the kernel, since they'd otherwise be ignored and
any future will become impossible if this happens.
Add NLA_REJECT to the policy which does nothing but reject (with
EINVAL) validation of any messages containing this attribute.
Allow for returning a specific extended ACK error message in the
validation_data pointer.
While at it fix the indentation of NLA_BITFIELD32 and describe the
validation_data pointer for it.
The specific case I have in mind now is a shared nested attribute
containing request/response data, and it would be pointless and
potentially confusing to have userspace include response data in
the messages that actually contain a request.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/netlink.h | 6 +++++-
lib/nlattr.c | 12 +++++++++---
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..04e40fcc70d6 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -180,6 +180,7 @@ enum {
NLA_S32,
NLA_S64,
NLA_BITFIELD32,
+ NLA_REJECT,
__NLA_TYPE_MAX,
};
@@ -208,7 +209,10 @@ enum {
* NLA_MSECS Leaving the length field zero will verify the
* given type fits, using it verifies minimum length
* just like "All other"
- * NLA_BITFIELD32 A 32-bit bitmap/bitselector attribute
+ * NLA_BITFIELD32 A 32-bit bitmap/bitselector attribute, validation
+ * data must point to a u32 value of valid flags
+ * NLA_REJECT Reject this attribute, validation data may point
+ * to a string to report as the error in extended ACK.
* All other Minimum length of attribute payload
*
* Example:
diff --git a/lib/nlattr.c b/lib/nlattr.c
index e335bcafa9e4..9ec0cc151148 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
}
static int validate_nla(const struct nlattr *nla, int maxtype,
- const struct nla_policy *policy)
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
{
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
@@ -87,6 +88,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
}
switch (pt->type) {
+ case NLA_REJECT:
+ if (pt->validation_data && extack)
+ extack->_msg = pt->validation_data;
+ return -EINVAL;
+
case NLA_FLAG:
if (attrlen > 0)
return -ERANGE;
@@ -180,7 +186,7 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
int rem;
nla_for_each_attr(nla, head, len, rem) {
- int err = validate_nla(nla, maxtype, policy);
+ int err = validate_nla(nla, maxtype, policy, extack);
if (err < 0) {
if (extack)
@@ -251,7 +257,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
if (type > 0 && type <= maxtype) {
if (policy) {
- err = validate_nla(nla, maxtype, policy);
+ err = validate_nla(nla, maxtype, policy, extack);
if (err < 0) {
NL_SET_ERR_MSG_ATTR(extack, nla,
"Attribute failed policy validation");
--
2.14.4
On Wed, 2018-09-12 at 21:29 +0200, Michal Kubecek wrote:
> > 3) eventually replace nlmsg_parse() calls by nlmsg_parse_strict() and
> > see what breaks? :-) We won't be able to rely on that any time soon
> > though (unless userspace first checks with a guaranteed rejected
> > attribute, e.g. one that has NLA_REJECT, perhaps the u64 pad
> > attributes could be marked such since the kernel can't assume
> > alignment anyway)
>
> I'm not so sure we (eventually) want to reject unknown attributes
> everywhere. I don't have any concrete example in mind but I think there
> will be use cases where we want to ignore unrecognized attributes
> (probably per parse call). But it makes sense to require caller to
> explicitely declare this is the case.
Yeah, I think nla_parse() vs. nla_parse_strict() - along with
remembering in review to say "perhaps you should prefer
nla_parse_strict() for this new thing" might be all we want (or
realistically can do).
> > While we're talking about wishlist, I'm also toying with the idea of
> > having some sort of generic mechanism to convert netlink attributes
> > to/from structs, for internal kernel representation; so far though I
> > haven't been able to come up with anything useful.
>
> I was also thinking about something like this. One motivation was to
> design extensible version of ethtool_ops, the other was allowing complex
> data types (structures, arrays) for ethtool tunables. But I have nothing
> more than a vague idea so far.
I played with macros a bit, but ultimately that wasn't so readable.
There's no way to do upper-casing in the preprocessor :-) Realistically,
I ended up with something that would require either a lot of boiler-
plate code estill, or perhaps double-including a header file (though I
never really finished the latter thought.)
This is what I toyed with earlier today:
https://p.sipsolutions.net/35e260d7debaa406.txt
johannes
On 9/12/2018 10:36 AM, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> Commonly, ethernet addresses are just using a policy of
> { .len = ETH_ALEN }
> which leaves userspace free to send more data than it should,
> which may hide bugs.
>
> Introduce NLA_ETH_ADDR which checks for exact size, and rejects
> the attribute if the length isn't ETH_ALEN.
>
> Also add NLA_ETH_ADDR_COMPAT which can be used in place of the
> policy above, but will, in addition, warn on an address that's
> too long.
Not sure if this is correctly described here. It seems longer addresses
are not rejected, but only result in a warning message. I guess the
problem is in the reference to the "policy above" ;-)
Regards,
Arend
On Wed, 2018-09-12 at 11:15 -0700, David Miller wrote:
> This looks great, no objections to this idea or the facility.
Great. I'll post this (with the fixups) for real tomorrow then, I guess.
A bit too late for me to do now.
> It does, however, remind me about about the classic problem of how bad
> we are at feature support detection because unrecognized attributes are
> ignored.
>
> I do really hope we can fully solve that problem some day.
Yes.
There may be two or more levels to this.
It wouldn't be hard to reject attributes that are higher than maxtype -
we already pass that to nla_parse() wherever we call it, but we'd have
to find a way to make it optional I guess, for compatibility reasons.
Perhaps with a warning, like attribute validation. For genetlink, a flag
in the family (something like "strict attribute validation") would be
easy, but for "netlink proper" we have a lot of nlmsg_parse() calls to
patch, and/or replace by nlmsg_parse_strict().
I guess we should
1) implement nlmsg_parse_strict() for those new things that want it
strictly - greenfield type stuff that doesn't need to work with
existing applications
2) add a warning to nlmsg_parse() when a too high attribute is
encountered
3) eventually replace nlmsg_parse() calls by nlmsg_parse_strict() and
see what breaks? :-) We won't be able to rely on that any time soon
though (unless userspace first checks with a guaranteed rejected
attribute, e.g. one that has NLA_REJECT, perhaps the u64 pad
attributes could be marked such since the kernel can't assume
alignment anyway)
Perhaps we also have too many calls to nlmsg_parse() without a policy,
but that's orthogonal to this check.
On a second level though, with complex things like nl80211 it's often
not clear at all which attributes are used with which commands. Some
attributes (like NL80211_ATTR_IFINDEX) are (almost) universal, but there
are others that aren't. Perhaps this isn't all that important, since if
you try to trigger scanning and at the same time tell the kernel about a
key index, that clearly makes no sense at all. OTOH, we have no good way
of discovering what attribute is used where - we (try to) document this
well in the nl80211.h kernel-doc, but that isn't always complete.
So more introspection (of sorts) could be useful.
While we're talking about wishlist, I'm also toying with the idea of
having some sort of generic mechanism to convert netlink attributes
to/from structs, for internal kernel representation; so far though I
haven't been able to come up with anything useful.
johannes
On Wed, 2018-09-12 at 10:36 +0200, Johannes Berg wrote:
> @@ -251,7 +257,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
>
> if (type > 0 && type <= maxtype) {
> if (policy) {
> - err = validate_nla(nla, maxtype, policy);
> + err = validate_nla(nla, maxtype, policy, extack);
> if (err < 0) {
> NL_SET_ERR_MSG_ATTR(extack, nla,
> "Attribute failed policy validation");
Err... I should read my patches before sending them :-)
Clearly, this NL_SET_ERR_MSG_ATTR() overwrites the error message, so
would have to be made conditional.
I can fix that (and I should use/test it) if we decide it's worthwhile.
johannes
On Wed, Sep 12, 2018 at 08:34:45PM +0200, Johannes Berg wrote:
> It wouldn't be hard to reject attributes that are higher than maxtype -
> we already pass that to nla_parse() wherever we call it, but we'd have
> to find a way to make it optional I guess, for compatibility reasons.
> Perhaps with a warning, like attribute validation. For genetlink, a flag
> in the family (something like "strict attribute validation") would be
> easy, but for "netlink proper" we have a lot of nlmsg_parse() calls to
> patch, and/or replace by nlmsg_parse_strict().
>
> I guess we should
>
> 1) implement nlmsg_parse_strict() for those new things that want it
> strictly - greenfield type stuff that doesn't need to work with
> existing applications
>
> 2) add a warning to nlmsg_parse() when a too high attribute is
> encountered
>
> 3) eventually replace nlmsg_parse() calls by nlmsg_parse_strict() and
> see what breaks? :-) We won't be able to rely on that any time soon
> though (unless userspace first checks with a guaranteed rejected
> attribute, e.g. one that has NLA_REJECT, perhaps the u64 pad
> attributes could be marked such since the kernel can't assume
> alignment anyway)
I'm not so sure we (eventually) want to reject unknown attributes
everywhere. I don't have any concrete example in mind but I think there
will be use cases where we want to ignore unrecognized attributes
(probably per parse call). But it makes sense to require caller to
explicitely declare this is the case.
> While we're talking about wishlist, I'm also toying with the idea of
> having some sort of generic mechanism to convert netlink attributes
> to/from structs, for internal kernel representation; so far though I
> haven't been able to come up with anything useful.
I was also thinking about something like this. One motivation was to
design extensible version of ethtool_ops, the other was allowing complex
data types (structures, arrays) for ethtool tunables. But I have nothing
more than a vague idea so far.
Michal Kubecek
From: Johannes Berg <[email protected]>
Date: Wed, 12 Sep 2018 10:36:09 +0200
> From: Johannes Berg <[email protected]>
>
> In some situations some netlink attributes may be used for output
> only (kernel->userspace) or may be reserved for future use. It's
> then helpful to be able to prevent userspace from using them in
> messages sent to the kernel, since they'd otherwise be ignored and
> any future will become impossible if this happens.
>
> Add NLA_REJECT to the policy which does nothing but reject (with
> EINVAL) validation of any messages containing this attribute.
> Allow for returning a specific extended ACK error message in the
> validation_data pointer.
>
> While at it fix the indentation of NLA_BITFIELD32 and describe the
> validation_data pointer for it.
>
> The specific case I have in mind now is a shared nested attribute
> containing request/response data, and it would be pointless and
> potentially confusing to have userspace include response data in
> the messages that actually contain a request.
>
> Signed-off-by: Johannes Berg <[email protected]>
This looks great, no objections to this idea or the facility.
It does, however, remind me about about the classic problem of how bad
we are at feature support detection because unrecognized attributes are
ignored.
I do really hope we can fully solve that problem some day.
On Wed, 2018-09-12 at 10:49 +0200, Arend van Spriel wrote:
> On 9/12/2018 10:36 AM, Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > Commonly, ethernet addresses are just using a policy of
> > { .len = ETH_ALEN }
> > which leaves userspace free to send more data than it should,
> > which may hide bugs.
> >
> > Introduce NLA_ETH_ADDR which checks for exact size, and rejects
> > the attribute if the length isn't ETH_ALEN.
> >
> > Also add NLA_ETH_ADDR_COMPAT which can be used in place of the
> > policy above, but will, in addition, warn on an address that's
> > too long.
>
> Not sure if this is correctly described here. It seems longer addresses
> are not rejected, but only result in a warning message. I guess the
> problem is in the reference to the "policy above" ;-)
Yeah, good point. I meant ".len = ETH_ALEN" but should clarify that.
johannes
From: Johannes Berg <[email protected]>
Commonly, ethernet addresses are just using a policy of
{ .len = ETH_ALEN }
which leaves userspace free to send more data than it should,
which may hide bugs.
Introduce NLA_ETH_ADDR which checks for exact size, and rejects
the attribute if the length isn't ETH_ALEN.
Also add NLA_ETH_ADDR_COMPAT which can be used in place of the
policy above, but will, in addition, warn on an address that's
too long.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/netlink.h | 4 ++++
lib/nlattr.c | 8 ++++++++
2 files changed, 12 insertions(+)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 04e40fcc70d6..1139163c0db0 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -181,6 +181,8 @@ enum {
NLA_S64,
NLA_BITFIELD32,
NLA_REJECT,
+ NLA_ETH_ADDR,
+ NLA_ETH_ADDR_COMPAT,
__NLA_TYPE_MAX,
};
@@ -213,6 +215,8 @@ enum {
* data must point to a u32 value of valid flags
* NLA_REJECT Reject this attribute, validation data may point
* to a string to report as the error in extended ACK.
+ * NLA_ETH_ADDR Ethernet address, rejected if not exactly 6 octets.
+ * NLA_ETH_ADDR_COMPAT Ethernet address, only warns if not exactly 6 octets.
* All other Minimum length of attribute payload
*
* Example:
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 9ec0cc151148..3165b6d0baaa 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -29,6 +29,8 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
[NLA_S16] = sizeof(s16),
[NLA_S32] = sizeof(s32),
[NLA_S64] = sizeof(s64),
+ [NLA_ETH_ADDR] = ETH_ALEN,
+ [NLA_ETH_ADDR_COMPAT] = ETH_ALEN,
};
static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
@@ -42,6 +44,7 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
[NLA_S16] = sizeof(s16),
[NLA_S32] = sizeof(s32),
[NLA_S64] = sizeof(s64),
+ [NLA_ETH_ADDR_COMPAT] = ETH_ALEN,
};
static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -93,6 +96,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
extack->_msg = pt->validation_data;
return -EINVAL;
+ case NLA_ETH_ADDR:
+ if (attrlen != ETH_ALEN)
+ return -ERANGE;
+ break;
+
case NLA_FLAG:
if (attrlen > 0)
return -ERANGE;
--
2.14.4