Re: [PATCH net-next v6 02/15] netlink: rename nl80211_validate_nested() to nla_validate_nested()
On Tue, 2019-07-02 at 13:49 +0200, Michal Kubecek wrote: > Function nl80211_validate_nested() is not specific to nl80211, it's > a counterpart to nla_validate_nested_deprecated() with strict validation. > For consistency with other validation and parse functions, rename it to > nla_validate_nested(). Umm, right, not sure how that happened. Sorry about that. Reviewed-by: Johannes Berg
Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action
(switching to my personal email) > > I can't add these actions with current net-next and iproute-next: > > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc00 0x0100 > > Error: NLA_F_NESTED is missing. > > We have an error talking to the kernel > > > > This also happens with the current post of act_ct and should also > > happen with the act_mpls post (thus why Cc'ing John as well). > > > > I'm not sure how we should fix this. In theory the kernel can't get > > stricter with userspace here, as that breaks user applications as > > above, so older actions can't use the more stricter parser. Should we > > have some actions behaving one way, and newer ones in a different way? > > That seems bad. I think you could just fix all of the actions in userspace, since the older kernel would allow both with and without the flag, and then from a userspace POV it all behaves the same, just the kernel accepts some things without the flag for compatibility with older iproute2? > > Or maybe all actions should just use nla_parse_nested_deprecated()? > > I'm thinking this last. Yet, then the _deprecated suffix may not make > > much sense here. WDYT? > > Surely for new actions we can require strict validation, there is > no existing user space to speak of.. That was the original idea. > Perhaps act_ctinfo and act_ct > got slightly confused with the race you described, but in principle > there is nothing stopping new actions from implementing the user space > correctly, right? There's one potential thing where you have a new command in netlink (which thus will use strict validation), but you use existing code in userspace to build the netlink message or parts thereof? But then again you can just fix that while you test it, and the current and older kernel will accept the stricter version for the existing use of the existing code too, right? johannes
Re: [PATCH] NFC: Orphan the subsystem
On Tue, 2019-05-14 at 13:02 +0300, Andy Shevchenko wrote: > On Tue, May 14, 2019 at 11:02:31AM +0200, Johannes Berg wrote: > > Samuel clearly hasn't been working on this in many years and > > patches getting to the wireless list are just being ignored > > entirely now. Mark the subsystem as orphan to reflect the > > current state and revert back to the netdev list so at least > > some fixes can be picked up by Dave. > > Good to know. > > But Samuel was active like year ago and he is still at Intel, right? Not AFAICT and yes. I guess you can reach out to him internally, you're probably closer to him than I am, org-wise? :) https://patchwork.kernel.org/project/linux-wireless/list/?delegate=sameo johannes
[PATCH] NFC: Orphan the subsystem
Samuel clearly hasn't been working on this in many years and patches getting to the wireless list are just being ignored entirely now. Mark the subsystem as orphan to reflect the current state and revert back to the netdev list so at least some fixes can be picked up by Dave. Signed-off-by: Johannes Berg --- MAINTAINERS | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index fb9f9d71f7a2..b2659312e9ed 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11028,10 +11028,8 @@ S: Supported F: drivers/net/ethernet/qlogic/netxen/ NFC SUBSYSTEM -M: Samuel Ortiz -L: linux-wirel...@vger.kernel.org -L: linux-...@lists.01.org (subscribers-only) -S: Supported +L: netdev@vger.kernel.org +S: Orphan F: net/nfc/ F: include/net/nfc/ F: include/uapi/linux/nfc.h -- 2.17.2
[PATCH net] um: vector netdev: adjust to xmit_more API change
From: Johannes Berg
Replace skb->xmit_more usage by netdev_xmit_more().
Fixes: 4f296edeb9d4 ("drivers: net: aurora: use netdev_xmit_more helper")
Signed-off-by: Johannes Berg
---
arch/um/drivers/vector_kern.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
index 596e7056f376..e190e4ca52e1 100644
--- a/arch/um/drivers/vector_kern.c
+++ b/arch/um/drivers/vector_kern.c
@@ -1043,7 +1043,7 @@ static int vector_net_start_xmit(struct sk_buff *skb,
struct net_device *dev)
vector_send(vp->tx_queue);
return NETDEV_TX_OK;
}
- if (skb->xmit_more) {
+ if (netdev_xmit_more()) {
mod_timer(&vp->tl, vp->coalesce);
return NETDEV_TX_OK;
}
--
2.17.2
[PATCH v2 2/8] netlink: remove type-unsafe validation_data pointer
From: Johannes Berg
In the netlink policy, we currently have a void *validation_data
that's pointing to different things:
* a u32 value for bitfield32,
* the netlink policy for nested/nested array
* the string for NLA_REJECT
Remove the pointer and place appropriate type-safe items in the
union instead.
While at it, completely dissolve the pointer for the bitfield32
case and just put the value there directly.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 55 ---
lib/nlattr.c | 20
net/sched/act_api.c | 4 +---
3 files changed, 42 insertions(+), 37 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 679f649748d4..0dd4546fb68c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -217,7 +217,7 @@ enum nla_policy_validation {
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
- * validation_data is also used, for the max attr
+ * nested_policy is also used, for the max attr
* number in the nested policy.
*NLA_U8, NLA_U16,
*NLA_U32, NLA_U64,
@@ -235,27 +235,25 @@ enum nla_policy_validation {
*NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
- * Meaning of `validation_data' field:
+ * Meaning of validation union:
*NLA_BITFIELD32 This is a 32-bit bitmap/bitselector attribute and
- * validation data must point to a u32 value of valid
- * flags
- *NLA_REJECT This attribute is always rejected and validation
data
+ * `bitfield32_valid' is the u32 value of valid flags
+ *NLA_REJECT This attribute is always rejected and
`reject_message'
* may point to a string to report as the error instead
* of the generic one in extended ACK.
- *NLA_NESTED Points to a nested policy to validate, must also set
- * `len' to the max attribute number.
+ *NLA_NESTED `nested_policy' to a nested policy to validate, must
+ * also set `len' to the max attribute number. Use the
+ * provided NLA_POLICY_NESTED() macro.
* Note that nla_parse() will validate, but of course
not
* parse, the nested sub-policies.
- *NLA_NESTED_ARRAY Points to a nested policy to validate, must also set
- * `len' to the max attribute number. The difference to
- * NLA_NESTED is the structure - NLA_NESTED has the
- * nested attributes directly inside, while an array
has
- * the nested attributes at another level down and the
- * attributes directly in the nesting don't matter.
- *All otherUnused - but note that it's a union
- *
- * Meaning of `min' and `max' fields, use via NLA_POLICY_MIN, NLA_POLICY_MAX
- * and NLA_POLICY_RANGE:
+ *NLA_NESTED_ARRAY `nested_policy' points to a nested policy to
validate,
+ * must also set `len' to the max attribute number. Use
+ * the provided NLA_POLICY_NESTED_ARRAY() macro.
+ * The difference to NLA_NESTED is the structure:
+ * NLA_NESTED has the nested attributes directly inside
+ * while an array has the nested attributes at another
+ * level down and the attribute types directly in the
+ * nesting don't matter.
*NLA_U8,
*NLA_U16,
*NLA_U32,
@@ -263,14 +261,16 @@ enum nla_policy_validation {
*NLA_S8,
*NLA_S16,
*NLA_S32,
- *NLA_S64 These are used depending on the validation_type
- * field, if that is min/max/range then the minimum,
- * maximum and both are used (respectively) to check
+ *NLA_S64 The `min' and `max' fields are used depending on the
+ * validation_type field, if that is min/max/range then
+ * the min, max or both are used (respectively) to
check
* the value of the integer attribute.
* Note that in the interest of code simplicity and
* struct size both limits are s16, so you cannot
* enforce a range that doesn't fall within the range
* of s16 - do that as usual in th
[PATCH v2 5/8] netlink: remove NLA_EXACT_LEN_WARN
From: Johannes Berg
Use a validation type instead, so we can later expose
the NLA_* values to userspace for policy descriptions.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 15 ---
lib/nlattr.c | 16 ++--
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 2b035bf8daf6..3c3bbd2ae2dc 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -182,7 +182,6 @@ enum {
NLA_BITFIELD32,
NLA_REJECT,
NLA_EXACT_LEN,
- NLA_EXACT_LEN_WARN,
NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -204,6 +203,7 @@ enum nla_policy_validation {
NLA_VALIDATE_MAX,
NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
+ NLA_VALIDATE_WARN_TOO_LONG,
};
/**
@@ -237,10 +237,10 @@ enum nla_policy_validation {
* just like "All other"
*NLA_BITFIELD32 Unused
*NLA_REJECT Unused
- *NLA_EXACT_LENAttribute must have exactly this length, otherwise
- * it is rejected.
- *NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
- * is logged if it is longer, shorter is rejected.
+ *NLA_EXACT_LENAttribute should have exactly this length, otherwise
+ * it is rejected or warned about, the latter happening
+ * if and only if the `validation_type' is set to
+ * NLA_VALIDATE_WARN_TOO_LONG.
*NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
@@ -353,8 +353,9 @@ struct nla_policy {
};
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
-#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
- .len = _len }
+#define NLA_POLICY_EXACT_LEN_WARN(_len) \
+ { .type = NLA_EXACT_LEN, .len = _len, \
+ .validation_type = NLA_VALIDATE_WARN_TOO_LONG, }
#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index c8789de96046..05761d2a74cc 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -245,7 +245,9 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
BUG_ON(pt->type > NLA_TYPE_MAX);
if ((nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) ||
- (pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) {
+ (pt->type == NLA_EXACT_LEN &&
+pt->validation_type == NLA_VALIDATE_WARN_TOO_LONG &&
+attrlen != pt->len)) {
pr_warn_ratelimited("netlink: '%s': attribute type %d has an
invalid length.\n",
current->comm, type);
if (validate & NL_VALIDATE_STRICT_ATTRS) {
@@ -256,11 +258,6 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
switch (pt->type) {
- case NLA_EXACT_LEN:
- if (attrlen != pt->len)
- goto out_err;
- break;
-
case NLA_REJECT:
if (extack && pt->reject_message) {
NL_SET_BAD_ATTR(extack, nla);
@@ -373,6 +370,13 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
goto out_err;
break;
+ case NLA_EXACT_LEN:
+ if (pt->validation_type != NLA_VALIDATE_WARN_TOO_LONG) {
+ if (attrlen != pt->len)
+ goto out_err;
+ break;
+ }
+ /* fall through */
default:
if (pt->len)
minlen = pt->len;
--
2.17.2
[PATCH v2 0/8] netlink policy export and recursive validation
Here's (finally, sorry) the respin with the range/range_signed assignment fixed up. I've now included the validation recursion protection so it's clear that it applies on top of the other patches only. johannes
[PATCH v2 6/8] netlink: factor out policy range helpers
From: Johannes Berg
Add helpers to get the policy's signed/unsigned range
validation data.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 5 +++
lib/nlattr.c | 95 +--
2 files changed, 79 insertions(+), 21 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 3c3bbd2ae2dc..c2b4bc819784 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1895,4 +1895,9 @@ static inline bool nla_is_last(const struct nlattr *nla,
int rem)
return nla->nla_len == rem;
}
+void nla_get_range_unsigned(const struct nla_policy *pt,
+ struct netlink_range_validation *range);
+void nla_get_range_signed(const struct nla_policy *pt,
+ struct netlink_range_validation_signed *range);
+
#endif
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 05761d2a74cc..3db7a6984cb0 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,25 +96,39 @@ static int nla_validate_array(const struct nlattr *head,
int len, int maxtype,
return 0;
}
-static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
- const struct nlattr *nla,
- struct netlink_ext_ack *extack)
+void nla_get_range_unsigned(const struct nla_policy *pt,
+ struct netlink_range_validation *range)
{
- struct netlink_range_validation _range = {
- .min = 0,
- .max = U64_MAX,
- }, *range = &_range;
- u64 value;
-
WARN_ON_ONCE(pt->min < 0 || pt->max < 0);
+ range->min = 0;
+
+ switch (pt->type) {
+ case NLA_U8:
+ range->max = U8_MAX;
+ break;
+ case NLA_U16:
+ range->max = U16_MAX;
+ break;
+ case NLA_U32:
+ range->max = U32_MAX;
+ break;
+ case NLA_U64:
+ case NLA_MSECS:
+ range->max = U64_MAX;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
+
switch (pt->validation_type) {
case NLA_VALIDATE_RANGE:
range->min = pt->min;
range->max = pt->max;
break;
case NLA_VALIDATE_RANGE_PTR:
- range = pt->range;
+ *range = *pt->range;
break;
case NLA_VALIDATE_MIN:
range->min = pt->min;
@@ -122,7 +136,17 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
case NLA_VALIDATE_MAX:
range->max = pt->max;
break;
+ default:
+ break;
}
+}
+
+static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
+ const struct nlattr *nla,
+ struct netlink_ext_ack *extack)
+{
+ struct netlink_range_validation range;
+ u64 value;
switch (pt->type) {
case NLA_U8:
@@ -142,7 +166,9 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
return -EINVAL;
}
- if (value < range->min || value > range->max) {
+ nla_get_range_unsigned(pt, &range);
+
+ if (value < range.min || value > range.max) {
NL_SET_ERR_MSG_ATTR(extack, nla,
"integer out of range");
return -ERANGE;
@@ -151,15 +177,30 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
return 0;
}
-static int nla_validate_int_range_signed(const struct nla_policy *pt,
-const struct nlattr *nla,
-struct netlink_ext_ack *extack)
+void nla_get_range_signed(const struct nla_policy *pt,
+ struct netlink_range_validation_signed *range)
{
- struct netlink_range_validation_signed _range = {
- .min = S64_MIN,
- .max = S64_MAX,
- }, *range = &_range;
- s64 value;
+ switch (pt->type) {
+ case NLA_S8:
+ range->min = S8_MIN;
+ range->max = S8_MAX;
+ break;
+ case NLA_S16:
+ range->min = S16_MIN;
+ range->max = S16_MAX;
+ break;
+ case NLA_S32:
+ range->min = S32_MIN;
+ range->max = S32_MAX;
+ break;
+ case NLA_S64:
+ range->min = S64_MIN;
+ range->max = S64_MAX;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
switch (pt->validation_type) {
case NLA_VALIDATE_RANGE:
@@ -167,7 +208,7 @@
[PATCH v2 4/8] netlink: allow NLA_MSECS to have range validation
From: Johannes Berg
Since NLA_MSECS is really equivalent to NLA_U64, allow
it to have range validation as well.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 --
lib/nlattr.c | 2 ++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 2b91a15803b0..2b035bf8daf6 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -374,7 +374,8 @@ struct nla_policy {
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
#define NLA_ENSURE_UINT_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
- tp == NLA_U32 || tp == NLA_U64) + tp)
+ tp == NLA_U32 || tp == NLA_U64 || \
+ tp == NLA_MSECS) + tp)
#define NLA_ENSURE_SINT_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_S16 || \
tp == NLA_S32 || tp == NLA_S64) + tp)
@@ -382,7 +383,8 @@ struct nla_policy {
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 || \
tp == NLA_S16 || tp == NLA_U16 || \
tp == NLA_S32 || tp == NLA_U32 || \
- tp == NLA_S64 || tp == NLA_U64) + tp)
+ tp == NLA_S64 || tp == NLA_U64 || \
+ tp == NLA_MSECS) + tp)
#define NLA_ENSURE_NO_VALIDATION_PTR(tp) \
(__NLA_ENSURE(tp != NLA_BITFIELD32 && \
tp != NLA_REJECT && \
diff --git a/lib/nlattr.c b/lib/nlattr.c
index b549b290d3fa..c8789de96046 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -135,6 +135,7 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
value = nla_get_u32(nla);
break;
case NLA_U64:
+ case NLA_MSECS:
value = nla_get_u64(nla);
break;
default:
@@ -211,6 +212,7 @@ static int nla_validate_int_range(const struct nla_policy
*pt,
case NLA_U16:
case NLA_U32:
case NLA_U64:
+ case NLA_MSECS:
return nla_validate_int_range_unsigned(pt, nla, extack);
case NLA_S8:
case NLA_S16:
--
2.17.2
[PATCH v2 1/8] nl80211: fix NL80211_ATTR_FTM_RESPONDER policy
From: Johannes Berg
The nested policy here should be established using the
NLA_POLICY_NESTED() macro so the length is properly
filled in.
Fixes: 81e54d08d9d8 ("cfg80211: support FTM responder configuration/statistics")
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fffe4b371e23..f40a004ec6f2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -538,10 +538,8 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] =
{
[NL80211_ATTR_HE_CAPABILITY] = { .type = NLA_BINARY,
.len = NL80211_HE_MAX_CAPABILITY_LEN },
- [NL80211_ATTR_FTM_RESPONDER] = {
- .type = NLA_NESTED,
- .validation_data = nl80211_ftm_responder_policy,
- },
+ [NL80211_ATTR_FTM_RESPONDER] =
+ NLA_POLICY_NESTED(nl80211_ftm_responder_policy),
[NL80211_ATTR_TIMEOUT] = NLA_POLICY_MIN(NLA_U32, 1),
[NL80211_ATTR_PEER_MEASUREMENTS] =
NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
--
2.17.2
[PATCH v2 3/8] netlink: extend policy range validation
From: Johannes Berg
Using a pointer to a struct indicating the min/max values,
extend the ability to do range validation for arbitrary
values. Small values in the s16 range can be kept in the
policy directly.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 45 +
lib/nlattr.c | 112 ++
2 files changed, 136 insertions(+), 21 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0dd4546fb68c..2b91a15803b0 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -189,11 +189,20 @@ enum {
#define NLA_TYPE_MAX (__NLA_TYPE_MAX - 1)
+struct netlink_range_validation {
+ u64 min, max;
+};
+
+struct netlink_range_validation_signed {
+ s64 min, max;
+};
+
enum nla_policy_validation {
NLA_VALIDATE_NONE,
NLA_VALIDATE_RANGE,
NLA_VALIDATE_MIN,
NLA_VALIDATE_MAX,
+ NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
};
@@ -271,6 +280,22 @@ enum nla_policy_validation {
* of s16 - do that as usual in the code instead.
* Use the NLA_POLICY_MIN(), NLA_POLICY_MAX() and
* NLA_POLICY_RANGE() macros.
+ *NLA_U8,
+ *NLA_U16,
+ *NLA_U32,
+ *NLA_U64 If the validation_type field instead is set to
+ * NLA_VALIDATE_RANGE_PTR, `range' must be a pointer
+ * to a struct netlink_range_validation that indicates
+ * the min/max values.
+ * Use NLA_POLICY_FULL_RANGE().
+ *NLA_S8,
+ *NLA_S16,
+ *NLA_S32,
+ *NLA_S64 If the validation_type field instead is set to
+ * NLA_VALIDATE_RANGE_PTR, `range_signed' must be a
+ * pointer to a struct netlink_range_validation_signed
+ * that indicates the min/max values.
+ * Use NLA_POLICY_FULL_RANGE_SIGNED().
*All otherUnused - but note that it's a union
*
* Meaning of `validate' field, use via NLA_POLICY_VALIDATE_FN:
@@ -299,6 +324,8 @@ struct nla_policy {
const u32 bitfield32_valid;
const char *reject_message;
const struct nla_policy *nested_policy;
+ struct netlink_range_validation *range;
+ struct netlink_range_validation_signed *range_signed;
struct {
s16 min, max;
};
@@ -345,6 +372,12 @@ struct nla_policy {
{ .type = NLA_BITFIELD32, .bitfield32_valid = valid }
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
+#define NLA_ENSURE_UINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
+ tp == NLA_U32 || tp == NLA_U64) + tp)
+#define NLA_ENSURE_SINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_S8 || tp == NLA_S16 || \
+ tp == NLA_S32 || tp == NLA_S64) + tp)
#define NLA_ENSURE_INT_TYPE(tp)\
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 || \
tp == NLA_S16 || tp == NLA_U16 || \
@@ -363,6 +396,18 @@ struct nla_policy {
.max = _max \
}
+#define NLA_POLICY_FULL_RANGE(tp, _range) {\
+ .type = NLA_ENSURE_UINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_RANGE_PTR, \
+ .range = _range,\
+}
+
+#define NLA_POLICY_FULL_RANGE_SIGNED(tp, _range) { \
+ .type = NLA_ENSURE_SINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_RANGE_PTR, \
+ .range_signed = _range, \
+}
+
#define NLA_POLICY_MIN(tp, _min) { \
.type = NLA_ENSURE_INT_TYPE(tp),\
.validation_type = NLA_VALIDATE_MIN,\
diff --git a/lib/nlattr.c b/lib/nlattr.c
index c546db7c72dd..b549b290d3fa 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,17 +96,33 @@ static int nla_validate_array(const struct nlattr *head,
int len, int maxtype,
return 0;
}
-static int nla_validate_int_range(const struct nla_policy *pt,
- const struct nlattr *nla,
- struct netlink_ext_ack *extack)
+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 ||
-
[PATCH v2 7/8] netlink: add infrastructure to expose policies to userspace
From: Johannes Berg
Add, and use in generic netlink, helpers to dump out a netlink
policy to userspace, including all the range validation data,
nested policies etc.
This lets userspace discover what the kernel understands.
For families/commands other than generic netlink, the helpers
need to be used directly in an appropriate command, or we can
add some infrastructure (a new netlink family) that those can
register their policies with for introspection. I'm not that
familiar with non-generic netlink, so that's left out for now.
The data exposed to userspace also includes min and max length
for binary/string data, I've done that instead of letting the
userspace tools figure out whether min/max is intended based
on the type so that we can extend this later in the kernel, we
might want to just use the range data for example.
Because of this, I opted to not directly expose the NLA_*
values, even if some of them are already exposed via BPF, as
with min/max length we don't need to have different types here
for NLA_BINARY/NLA_MIN_LEN/NLA_EXACT_LEN, we just make them
all NL_ATTR_TYPE_BINARY with min/max length optionally set.
Similarly, we don't really need NLA_MSECS, and perhaps can
remove it in the future - but not if we encode it into the
userspace API now. It gets mapped to NL_ATTR_TYPE_U64 here.
Note that the exposing here corresponds to the strict policy
interpretation, and NLA_UNSPEC items are omitted entirely.
To get those, change them to NLA_MIN_LEN which behaves in
exactly the same way, but is exposed.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +
include/uapi/linux/genetlink.h | 2 +
include/uapi/linux/netlink.h | 103 +++
net/netlink/Makefile | 2 +-
net/netlink/genetlink.c| 77 +
net/netlink/policy.c | 308 +
6 files changed, 497 insertions(+), 1 deletion(-)
create mode 100644 net/netlink/policy.c
diff --git a/include/net/netlink.h b/include/net/netlink.h
index c2b4bc819784..e298838a57dc 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1900,4 +1900,10 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
void nla_get_range_signed(const struct nla_policy *pt,
struct netlink_range_validation_signed *range);
+int netlink_policy_dump_start(const struct nla_policy *policy,
+ unsigned int maxtype,
+ unsigned long *state);
+bool netlink_policy_dump_loop(unsigned long *state);
+int netlink_policy_dump_write(struct sk_buff *skb, unsigned long state);
+
#endif
diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
index 877f7fa95466..9c0636ec2286 100644
--- a/include/uapi/linux/genetlink.h
+++ b/include/uapi/linux/genetlink.h
@@ -48,6 +48,7 @@ enum {
CTRL_CMD_NEWMCAST_GRP,
CTRL_CMD_DELMCAST_GRP,
CTRL_CMD_GETMCAST_GRP, /* unused */
+ CTRL_CMD_GETPOLICY,
__CTRL_CMD_MAX,
};
@@ -62,6 +63,7 @@ enum {
CTRL_ATTR_MAXATTR,
CTRL_ATTR_OPS,
CTRL_ATTR_MCAST_GROUPS,
+ CTRL_ATTR_POLICY,
__CTRL_ATTR_MAX,
};
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 0a4d73317759..eac8a6a648ea 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -249,4 +249,107 @@ struct nla_bitfield32 {
__u32 selector;
};
+/*
+ * policy descriptions - it's specific to each family how this is used
+ * Normally, it should be retrieved via a dump inside another attribute
+ * specifying where it applies.
+ */
+
+/**
+ * enum netlink_attribute_type - type of an attribute
+ * @NL_ATTR_TYPE_INVALID: unused
+ * @NL_ATTR_TYPE_FLAG: flag attribute (present/not present)
+ * @NL_ATTR_TYPE_U8: 8-bit unsigned attribute
+ * @NL_ATTR_TYPE_U16: 16-bit unsigned attribute
+ * @NL_ATTR_TYPE_U32: 32-bit unsigned attribute
+ * @NL_ATTR_TYPE_U64: 64-bit unsigned attribute
+ * @NL_ATTR_TYPE_S8: 8-bit signed attribute
+ * @NL_ATTR_TYPE_S16: 16-bit signed attribute
+ * @NL_ATTR_TYPE_S32: 32-bit signed attribute
+ * @NL_ATTR_TYPE_S64: 64-bit signed attribute
+ * @NL_ATTR_TYPE_BINARY: binary data, min/max length may be specified
+ * @NL_ATTR_TYPE_STRING: string, min/max length may be specified
+ * @NL_ATTR_TYPE_NUL_STRING: NUL-terminated string,
+ * min/max length may be specified
+ * @NL_ATTR_TYPE_NESTED: nested, i.e. the content of this attribute
+ * consists of sub-attributes. The nested policy and maxtype
+ * inside may be specified.
+ * @NL_ATTR_TYPE_NESTED_ARRAY: nested array, i.e. the content of this
+ * attribute contains sub-attributes whose type is irrelevant
+ * (just used to separate the array entries) and each such array
+ * entry has attributes again, the policy for those inner ones
+ * and the corresponding maxtype may be specified.
+ * @NL_ATTR_TYPE_BITFIELD32: &struct nla_bitfield32 attribute
[PATCH v2 8/8] netlink: limit recursion depth in policy validation
From: Johannes Berg
Now that we have nested policies, we can theoretically
recurse forever parsing attributes if a (sub-)policy
refers back to a higher level one. This is a situation
that has happened in nl80211, and we've avoided it there
by not linking it.
Add some code to netlink parsing to limit recursion depth,
allowing us to safely change nl80211 to actually link the
nested policy, which in turn allows some code cleanups.
Signed-off-by: Johannes Berg
---
lib/nlattr.c | 46 +++---
net/wireless/nl80211.c | 10 -
net/wireless/nl80211.h | 2 --
net/wireless/pmsr.c| 3 +--
4 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 3db7a6984cb0..ef06645de56c 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -44,6 +44,20 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
[NLA_S64] = sizeof(s64),
};
+/*
+ * Nested policies might refer back to the original
+ * policy in some cases, and userspace could try to
+ * abuse that and recurse by nesting in the right
+ * ways. Limit recursion to avoid this problem.
+ */
+#define MAX_POLICY_RECURSION_DEPTH 10
+
+static int __nla_validate_parse(const struct nlattr *head, int len, int
maxtype,
+ const struct nla_policy *policy,
+ unsigned int validate,
+ struct netlink_ext_ack *extack,
+ struct nlattr **tb, unsigned int depth);
+
static int validate_nla_bitfield32(const struct nlattr *nla,
const u32 valid_flags_mask)
{
@@ -70,7 +84,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack,
- unsigned int validate)
+ unsigned int validate, unsigned int depth)
{
const struct nlattr *entry;
int rem;
@@ -87,8 +101,9 @@ static int nla_validate_array(const struct nlattr *head, int
len, int maxtype,
return -ERANGE;
}
- ret = __nla_validate(nla_data(entry), nla_len(entry),
-maxtype, policy, validate, extack);
+ ret = __nla_validate_parse(nla_data(entry), nla_len(entry),
+ maxtype, policy, validate, extack,
+ NULL, depth + 1);
if (ret < 0)
return ret;
}
@@ -280,7 +295,7 @@ static int nla_validate_int_range(const struct nla_policy
*pt,
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy, unsigned int validate,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack, unsigned int depth)
{
u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
@@ -375,9 +390,10 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
if (attrlen < NLA_HDRLEN)
goto out_err;
if (pt->nested_policy) {
- err = __nla_validate(nla_data(nla), nla_len(nla),
pt->len,
-pt->nested_policy, validate,
-extack);
+ err = __nla_validate_parse(nla_data(nla), nla_len(nla),
+ pt->len, pt->nested_policy,
+ validate, extack, NULL,
+ depth + 1);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -400,7 +416,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
err = nla_validate_array(nla_data(nla), nla_len(nla),
pt->len, pt->nested_policy,
-extack, validate);
+extack, validate, depth);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -472,11 +488,17 @@ static int __nla_validate_parse(const struct nlattr
*head, int len, int maxtype,
const struct nla_policy *policy,
unsigned int validate,
struct netlink_ext_ack *extack,
- struct nlattr **tb)
+
Re: [PATCH] netlink: limit recursion depth in policy validation
On Mon, 2019-04-29 at 23:08 -0400, David Miller wrote: > From: Johannes Berg > Date: Fri, 26 Apr 2019 14:13:46 +0200 > > > From: Johannes Berg > > > > Now that we have nested policies, we can theoretically > > recurse forever parsing attributes if a (sub-)policy > > refers back to a higher level one. This is a situation > > that has happened in nl80211, and we've avoided it there > > by not linking it. > > > > Add some code to netlink parsing to limit recursion depth, > > allowing us to safely change nl80211 to actually link the > > nested policy, which in turn allows some code cleanups. > > > > Signed-off-by: Johannes Berg > > This doesn't apply cleanly to 'net', is there some dependency I am > unaware of or is this because of a recent mac80211 pull into my tree? Sorry, I should've made it clear that this applies on top of the patchset to expose netlink policies to userspace; due to all the overlaping changes in lib/nlattr.c that seemed like the best solution to me. There's no real need to have this safeguard right now in net as nothing there actually specifies a recursive policy (I knew about this issue and explicitly made nl80211 *not* have a recursive policy as you can see in this patch changing that), so I figured net-next was fine. I'll rebase this on net-next along with the policy export (fixing the full signed range thing) and resend as a combined set to clarify the dependencies. If you prefer to have the safeguard in net even if it shouldn't be needed now, let me know and I'll make a version that applies there, but note that will invariably cause conflicts with all the other changes in lib/nlattr.c. johannes
Re: [PATCH 2/6] netlink: extend policy range validation
On Mon, 2019-04-29 at 22:49 -0400, David Miller wrote:
> From: Johannes Berg
> Date: Fri, 26 Apr 2019 14:13:02 +0200
>
> > * NLA_POLICY_RANGE() macros.
> > + *NLA_U8,
> > + *NLA_U16,
> > + *NLA_U32,
> > + *NLA_U64 If the validation_type field instead is set to
> > + * NLA_VALIDATE_RANGE_PTR, `range' must be a
> > pointer
> > + * to a struct netlink_range_validation that
> > indicates
> > + * the min/max values.
> > + * Use NLA_POLICY_FULL_RANGE().
> > + *NLA_S8,
> > + *NLA_S16,
> > + *NLA_S32,
> > + *NLA_S64 If the validation_type field instead is set to
> > + * NLA_VALIDATE_RANGE_PTR, `range_signed' must be a
> > + * pointer to a struct
> > netlink_range_validation_signed
> > + * that indicates the min/max values.
> > + * Use NLA_POLICY_FULL_RANGE_SIGNED().
>
> Documentation and datastructure says that "range_signed" member should be set
> for signed ranges, however:
>
> > +#define NLA_POLICY_FULL_RANGE(tp, _range) { \
> > + .type = NLA_ENSURE_UINT_TYPE(tp), \
> > + .validation_type = NLA_VALIDATE_RANGE_PTR, \
> > + .range = _range,\
> > +}
> > +
> > +#define NLA_POLICY_FULL_RANGE_SIGNED(tp, _range) { \
> > + .type = NLA_ENSURE_SINT_TYPE(tp), \
> > + .validation_type = NLA_VALIDATE_RANGE_PTR, \
> > + .range = _range,\
> > +}
>
> The NLA_POLICY_FULL_RANGE_SIGNED macros sets 'range' not 'range_signed'.
D'oh. Copy/paste error, and I must've missed the compiler warning that
should appear here on usage then. At least I'm pretty sure I tested that
with the policy exposition patch.
Will fix.
> Also, since range and range_signed are in a union however there is only one
> NLA_VALIDATE_RANGE_PTR type, how does one differentiate between signed and
> unsigned ranges exactly?
Based on the type - NLA_S* or NLA_U*. See the NLA_ENSURE_SINT_TYPE() and
NLA_ENSURE_UINT_TYPE() in the macros - that ensures you can only use
NLA_POLICY_FULL_RANGE_SIGNED() with NLA_S*, and NLA_POLICY_FULL_RANGE()
with NLA_U*.
johannes
Re: [PATCH 6/6] netlink: add infrastructure to expose policies to userspace
On Sat, 2019-04-27 at 13:25 +0200, Pablo Neira Ayuso wrote:
>
> > Take IFLA_AF_SPEC for example. To validate that, we end up calling into
> > validate_link_af() which is defined in IPv4 and IPv6, rather than having
> > the inet_af_policy/inet6_af_policy available and doing it in the caller
> > (also, validate_link_af() does some additional validation, though for
> > IFLA_INET_CONF that can actually now be expressed as a nested policy
> > inside inet_af_policy, I believe).
> >
> > So to really generalize that you'd have change this - at least as far as
> > the netlink attribute validation is concerned, not the extra code - to
> > be data driven, rather than coded.
> >
> > Then you could use and expose that data pretty easily.
>
> I see, agreed, we would need to rework this to make data driven, so
> this nest:
>
> IFLA_AF_SPEC (nest)
> family = AF_INET6 (nest)
> IFLA_INET6_...
>
> IFLA_AF_SPEC (nest)
> family = AF_INET4 (nest)
> IFLA_INET_...
>
> needs a nla_policy definition for each family.
Right.
> We can do this rework progressively, as we start exposing description
> though the list of nla_policy structure for each subsystem.
Sure, of course.
I'm just saying it's not easy right now to provide that, as all of this
is mostly code-driven today. You have
rtnl_register(PF_UNSPEC, RTM_NEWLINK, rtnl_newlink, NULL, 0);
but the actual parsing happens WAY down in the code, multiple levels of
functions deep that are sometimes shared etc.
I'm not even sure how we could express this in the policy - right now we
only have
[ILFA_AF_SPEC] = NLA_POLICY_NESTED(xyz_policy)
but that cannot capture the fact that the inner attributes depend on
another attribute or some other circumstance.
Oh, actually, it's not hard in this case because the AF is prescribed by
the type, so we'd need something like
struct nla_policy af_spec_policy[...] = {
[AF_INET] = NLA_POLICY_NESTED(af_inet_policy),
[AF_INET6] = NLA_POLICY_NESTED(af_inet6_policy),
// .. also for MPLS
};
struct nla_policy ifla_policy[...] = {
[IFLA_AF_SPEC] = NLA_POLICY_NESTED(af_spec_policy),
};
So it's actually easy to do and we'd just need to change the
rtnl_register() to something like rtnl_register_with_policy() or
something like that, and then we'd have all the data we want directly
available and would actually save quite a bit of code, including
possibly the whole validate_link_af() method pointer.
johannes
Re: [PATCH] netlink: limit recursion depth in policy validation
Hi Pablo, > > + [NL80211_PMSR_PEER_ATTR_CHAN] = NLA_POLICY_NESTED(nl80211_policy), > > I guess you only allow one more nested instance of this attribute? > > I mean, how many times is NL80211 allow to recurse on this? It doesn't actually recurse on this at all. We really should've specified a channel originally as a nested attribute, and have had: * NL80211_ATTR_CHAN * NL80211_CHAN_ATTR_* and then we could've reused NL80211_CHAN_ATTR_* here as * NL80211_ATTR_SOMETHING ... * NL80211_PMSR_PEER_ATTR_CHAN * NL80211_CHAN_ATTR_* However, as we didn't, we have a few NL80211_ATTR_CHAN_* attributes (at least conceptually) and allow these here inside this deeply nested object so that we don't have to rewrite the channel parsing and writing code in both kernel and userspace now. > Probably you can define a new nl80211_policy_recurse object and set a > flag somewhere to describe that no more recursion are permitted? We really should even define which attributes are permitted in the nesting to start with, and then we wouldn't even get into a recursion situation, at least not in this particular case. > I would try to handle this from nl80211, instead of from the core by > limiting recursions to 10. Nevertheless, I think (arbitrarily) limiting recursion is necessary, because we don't want people to even accidentally build a policy that links to sub a sub-policy and then somehow ends up linking back up to a policy that's further up in the chain, and then this never really gets thought about until somebody abuses it for a stack clash attack. After all, I expect in most of these cases - if they happen - that it'd be similar to the nl80211 example above, where semantically the recursion makes no sense. > Once we expose the descriptions to userspace, I would expect we'll end > with tools to validate all kind of stuff like this, eg. fuzzy tested, > check for recursions like this (which IMO they should not be allowed). Yeah, but I'd rather have the fuzzers run into a reject than actually have a stack clash bug here that we then find and fix, but leave as a vulnerability until we do. johannes
Re: [PATCH v2 0/5] strict netlink validation
On Fri, 2019-04-26 at 20:28 -0600, David Ahern wrote: > > I agree with this set and will help moving forward. As I recall it > requires follow up patches for each policy to set strict_start_type > opting in to the strict checking. With that in place new userspace on > old kernels will get a failure trying to configure a feature the old > kernel does not recognize. Actually, that part you had already handled with nla_parse_strict() (now nla_parse_strict_deprecated()) - and I'm not sure we can make this even stricter because existing code might be setting future attributes already, expecting them to be ignored. This is already fishy of applications to expect though, but I'm not sure we really can change that? I don't think I'm actually changing something here, but I'm certainly open to suggestions - after all, when we actually do get around to adding that future attribute it almost certainly will have a different type than a (buggy) application would be using now. However, what I did already do is that adding strict_start_type to policies means that all future attributes added to those policies will be handled in a strict fashion, i.e. if you add strict_start_type and then add a new u32 attribute >= strict_start_type, that new u32 attribute will not be permitted to have a size other than 4 octets. johannes
Re: [PATCH 6/6] netlink: add infrastructure to expose policies to userspace
On Fri, 2019-04-26 at 20:21 +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 26, 2019 at 02:13:06PM +0200, Johannes Berg wrote:
> > diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
> > index 877f7fa95466..9c0636ec2286 100644
> > --- a/include/uapi/linux/genetlink.h
> > +++ b/include/uapi/linux/genetlink.h
> > @@ -48,6 +48,7 @@ enum {
> > CTRL_CMD_NEWMCAST_GRP,
> > CTRL_CMD_DELMCAST_GRP,
> > CTRL_CMD_GETMCAST_GRP, /* unused */
> > + CTRL_CMD_GETPOLICY,
>
> It would be good to single entry point to request descriptions, ie.
> have a NETLINK_DESC family for this. Thus, we could use the same
> program to pull for policy/command descriptions without updating an
> array that includes the command to get the policy _for each
> subsystem_.
>
> The program to inquire for policy/command descriptions would be very
> much the same along time, no need for updates to include new command
> type for each subsystem.
>
> It would just spin over NETLINK_DESC discovering subsystems ID that we
> support.
No objection to that. The only problem I think is that there's no
natural point to "hang" the policy data, mostly it's just used in the
nla_parse() or nla_validate() calls in the code. Basically, it's code-
driven, not data-driven like generic netlink.
Take IFLA_AF_SPEC for example. To validate that, we end up calling into
validate_link_af() which is defined in IPv4 and IPv6, rather than having
the inet_af_policy/inet6_af_policy available and doing it in the caller
(also, validate_link_af() does some additional validation, though for
IFLA_INET_CONF that can actually now be expressed as a nested policy
inside inet_af_policy, I believe).
So to really generalize that you'd have change this - at least as far as
the netlink attribute validation is concerned, not the extra code - to
be data driven, rather than coded.
Then you could use and expose that data pretty easily.
> In genetlink, I understand this can be exception if you prefer so, ie.
> I'll be fine with this CTRL_CMD_GETPOLICY if that makes it look nicer
> in terms of integration with the existing infrastructure. But for
> other netlink subsystems, NETLINK_DESC allows you to pull the
> description for genetlink itself, not the internal subsystems.
I think genetlink it really makes more sense this way - the genetlink
family it basically the introspection point already: it lets you
discover which families there are, which commands they support,
multicast groups they have etc.
It's also easier to deal with in userspace because you already need to
deal with the genetlink family itself (to get your family ID), so
interacting with another NETLINK_DESC family would be annoying.
However, if we wanted to generalize that I guess we could make
NETLINK_DESC able to cover generic netlink as well, providing two entry
points to the same information? Very small amount of code, I guess.
johannes
Re: [RFC] netlink: limit recursion depth in policy validation
On Fri, 2019-04-26 at 19:06 +0200, Pablo Neira Ayuso wrote: > > > This basically flattens the whole thing. > > > > Obviously, the walking may allocate some memory, and the last loop to > > send it out isn't actually a loop like that because it's a netlink dump > > with each entry being in a separate netlink message, but that's the gist > > of it. > > I see, following this approach, I can just remove the duplicated code > in my netlink description stuff by using the list of policy > structures. I wrote the code in a way that you can reuse it easily, check out the patch :-) Generic netlink is one user added by the patch, but the actual exposing code is with general attributes and general code that you can easily call. Meanwhile, I'm still writing a response to your other email, give me a few minutes. johannes
Re: [RFC] netlink: limit recursion depth in policy validation
On Fri, 2019-04-26 at 18:57 +0200, Pablo Neira Ayuso wrote:
>
> > +/*
> > + * Nested policies might refer back to the original
> > + * policy in some cases, and userspace could try to
> > + * abuse that and recurse by nesting in the right
> > + * ways. Limit recursion to avoid this problem.
> > + */
> > +#define MAX_POLICY_RECURSION_DEPTH 10
>
> In your policy description approach, you iterate over the policy
> structures. How do you deal with this recursions from there?
Well, check out the code :-)
It doesn't actually recurse. What it does is build a list of policies
that are reachable from the root policy and each policy in the list. So
basically, there we do:
list = [root policy]
list_len = 1
i = 0
walk_policy(policy)
{
for_each_policy_entry(entry, policy) {
nested = nested_policy_or_null(entry);
if (nested) {
list[i] = nested;
list_len += 1
}
}
}
while (i < list_len) {
walk_policy(list[i]);
i++;
}
Then, we walk the list again:
for (i = 0; i < list_len; i++) {
for_each_policy_entry(entry, list[i]) {
send_entry_to_userspace(i, entry); // mark it as occurring in policy i
}
}
This basically flattens the whole thing.
Obviously, the walking may allocate some memory, and the last loop to
send it out isn't actually a loop like that because it's a netlink dump
with each entry being in a separate netlink message, but that's the gist
of it.
johannes
[PATCH] netlink: limit recursion depth in policy validation
From: Johannes Berg
Now that we have nested policies, we can theoretically
recurse forever parsing attributes if a (sub-)policy
refers back to a higher level one. This is a situation
that has happened in nl80211, and we've avoided it there
by not linking it.
Add some code to netlink parsing to limit recursion depth,
allowing us to safely change nl80211 to actually link the
nested policy, which in turn allows some code cleanups.
Signed-off-by: Johannes Berg
---
lib/nlattr.c | 46 +++---
net/wireless/nl80211.c | 10 -
net/wireless/nl80211.h | 2 --
net/wireless/pmsr.c| 3 +--
4 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 3db7a6984cb0..ef06645de56c 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -44,6 +44,20 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
[NLA_S64] = sizeof(s64),
};
+/*
+ * Nested policies might refer back to the original
+ * policy in some cases, and userspace could try to
+ * abuse that and recurse by nesting in the right
+ * ways. Limit recursion to avoid this problem.
+ */
+#define MAX_POLICY_RECURSION_DEPTH 10
+
+static int __nla_validate_parse(const struct nlattr *head, int len, int
maxtype,
+ const struct nla_policy *policy,
+ unsigned int validate,
+ struct netlink_ext_ack *extack,
+ struct nlattr **tb, unsigned int depth);
+
static int validate_nla_bitfield32(const struct nlattr *nla,
const u32 valid_flags_mask)
{
@@ -70,7 +84,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack,
- unsigned int validate)
+ unsigned int validate, unsigned int depth)
{
const struct nlattr *entry;
int rem;
@@ -87,8 +101,9 @@ static int nla_validate_array(const struct nlattr *head, int
len, int maxtype,
return -ERANGE;
}
- ret = __nla_validate(nla_data(entry), nla_len(entry),
-maxtype, policy, validate, extack);
+ ret = __nla_validate_parse(nla_data(entry), nla_len(entry),
+ maxtype, policy, validate, extack,
+ NULL, depth + 1);
if (ret < 0)
return ret;
}
@@ -280,7 +295,7 @@ static int nla_validate_int_range(const struct nla_policy
*pt,
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy, unsigned int validate,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack, unsigned int depth)
{
u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
@@ -375,9 +390,10 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
if (attrlen < NLA_HDRLEN)
goto out_err;
if (pt->nested_policy) {
- err = __nla_validate(nla_data(nla), nla_len(nla),
pt->len,
-pt->nested_policy, validate,
-extack);
+ err = __nla_validate_parse(nla_data(nla), nla_len(nla),
+ pt->len, pt->nested_policy,
+ validate, extack, NULL,
+ depth + 1);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -400,7 +416,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
err = nla_validate_array(nla_data(nla), nla_len(nla),
pt->len, pt->nested_policy,
-extack, validate);
+extack, validate, depth);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -472,11 +488,17 @@ static int __nla_validate_parse(const struct nlattr
*head, int len, int maxtype,
const struct nla_policy *policy,
unsigned int validate,
struct netlink_ext_ack *extack,
- struct nlattr **tb)
+
[PATCH 5/6] netlink: factor out policy range helpers
From: Johannes Berg
Add helpers to get the policy's signed/unsigned range
validation data.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 5 +++
lib/nlattr.c | 95 +--
2 files changed, 79 insertions(+), 21 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0a02d25fb163..aab0a30021b6 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1875,4 +1875,9 @@ static inline bool nla_is_last(const struct nlattr *nla,
int rem)
return nla->nla_len == rem;
}
+void nla_get_range_unsigned(const struct nla_policy *pt,
+ struct netlink_range_validation *range);
+void nla_get_range_signed(const struct nla_policy *pt,
+ struct netlink_range_validation_signed *range);
+
#endif
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 05761d2a74cc..3db7a6984cb0 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,25 +96,39 @@ static int nla_validate_array(const struct nlattr *head,
int len, int maxtype,
return 0;
}
-static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
- const struct nlattr *nla,
- struct netlink_ext_ack *extack)
+void nla_get_range_unsigned(const struct nla_policy *pt,
+ struct netlink_range_validation *range)
{
- struct netlink_range_validation _range = {
- .min = 0,
- .max = U64_MAX,
- }, *range = &_range;
- u64 value;
-
WARN_ON_ONCE(pt->min < 0 || pt->max < 0);
+ range->min = 0;
+
+ switch (pt->type) {
+ case NLA_U8:
+ range->max = U8_MAX;
+ break;
+ case NLA_U16:
+ range->max = U16_MAX;
+ break;
+ case NLA_U32:
+ range->max = U32_MAX;
+ break;
+ case NLA_U64:
+ case NLA_MSECS:
+ range->max = U64_MAX;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
+
switch (pt->validation_type) {
case NLA_VALIDATE_RANGE:
range->min = pt->min;
range->max = pt->max;
break;
case NLA_VALIDATE_RANGE_PTR:
- range = pt->range;
+ *range = *pt->range;
break;
case NLA_VALIDATE_MIN:
range->min = pt->min;
@@ -122,7 +136,17 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
case NLA_VALIDATE_MAX:
range->max = pt->max;
break;
+ default:
+ break;
}
+}
+
+static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
+ const struct nlattr *nla,
+ struct netlink_ext_ack *extack)
+{
+ struct netlink_range_validation range;
+ u64 value;
switch (pt->type) {
case NLA_U8:
@@ -142,7 +166,9 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
return -EINVAL;
}
- if (value < range->min || value > range->max) {
+ nla_get_range_unsigned(pt, &range);
+
+ if (value < range.min || value > range.max) {
NL_SET_ERR_MSG_ATTR(extack, nla,
"integer out of range");
return -ERANGE;
@@ -151,15 +177,30 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
return 0;
}
-static int nla_validate_int_range_signed(const struct nla_policy *pt,
-const struct nlattr *nla,
-struct netlink_ext_ack *extack)
+void nla_get_range_signed(const struct nla_policy *pt,
+ struct netlink_range_validation_signed *range)
{
- struct netlink_range_validation_signed _range = {
- .min = S64_MIN,
- .max = S64_MAX,
- }, *range = &_range;
- s64 value;
+ switch (pt->type) {
+ case NLA_S8:
+ range->min = S8_MIN;
+ range->max = S8_MAX;
+ break;
+ case NLA_S16:
+ range->min = S16_MIN;
+ range->max = S16_MAX;
+ break;
+ case NLA_S32:
+ range->min = S32_MIN;
+ range->max = S32_MAX;
+ break;
+ case NLA_S64:
+ range->min = S64_MIN;
+ range->max = S64_MAX;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
switch (pt->validation_type) {
case NLA_VALIDATE_RANGE:
@@ -167,7 +208,7 @@
[PATCH 4/6] netlink: remove NLA_EXACT_LEN_WARN
From: Johannes Berg
Use a validation type instead, so we can later expose
the NLA_* values to userspace for policy descriptions.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 15 ---
lib/nlattr.c | 16 ++--
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 2a75927d368a..0a02d25fb163 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -182,7 +182,6 @@ enum {
NLA_BITFIELD32,
NLA_REJECT,
NLA_EXACT_LEN,
- NLA_EXACT_LEN_WARN,
NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -204,6 +203,7 @@ enum nla_policy_validation {
NLA_VALIDATE_MAX,
NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
+ NLA_VALIDATE_WARN_TOO_LONG,
};
/**
@@ -237,10 +237,10 @@ enum nla_policy_validation {
* just like "All other"
*NLA_BITFIELD32 Unused
*NLA_REJECT Unused
- *NLA_EXACT_LENAttribute must have exactly this length, otherwise
- * it is rejected.
- *NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
- * is logged if it is longer, shorter is rejected.
+ *NLA_EXACT_LENAttribute should have exactly this length, otherwise
+ * it is rejected or warned about, the latter happening
+ * if and only if the `validation_type' is set to
+ * NLA_VALIDATE_WARN_TOO_LONG.
*NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
@@ -353,8 +353,9 @@ struct nla_policy {
};
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
-#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
- .len = _len }
+#define NLA_POLICY_EXACT_LEN_WARN(_len) \
+ { .type = NLA_EXACT_LEN, .len = _len, \
+ .validation_type = NLA_VALIDATE_WARN_TOO_LONG, }
#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index c8789de96046..05761d2a74cc 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -245,7 +245,9 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
BUG_ON(pt->type > NLA_TYPE_MAX);
if ((nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) ||
- (pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) {
+ (pt->type == NLA_EXACT_LEN &&
+pt->validation_type == NLA_VALIDATE_WARN_TOO_LONG &&
+attrlen != pt->len)) {
pr_warn_ratelimited("netlink: '%s': attribute type %d has an
invalid length.\n",
current->comm, type);
if (validate & NL_VALIDATE_STRICT_ATTRS) {
@@ -256,11 +258,6 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
switch (pt->type) {
- case NLA_EXACT_LEN:
- if (attrlen != pt->len)
- goto out_err;
- break;
-
case NLA_REJECT:
if (extack && pt->reject_message) {
NL_SET_BAD_ATTR(extack, nla);
@@ -373,6 +370,13 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
goto out_err;
break;
+ case NLA_EXACT_LEN:
+ if (pt->validation_type != NLA_VALIDATE_WARN_TOO_LONG) {
+ if (attrlen != pt->len)
+ goto out_err;
+ break;
+ }
+ /* fall through */
default:
if (pt->len)
minlen = pt->len;
--
2.17.2
[PATCH 3/6] netlink: allow NLA_MSECS to have range validation
From: Johannes Berg
Since NLA_MSECS is really equivalent to NLA_U64, allow
it to have range validation as well.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 --
lib/nlattr.c | 2 ++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 66cc7591c000..2a75927d368a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -374,7 +374,8 @@ struct nla_policy {
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
#define NLA_ENSURE_UINT_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
- tp == NLA_U32 || tp == NLA_U64) + tp)
+ tp == NLA_U32 || tp == NLA_U64 || \
+ tp == NLA_MSECS) + tp)
#define NLA_ENSURE_SINT_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_S16 || \
tp == NLA_S32 || tp == NLA_S64) + tp)
@@ -382,7 +383,8 @@ struct nla_policy {
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 || \
tp == NLA_S16 || tp == NLA_U16 || \
tp == NLA_S32 || tp == NLA_U32 || \
- tp == NLA_S64 || tp == NLA_U64) + tp)
+ tp == NLA_S64 || tp == NLA_U64 || \
+ tp == NLA_MSECS) + tp)
#define NLA_ENSURE_NO_VALIDATION_PTR(tp) \
(__NLA_ENSURE(tp != NLA_BITFIELD32 && \
tp != NLA_REJECT && \
diff --git a/lib/nlattr.c b/lib/nlattr.c
index b549b290d3fa..c8789de96046 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -135,6 +135,7 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
value = nla_get_u32(nla);
break;
case NLA_U64:
+ case NLA_MSECS:
value = nla_get_u64(nla);
break;
default:
@@ -211,6 +212,7 @@ static int nla_validate_int_range(const struct nla_policy
*pt,
case NLA_U16:
case NLA_U32:
case NLA_U64:
+ case NLA_MSECS:
return nla_validate_int_range_unsigned(pt, nla, extack);
case NLA_S8:
case NLA_S16:
--
2.17.2
[PATCH 1/6] netlink: remove type-unsafe validation_data pointer
From: Johannes Berg
In the netlink policy, we currently have a void *validation_data
that's pointing to different things:
* a u32 value for bitfield32,
* the netlink policy for nested/nested array
* the string for NLA_REJECT
Remove the pointer and place appropriate type-safe items in the
union instead.
While at it, completely dissolve the pointer for the bitfield32
case and just put the value there directly.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 55 ---
lib/nlattr.c | 20
net/sched/act_api.c | 4 +---
3 files changed, 42 insertions(+), 37 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 24dbf8bb695a..0379fdc3b610 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -217,7 +217,7 @@ enum nla_policy_validation {
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
- * validation_data is also used, for the max attr
+ * nested_policy is also used, for the max attr
* number in the nested policy.
*NLA_U8, NLA_U16,
*NLA_U32, NLA_U64,
@@ -235,27 +235,25 @@ enum nla_policy_validation {
*NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
- * Meaning of `validation_data' field:
+ * Meaning of validation union:
*NLA_BITFIELD32 This is a 32-bit bitmap/bitselector attribute and
- * validation data must point to a u32 value of valid
- * flags
- *NLA_REJECT This attribute is always rejected and validation
data
+ * `bitfield32_valid' is the u32 value of valid flags
+ *NLA_REJECT This attribute is always rejected and
`reject_message'
* may point to a string to report as the error instead
* of the generic one in extended ACK.
- *NLA_NESTED Points to a nested policy to validate, must also set
- * `len' to the max attribute number.
+ *NLA_NESTED `nested_policy' to a nested policy to validate, must
+ * also set `len' to the max attribute number. Use the
+ * provided NLA_POLICY_NESTED() macro.
* Note that nla_parse() will validate, but of course
not
* parse, the nested sub-policies.
- *NLA_NESTED_ARRAY Points to a nested policy to validate, must also set
- * `len' to the max attribute number. The difference to
- * NLA_NESTED is the structure - NLA_NESTED has the
- * nested attributes directly inside, while an array
has
- * the nested attributes at another level down and the
- * attributes directly in the nesting don't matter.
- *All otherUnused - but note that it's a union
- *
- * Meaning of `min' and `max' fields, use via NLA_POLICY_MIN, NLA_POLICY_MAX
- * and NLA_POLICY_RANGE:
+ *NLA_NESTED_ARRAY `nested_policy' points to a nested policy to
validate,
+ * must also set `len' to the max attribute number. Use
+ * the provided NLA_POLICY_NESTED_ARRAY() macro.
+ * The difference to NLA_NESTED is the structure:
+ * NLA_NESTED has the nested attributes directly inside
+ * while an array has the nested attributes at another
+ * level down and the attribute types directly in the
+ * nesting don't matter.
*NLA_U8,
*NLA_U16,
*NLA_U32,
@@ -263,14 +261,16 @@ enum nla_policy_validation {
*NLA_S8,
*NLA_S16,
*NLA_S32,
- *NLA_S64 These are used depending on the validation_type
- * field, if that is min/max/range then the minimum,
- * maximum and both are used (respectively) to check
+ *NLA_S64 The `min' and `max' fields are used depending on the
+ * validation_type field, if that is min/max/range then
+ * the min, max or both are used (respectively) to
check
* the value of the integer attribute.
* Note that in the interest of code simplicity and
* struct size both limits are s16, so you cannot
* enforce a range that doesn't fall within the range
* of s16 - do that as usual in th
[PATCH 2/6] netlink: extend policy range validation
From: Johannes Berg
Using a pointer to a struct indicating the min/max values,
extend the ability to do range validation for arbitrary
values. Small values in the s16 range can be kept in the
policy directly.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 45 +
lib/nlattr.c | 112 ++
2 files changed, 136 insertions(+), 21 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0379fdc3b610..66cc7591c000 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -189,11 +189,20 @@ enum {
#define NLA_TYPE_MAX (__NLA_TYPE_MAX - 1)
+struct netlink_range_validation {
+ u64 min, max;
+};
+
+struct netlink_range_validation_signed {
+ s64 min, max;
+};
+
enum nla_policy_validation {
NLA_VALIDATE_NONE,
NLA_VALIDATE_RANGE,
NLA_VALIDATE_MIN,
NLA_VALIDATE_MAX,
+ NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
};
@@ -271,6 +280,22 @@ enum nla_policy_validation {
* of s16 - do that as usual in the code instead.
* Use the NLA_POLICY_MIN(), NLA_POLICY_MAX() and
* NLA_POLICY_RANGE() macros.
+ *NLA_U8,
+ *NLA_U16,
+ *NLA_U32,
+ *NLA_U64 If the validation_type field instead is set to
+ * NLA_VALIDATE_RANGE_PTR, `range' must be a pointer
+ * to a struct netlink_range_validation that indicates
+ * the min/max values.
+ * Use NLA_POLICY_FULL_RANGE().
+ *NLA_S8,
+ *NLA_S16,
+ *NLA_S32,
+ *NLA_S64 If the validation_type field instead is set to
+ * NLA_VALIDATE_RANGE_PTR, `range_signed' must be a
+ * pointer to a struct netlink_range_validation_signed
+ * that indicates the min/max values.
+ * Use NLA_POLICY_FULL_RANGE_SIGNED().
*All otherUnused - but note that it's a union
*
* Meaning of `validate' field, use via NLA_POLICY_VALIDATE_FN:
@@ -299,6 +324,8 @@ struct nla_policy {
const u32 bitfield32_valid;
const char *reject_message;
const struct nla_policy *nested_policy;
+ struct netlink_range_validation *range;
+ struct netlink_range_validation_signed *range_signed;
struct {
s16 min, max;
};
@@ -345,6 +372,12 @@ struct nla_policy {
{ .type = NLA_BITFIELD32, .bitfield32_valid = valid }
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
+#define NLA_ENSURE_UINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
+ tp == NLA_U32 || tp == NLA_U64) + tp)
+#define NLA_ENSURE_SINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_S8 || tp == NLA_S16 || \
+ tp == NLA_S32 || tp == NLA_S64) + tp)
#define NLA_ENSURE_INT_TYPE(tp)\
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 || \
tp == NLA_S16 || tp == NLA_U16 || \
@@ -363,6 +396,18 @@ struct nla_policy {
.max = _max \
}
+#define NLA_POLICY_FULL_RANGE(tp, _range) {\
+ .type = NLA_ENSURE_UINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_RANGE_PTR, \
+ .range = _range,\
+}
+
+#define NLA_POLICY_FULL_RANGE_SIGNED(tp, _range) { \
+ .type = NLA_ENSURE_SINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_RANGE_PTR, \
+ .range = _range,\
+}
+
#define NLA_POLICY_MIN(tp, _min) { \
.type = NLA_ENSURE_INT_TYPE(tp),\
.validation_type = NLA_VALIDATE_MIN,\
diff --git a/lib/nlattr.c b/lib/nlattr.c
index c546db7c72dd..b549b290d3fa 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,17 +96,33 @@ static int nla_validate_array(const struct nlattr *head,
int len, int maxtype,
return 0;
}
-static int nla_validate_int_range(const struct nla_policy *pt,
- const struct nlattr *nla,
- struct netlink_ext_ack *extack)
+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 ||
-
[PATCH 6/6] netlink: add infrastructure to expose policies to userspace
From: Johannes Berg
Add, and use in generic netlink, helpers to dump out a netlink
policy to userspace, including all the range validation data,
nested policies etc.
This lets userspace discover what the kernel understands.
For families/commands other than generic netlink, the helpers
need to be used directly in an appropriate command, or we can
add some infrastructure (a new netlink family) that those can
register their policies with for introspection. I'm not that
familiar with non-generic netlink, so that's left out for now.
The data exposed to userspace also includes min and max length
for binary/string data, I've done that instead of letting the
userspace tools figure out whether min/max is intended based
on the type so that we can extend this later in the kernel, we
might want to just use the range data for example.
Because of this, I opted to not directly expose the NLA_*
values, even if some of them are already exposed via BPF, as
with min/max length we don't need to have different types here
for NLA_BINARY/NLA_MIN_LEN/NLA_EXACT_LEN, we just make them
all NL_ATTR_TYPE_BINARY with min/max length optionally set.
Similarly, we don't really need NLA_MSECS, and perhaps can
remove it in the future - but not if we encode it into the
userspace API now. It gets mapped to NL_ATTR_TYPE_U64 here.
Note that the exposing here corresponds to the strict policy
interpretation, and NLA_UNSPEC items are omitted entirely.
To get those, change them to NLA_MIN_LEN which behaves in
exactly the same way, but is exposed.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +
include/uapi/linux/genetlink.h | 2 +
include/uapi/linux/netlink.h | 103 +++
net/netlink/Makefile | 2 +-
net/netlink/genetlink.c| 77 +
net/netlink/policy.c | 308 +
6 files changed, 497 insertions(+), 1 deletion(-)
create mode 100644 net/netlink/policy.c
diff --git a/include/net/netlink.h b/include/net/netlink.h
index aab0a30021b6..ca9dc2e6500e 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1880,4 +1880,10 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
void nla_get_range_signed(const struct nla_policy *pt,
struct netlink_range_validation_signed *range);
+int netlink_policy_dump_start(const struct nla_policy *policy,
+ unsigned int maxtype,
+ unsigned long *state);
+bool netlink_policy_dump_loop(unsigned long *state);
+int netlink_policy_dump_write(struct sk_buff *skb, unsigned long state);
+
#endif
diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
index 877f7fa95466..9c0636ec2286 100644
--- a/include/uapi/linux/genetlink.h
+++ b/include/uapi/linux/genetlink.h
@@ -48,6 +48,7 @@ enum {
CTRL_CMD_NEWMCAST_GRP,
CTRL_CMD_DELMCAST_GRP,
CTRL_CMD_GETMCAST_GRP, /* unused */
+ CTRL_CMD_GETPOLICY,
__CTRL_CMD_MAX,
};
@@ -62,6 +63,7 @@ enum {
CTRL_ATTR_MAXATTR,
CTRL_ATTR_OPS,
CTRL_ATTR_MCAST_GROUPS,
+ CTRL_ATTR_POLICY,
__CTRL_ATTR_MAX,
};
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 0a4d73317759..eac8a6a648ea 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -249,4 +249,107 @@ struct nla_bitfield32 {
__u32 selector;
};
+/*
+ * policy descriptions - it's specific to each family how this is used
+ * Normally, it should be retrieved via a dump inside another attribute
+ * specifying where it applies.
+ */
+
+/**
+ * enum netlink_attribute_type - type of an attribute
+ * @NL_ATTR_TYPE_INVALID: unused
+ * @NL_ATTR_TYPE_FLAG: flag attribute (present/not present)
+ * @NL_ATTR_TYPE_U8: 8-bit unsigned attribute
+ * @NL_ATTR_TYPE_U16: 16-bit unsigned attribute
+ * @NL_ATTR_TYPE_U32: 32-bit unsigned attribute
+ * @NL_ATTR_TYPE_U64: 64-bit unsigned attribute
+ * @NL_ATTR_TYPE_S8: 8-bit signed attribute
+ * @NL_ATTR_TYPE_S16: 16-bit signed attribute
+ * @NL_ATTR_TYPE_S32: 32-bit signed attribute
+ * @NL_ATTR_TYPE_S64: 64-bit signed attribute
+ * @NL_ATTR_TYPE_BINARY: binary data, min/max length may be specified
+ * @NL_ATTR_TYPE_STRING: string, min/max length may be specified
+ * @NL_ATTR_TYPE_NUL_STRING: NUL-terminated string,
+ * min/max length may be specified
+ * @NL_ATTR_TYPE_NESTED: nested, i.e. the content of this attribute
+ * consists of sub-attributes. The nested policy and maxtype
+ * inside may be specified.
+ * @NL_ATTR_TYPE_NESTED_ARRAY: nested array, i.e. the content of this
+ * attribute contains sub-attributes whose type is irrelevant
+ * (just used to separate the array entries) and each such array
+ * entry has attributes again, the policy for those inner ones
+ * and the corresponding maxtype may be specified.
+ * @NL_ATTR_TYPE_BITFIELD32: &struct nla_bitfield32 attribute
[PATCH 0/6] netlink: expose policies to userspace
Make some cleanups in policy validation, and add tools to expose the policies to userspace. This lets userspace discover what the appropriate data for an element is. Note that NLA_UNSPEC items are omitted as they're rejected in strict validation mode so their policy entries should be changed to NLA_MIN_LEN - this is just a representation change. Eventually, I'd like to add a separation between input and output policies (currently only the input policy is exposed), but that needs some extra work to add "overrides". johannes
[PATCH v2 5/5] genetlink: optionally validate strictly/dumps
From: Johannes Berg
Add options to strictly validate messages and dump messages,
sometimes perhaps validating dump messages non-strictly may
be required, so add an option for that as well.
Since none of this can really be applied to existing commands,
set the options everwhere using the following spatch:
@@
identifier ops;
expression X;
@@
struct genl_ops ops[] = {
...,
{
.cmd = X,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
...
},
...
};
For new commands one should just not copy the .validate 'opt-out'
flags and thus get strict validation.
Signed-off-by: Johannes Berg
---
drivers/block/nbd.c | 4 +
drivers/net/gtp.c| 3 +
drivers/net/ieee802154/mac802154_hwsim.c | 6 ++
drivers/net/macsec.c | 10 +++
drivers/net/team/team.c | 4 +
drivers/net/wireless/mac80211_hwsim.c| 6 ++
drivers/target/target_core_user.c| 4 +
fs/dlm/netlink.c | 1 +
include/net/genetlink.h | 7 ++
kernel/taskstats.c | 2 +
net/batman-adv/netlink.c | 18
net/core/devlink.c | 38 +
net/core/drop_monitor.c | 3 +
net/hsr/hsr_netlink.c| 2 +
net/ieee802154/nl802154.c| 29 +++
net/ipv4/fou.c | 3 +
net/ipv4/tcp_metrics.c | 2 +
net/ipv6/ila/ila_main.c | 4 +
net/ipv6/seg6.c | 4 +
net/l2tp/l2tp_netlink.c | 9 ++
net/ncsi/ncsi-netlink.c | 6 ++
net/netfilter/ipvs/ip_vs_ctl.c | 16
net/netlabel/netlabel_calipso.c | 4 +
net/netlabel/netlabel_cipso_v4.c | 4 +
net/netlabel/netlabel_mgmt.c | 8 ++
net/netlabel/netlabel_unlabeled.c| 8 ++
net/netlink/genetlink.c | 29 ++-
net/nfc/netlink.c| 19 +
net/openvswitch/conntrack.c | 3 +
net/openvswitch/datapath.c | 13 +++
net/openvswitch/meter.c | 4 +
net/psample/psample.c| 1 +
net/smc/smc_pnet.c | 4 +
net/tipc/netlink.c | 21 +
net/tipc/netlink_compat.c| 1 +
net/wimax/stack.c| 4 +
net/wireless/nl80211.c | 104 +++
37 files changed, 405 insertions(+), 3 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 91cff60f2839..569c92520150 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2003,18 +2003,22 @@ static int nbd_genl_reconfigure(struct sk_buff *skb,
struct genl_info *info)
static const struct genl_ops nbd_connect_genl_ops[] = {
{
.cmd= NBD_CMD_CONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_connect,
},
{
.cmd= NBD_CMD_DISCONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_disconnect,
},
{
.cmd= NBD_CMD_RECONFIGURE,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_reconfigure,
},
{
.cmd= NBD_CMD_STATUS,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_status,
},
};
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index c06e31747288..eaf4311b4004 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1270,16 +1270,19 @@ static const struct nla_policy gtp_genl_policy[GTPA_MAX
+ 1] = {
static const struct genl_ops gtp_genl_ops[] = {
{
.cmd = GTP_CMD_NEWPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_new_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_DELPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_del_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_GETPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_get_pdp,
.dumpit = gtp_genl_dump_pdp,
.flags = GENL_ADMIN_PERM,
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c
b/drivers/net/ieee802154/mac802154_hwsim.c
index fa323e9ab616..a69dc9ed75fc 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -592,31 +592,37 @@ static co
[PATCH v2 3/5] netlink: re-add parse/validate functions in strict mode
From: Johannes Berg
This re-adds the parse and validate functions like nla_parse()
that are now actually strict after the previous rename and were
just split out to make sure everything is converted (and if not
compilation of the previous patch would fail.)
Signed-off-by: Johannes Berg
---
include/net/genetlink.h | 19 +
include/net/netlink.h | 87 +
2 files changed, 106 insertions(+)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 897cdba13569..68de579cfe5e 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -183,6 +183,25 @@ static inline int genlmsg_parse_deprecated(const struct
nlmsghdr *nlh,
policy, NL_VALIDATE_LIBERAL, extack);
}
+/**
+ * genlmsg_parse - parse attributes of a genetlink message
+ * @nlh: netlink message header
+ * @family: genetlink message family
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ */
+static inline int genlmsg_parse(const struct nlmsghdr *nlh,
+ const struct genl_family *family,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nlmsg_parse(nlh, family->hdrsize + GENL_HDRLEN, tb, maxtype,
+policy, NL_VALIDATE_STRICT, extack);
+}
+
/**
* genl_dump_check_consistent - check if sequence is consistent and advertise
if not
* @cb: netlink callback structure that stores the sequence number
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 21196c6ece91..ac0af9bac0b3 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -538,6 +538,31 @@ nlmsg_next(const struct nlmsghdr *nlh, int *remaining)
return (struct nlmsghdr *) ((unsigned char *) nlh + totlen);
}
+/**
+ * nla_parse - Parse a stream of attributes into a tb buffer
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @policy: validation policy
+ * @extack: extended ACK pointer
+ *
+ * Parses a stream of attributes and stores a pointer to each attribute in
+ * the tb array accessible via the attribute type. Attributes with a type
+ * exceeding maxtype will be rejected, policy must be specified, attributes
+ * will be validated in the strictest way possible.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+static inline int nla_parse(struct nlattr **tb, int maxtype,
+ const struct nlattr *head, int len,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, head, len, policy,
+ NL_VALIDATE_STRICT, extack);
+}
+
/**
* nla_parse_deprecated - Parse a stream of attributes into a tb buffer
* @tb: destination array with maxtype+1 elements
@@ -617,6 +642,27 @@ static inline int __nlmsg_parse(const struct nlmsghdr
*nlh, int hdrlen,
extack);
}
+/**
+ * nlmsg_parse - parse attributes of a netlink message
+ * @nlh: netlink message header
+ * @hdrlen: length of family specific header
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @validate: validation strictness
+ * @extack: extended ACK report struct
+ *
+ * See nla_parse()
+ */
+static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, nlmsg_attrdata(nlh, hdrlen),
+ nlmsg_attrlen(nlh, hdrlen), policy,
+ NL_VALIDATE_STRICT, extack);
+}
+
/**
* nlmsg_parse_deprecated - parse attributes of a netlink message
* @nlh: netlink message header
@@ -695,6 +741,28 @@ static inline int nla_validate_deprecated(const struct
nlattr *head, int len,
extack);
}
+/**
+ * nla_validate - Validate a stream of attributes
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @validate: validation strictness
+ * @extack: extended ACK report struct
+ *
+ * Validates all attributes in the specified attribute stream against the
+ * specified policy. Validation is done in strict mode.
+ * See documenation of struct nla_policy for more details.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+static inline int nla_validate(const struct nlattr *head, int len, int maxt
[PATCH v2 0/5] strict netlink validation
Here's a respin, with the following changes: * change message when rejecting unknown attribute types (David Ahern) * drop nl80211 patch - I'll apply it separately * remove NL_VALIDATE_POLICY - we have a lot of calls to nla_parse() that really should be without a policy as it has previously been validated - need to find a good way to handle this later * include the correct generic netlink change (d'oh, sorry) johannes
[PATCH v2 4/5] netlink: add strict parsing for future attributes
From: Johannes Berg
Unfortunately, we cannot add strict parsing for all attributes, as
that would break existing userspace. We currently warn about it, but
that's about all we can do.
For new attributes, however, the story is better: nobody is using
them, so we can reject bad sizes.
Also, for new attributes, we need not accept them when the policy
doesn't declare their usage.
David Ahern and I went back and forth on how to best encode this, and
the best way we found was to have a "boundary type", from which point
on new attributes have all possible validation applied, and NLA_UNSPEC
is rejected.
As we didn't want to add another argument to all functions that get a
netlink policy, the workaround is to encode that boundary in the first
entry of the policy array (which is for type 0 and thus probably not
really valid anyway). I put it into the validation union for the rare
possibility that somebody is actually using attribute 0, which would
continue to work fine unless they tried to use the extended validation,
which isn't likely. We also didn't find any in-tree users with type 0.
The reason for setting the "start strict here" attribute is that we
never really need to start strict from 0, which is invalid anyway (or
in legacy families where that isn't true, it cannot be set to strict),
so we can thus reserve the value 0 for "don't do this check" and don't
have to add the tag to all policies right now.
Thus, policies can now opt in to this validation, which we should do
for all existing policies, at least when adding new attributes.
Note that entirely *new* policies won't need to set it, as the use
of that should be using nla_parse()/nlmsg_parse() etc. which anyway
do fully strict validation now, regardless of this.
So in effect, this patch only covers the "existing command with new
attribute" case.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 18 ++
lib/nlattr.c | 4
2 files changed, 22 insertions(+)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index ac0af9bac0b3..24dbf8bb695a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -299,6 +299,24 @@ struct nla_policy {
};
int (*validate)(const struct nlattr *attr,
struct netlink_ext_ack *extack);
+ /* This entry is special, and used for the attribute at index 0
+* only, and specifies special data about the policy, namely it
+* specifies the "boundary type" where strict length validation
+* starts for any attribute types >= this value, also, strict
+* nesting validation starts here.
+*
+* Additionally, it means that NLA_UNSPEC is actually NLA_REJECT
+* for any types >= this, so need to use NLA_MIN_LEN to get the
+* previous pure { .len = xyz } behaviour. The advantage of this
+* is that types not specified in the policy will be rejected.
+*
+* For completely new families it should be set to 1 so that the
+* validation is enforced for all attributes. For existing ones
+* it should be set at least when new attributes are added to
+* the enum used by the policy, and be set to the new value that
+* was added to enforce strict validation from thereon.
+*/
+ u16 strict_start_type;
};
};
diff --git a/lib/nlattr.c b/lib/nlattr.c
index af0f8b0309c6..29f6336e2422 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -158,10 +158,14 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
const struct nla_policy *policy, unsigned int validate,
struct netlink_ext_ack *extack)
{
+ u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
int err = -ERANGE;
+ if (strict_start_type && type >= strict_start_type)
+ validate |= NL_VALIDATE_STRICT;
+
if (type <= 0 || type > maxtype)
return 0;
--
2.17.2
[PATCH v2 1/5] netlink: add NLA_MIN_LEN
From: Johannes Berg
Rather than using NLA_UNSPEC for this type of thing, use NLA_MIN_LEN
so we can make NLA_UNSPEC be NLA_REJECT under certain conditions for
future attributes.
While at it, also use NLA_EXACT_LEN for the struct example.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +-
lib/nlattr.c | 9 -
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 23f27b0b3cef..06f8605b740c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -183,6 +183,7 @@ enum {
NLA_REJECT,
NLA_EXACT_LEN,
NLA_EXACT_LEN_WARN,
+ NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -212,6 +213,7 @@ enum nla_policy_validation {
*NLA_NUL_STRING Maximum length of string (excluding NUL)
*NLA_FLAG Unused
*NLA_BINARY Maximum length of attribute payload
+ *NLA_MIN_LEN Minimum length of attribute payload
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
@@ -230,6 +232,7 @@ enum nla_policy_validation {
* it is rejected.
*NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
* is logged if it is longer, shorter is rejected.
+ *NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
* Meaning of `validation_data' field:
@@ -281,7 +284,7 @@ enum nla_policy_validation {
* static const struct nla_policy my_policy[ATTR_MAX+1] = {
* [ATTR_FOO] = { .type = NLA_U16 },
* [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
- * [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * [ATTR_BAZ] = { .type = NLA_EXACT_LEN, .len = sizeof(struct mystruct) },
* [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data = &myvalidflags
},
* };
*/
@@ -302,6 +305,7 @@ struct nla_policy {
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
.len = _len }
+#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index d26de6156b97..465c9e8ef8a5 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -278,10 +278,17 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
}
break;
+
+ case NLA_UNSPEC:
+ case NLA_MIN_LEN:
+ if (attrlen < pt->len)
+ goto out_err;
+ break;
+
default:
if (pt->len)
minlen = pt->len;
- else if (pt->type != NLA_UNSPEC)
+ else
minlen = nla_attr_minlen[pt->type];
if (attrlen < minlen)
--
2.17.2
Re: cellular modem driver APIs
On Sun, 2019-04-14 at 13:09 -0600, Subash Abhinov Kasiviswanathan wrote: > > Hmm, not sure I understand this. If you do RPS/RSS then that's a > > hardware function, and the netdev doesn't really come into play > > immediately? If the underlying driver directly deals with multiple > > netdevs that's actually an *advantage*, no? > > RPS is in SW only though. I'm sort of expecting that to change in 5G, but I guess designs will vary. > I think this shouldn't be a concern if the existing underlying netdev > model could co-exist with the new framework. Indeed. I'm just thinking that maybe a new framework shouldn't *force* this model. johannes
Re: cellular modem driver APIs
On Fri, 2019-04-12 at 16:27 +0200, Bjørn Mork wrote: > Johannes Berg writes: > > On Wed, 2019-04-10 at 21:54 -0600, Subash Abhinov Kasiviswanathan wrote: > > > > > These packets will be processed as raw IP muxed frames on the PC as > > > well, not as ethernet though. > > > > But in order to transport them, they're always encapsulated in ethernet? > > No. There is no ethernet encapsulation of QMAP muxed frames over USB. > > Same goes for MBIM, BTW. The cdc_mbim driver adds ethernet encapsulation > on the host side, but that is just an unfortunate design decision based > on the flawed assumption that ethernet interfaces are easier to relate > to. Yes yes, sorry. I snipped too much - we were talking here in the context of capturing the QMAP muxed frames remotely to another system, in particular the strange bridge mode rmnet has. And I said up the thread: > Yeah, I get it, it's just done in a strange way. You'd think adding a > tcpdump or some small application that just resends the packets directly > received from the underlying "real_dev" using a ptype_all socket would > be sufficient? Though perhaps not quite the same performance, but then > you could easily not use an application but a dev_add_pack() thing? Or > probably even tc's mirred? > > And to extend that thought, tc's ife action would let you encapsulate > the things you have in ethernet headers... I think. So basically yes, I know we (should) have only IP frames on the session netdevs, and only QMAP-muxed frames on the underlying netdev (assuming it exists), but I don't see the point in having kernel code to add ethernet headers to send it over some other netdev - you can trivially solve all of that in userspace or quite possibly even in the kernel with existing (tc) infrastructure. johannes
Re: cellular modem driver APIs
On Wed, 2019-04-10 at 21:54 -0600, Subash Abhinov Kasiviswanathan wrote: > > > We need raw IP frames from a embedded device transmitted to a PC > > > and vice versa. > > > > Sure. But you just need to encap them in some kind of ethernet frame to > > transport them on the wire, but don't really need to care much about > > how. > > > > These packets will be processed as raw IP muxed frames on the PC as > well, not as ethernet though. But in order to transport them, they're always encapsulated in ethernet? > Currently, iproute2 can be used to add the underlying dev as real_dev > and create rmnet links over it (ip link add link rmnet_ipa0 name rmnet0 type > rmnet mux_id 1). Would this continue to work if - > 1. the rmnet library were to be included directly as part of the > underlying driver itself > 2. there is no underlying dev at all Yeah, this is the big question. If there's no underlying netdev at all, then no, it wouldn't work. Though, it could be "faked" in a sense, by doing two things: a) having the driver/infra always create a default channel interface, say mux_id 0? b) by treating ip link add link rmnet_ipa0 name rmnet0 type rmnet mux_id 1 as not creating a new netdev *on top of* but rather *as sibling to* "rmnet0". The alternative is to just keep rmnet and the underlying driver, but tie them together a little more closely so that they can - together - register with a hypothetical new WWAN framework. See - even if we create such a framework in a way that it doesn't require an underlying netdev (which I think is the better choice for various reasons, particularly related to 5G), then there's nothing that says that you *cannot* have it anyway and keep the exact same rmnet + underlying driver model. > One additional use of underlying netdev is for RPS. > This helps to separate out the processing of the underlying netdev and > rmnet. If rmnet were to be converted into a library, we would still need > this functionality. Hmm, not sure I understand this. If you do RPS/RSS then that's a hardware function, and the netdev doesn't really come into play immediately? If the underlying driver directly deals with multiple netdevs that's actually an *advantage*, no? johannes
Re: cellular modem driver APIs
On Sat, 2019-04-06 at 19:20 +0200, Daniele Palmas wrote: > > the qmi_wwan sysfs qmap feature, being very easy to use, is serving > well for me and customers of the company I work for (mainly directly > with libqmi, not ModemManager), Yeah, I don't doubt this. In fact, we could arguably provide the same through some kind of common stack. I sort of have to believe though that it's even more limiting that we *don't* have any common interfaces, since that means you'll have to rewrite your software even if you just want to switch to a different Qualcomm modem (e.g. PCIe instead of USB). > but I understand the need to have a > common framework and will gladly test and provide feedback for any new > development related to this. Thanks :-) johannes
Re: cellular modem driver APIs
On Thu, 2019-04-04 at 22:45 -0600, Subash Abhinov Kasiviswanathan wrote: > On 2019-04-04 14:38, Johannes Berg wrote: > > Hi, > > > > > The normal mode of operation of rmnet is using the rmnet netdevices > > > in an embedded device. > > > > Sure. Can you say what driver this would typically live on top of? I'm > > actually a bit surprised to find out this isn't really a driver :-) > > > > This needs a physical net device such as IP accelerator > https://lkml.org/lkml/2018/11/7/233 or Modem host interface > https://lkml.org/lkml/2018/4/26/1159 OK. But it means that you have a very specific encapsulation mode on top of the "netdev". I'm still not convinced we should actually make that a netdev, but I'll elaborate elsewhere. > I recall Daniele also managed to get rmnet working with qmi_wwan > (with an additional patch in which I had made qmi_wwan a passthrough) I guess that uses the same encapsulation then, yes, I see it now: qmi_wwan's struct qmimux_hdr and rmnet's struct rmnet_map_header are very similar. Btw, I see that struct rmnet_map_header uses a bitfield - this seems to go down to the device so probably will not work right on both big and little endian. > > Yeah, I get it, it's just done in a strange way. You'd think adding a > > tcpdump or some small application that just resends the packets > > directly > > received from the underlying "real_dev" using a ptype_all socket would > > be sufficient? Though perhaps not quite the same performance, but then > > you could easily not use an application but a dev_add_pack() thing? Or > > probably even tc's mirred? > > > > And to extend that thought, tc's ife action would let you encapsulate > > the things you have in ethernet headers... I think. > > We need raw IP frames from a embedded device transmitted to a PC > and vice versa. Sure. But you just need to encap them in some kind of ethernet frame to transport them on the wire, but don't really need to care much about how. > Yes, the underlying netdev itself cannot do much on its own as network > stack wont be able to decipher the muxed frames. Right. > The operation of rmnet was to be agnostic of the underlying driver. > The netdev model was chosen for it since it was easy to have a > rx_handler attach to the netdevice exposed by any of those drivers. I really do think it's the wrong model though: 1. The network stack cannot do anything with the muxed data, and so there's no value from that perspective in exposing it that way. 2. The rx_handler attach thing is a bit of a red herring - if you have some other abstraction that the driver registers with, you can just say "send packets here" and then demux things properly, without having a netdev. Actually, I'd almost argue that rmnet should've just been a sort of encap/decap library that the drivers like qmi_wwan, rmnet_ipa and mhi use to talk to the device. 3. Having this underlying netdev is actually very limiting. We found this with wifi a long time ago, and I suspect with 5G coming up you'll be in a similar situation. You'll want to do things like multi-queue, different hardware queues for different channels, etc. and muxing it all over a single netdev (and a single queue there) becomes an easily avoidable point of contention. 4. (I thought about something else but it escapes me now) > > Now, OTOH, this loses a bunch of benefits. We may want to be able to > > use > > ethtool to flash a modem, start tcpdump on the underlying netdev > > directly to see everything, etc.? > > > > Yes, we use that underlying netdev to view the muxed raw IP frames in > tcpdump. That's the easiest of all - just make the framework able to add a 'sniffer' netdev that can see all the TX/RX for the other channels. Which I already said, and you asked: > Can you tell how this sniffer netdev works? Well, it's just an abstraction - a netdev that reports as received all frames that the stack or underlying driver sees. > If rmnet needed to create a > separate netdev for monitoring the muxed raw IP frames, wouldnt that > just serve the same purpose as that of underlying netdev. No, it's different in that it's only there for debugging, only reports all the frames the stack/driver saw, and doesn't interfere with your actual operation like the underlying netdev does like I described above. johannes
[RFC] netlink: limit recursion depth in policy validation
From: Johannes Berg
Now that we have nested policies, we can theoretically
recurse forever parsing attributes if a (sub-)policy
refers back to a higher level one. This is a situation
that has happened in nl80211, and we've avoided it there
by not linking it.
Add some code to netlink parsing to limit recursion depth,
allowing us to safely change nl80211 to actually link the
nested policy, which in turn allows some code cleanups.
Signed-off-by: Johannes Berg
---
lib/nlattr.c | 46 +++---
net/wireless/nl80211.c | 10 -
net/wireless/nl80211.h | 2 --
net/wireless/pmsr.c| 3 +--
4 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index baf27844ecc8..bc41d3d96945 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -44,6 +44,20 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
[NLA_S64] = sizeof(s64),
};
+/*
+ * Nested policies might refer back to the original
+ * policy in some cases, and userspace could try to
+ * abuse that and recurse by nesting in the right
+ * ways. Limit recursion to avoid this problem.
+ */
+#define MAX_POLICY_RECURSION_DEPTH 10
+
+static int __nla_validate_parse(const struct nlattr *head, int len, int
maxtype,
+ const struct nla_policy *policy,
+ unsigned int validate,
+ struct netlink_ext_ack *extack,
+ struct nlattr **tb, unsigned int depth);
+
static int validate_nla_bitfield32(const struct nlattr *nla,
const u32 valid_flags_mask)
{
@@ -70,7 +84,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack,
- unsigned int validate)
+ unsigned int validate, unsigned int depth)
{
const struct nlattr *entry;
int rem;
@@ -87,8 +101,9 @@ static int nla_validate_array(const struct nlattr *head, int
len, int maxtype,
return -ERANGE;
}
- ret = __nla_validate(nla_data(entry), nla_len(entry),
-maxtype, policy, validate, extack);
+ ret = __nla_validate_parse(nla_data(entry), nla_len(entry),
+ maxtype, policy, validate, extack,
+ NULL, depth + 1);
if (ret < 0)
return ret;
}
@@ -280,7 +295,7 @@ static int nla_validate_int_range(const struct nla_policy
*pt,
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy, unsigned int validate,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack, unsigned int depth)
{
u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
@@ -375,9 +390,10 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
if (attrlen < NLA_HDRLEN)
goto out_err;
if (pt->nested_policy) {
- err = __nla_validate(nla_data(nla), nla_len(nla),
pt->len,
-pt->nested_policy, validate,
-extack);
+ err = __nla_validate_parse(nla_data(nla), nla_len(nla),
+ pt->len, pt->nested_policy,
+ validate, extack, NULL,
+ depth + 1);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -400,7 +416,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
err = nla_validate_array(nla_data(nla), nla_len(nla),
pt->len, pt->nested_policy,
-extack, validate);
+extack, validate, depth);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -472,11 +488,17 @@ static int __nla_validate_parse(const struct nlattr
*head, int len, int maxtype,
const struct nla_policy *policy,
unsigned int validate,
struct netlink_ext_ack *extack,
- struct nlattr **tb)
+
[RFC 4/7] netlink: allow NLA_MSECS to have range validation
From: Johannes Berg
Since NLA_MSECS is really equivalent to NLA_U64, allow
it to have range validation as well.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 --
lib/nlattr.c | 2 ++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 12de018ec7b8..6c79a1d0e30f 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -374,7 +374,8 @@ struct nla_policy {
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
#define NLA_ENSURE_UINT_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
- tp == NLA_U32 || tp == NLA_U64) + tp)
+ tp == NLA_U32 || tp == NLA_U64 || \
+ tp == NLA_MSECS) + tp)
#define NLA_ENSURE_SINT_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_S16 || \
tp == NLA_S32 || tp == NLA_S64) + tp)
@@ -382,7 +383,8 @@ struct nla_policy {
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 || \
tp == NLA_S16 || tp == NLA_U16 || \
tp == NLA_S32 || tp == NLA_U32 || \
- tp == NLA_S64 || tp == NLA_U64) + tp)
+ tp == NLA_S64 || tp == NLA_U64 || \
+ tp == NLA_MSECS) + tp)
#define NLA_ENSURE_NO_VALIDATION_PTR(tp) \
(__NLA_ENSURE(tp != NLA_BITFIELD32 && \
tp != NLA_REJECT && \
diff --git a/lib/nlattr.c b/lib/nlattr.c
index b11079a9c97e..2e6e6e29efda 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -135,6 +135,7 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
value = nla_get_u32(nla);
break;
case NLA_U64:
+ case NLA_MSECS:
value = nla_get_u64(nla);
break;
default:
@@ -211,6 +212,7 @@ static int nla_validate_int_range(const struct nla_policy
*pt,
case NLA_U16:
case NLA_U32:
case NLA_U64:
+ case NLA_MSECS:
return nla_validate_int_range_unsigned(pt, nla, extack);
case NLA_S8:
case NLA_S16:
--
2.17.2
[RFC 7/7] netlink: add infrastructure to expose policies to userspace
From: Johannes Berg
Add, and use in generic netlink, helpers to dump out a netlink
policy to userspace, including all the range validation data,
nested policies etc.
This lets userspace discover what the kernel understands.
For families/commands other than generic netlink, the helpers
need to be used directly in an appropriate command, or we can
add some infrastructure (a new netlink family) that those can
register their policies with for introspection. I'm not that
familiar with non-generic netlink, so that's left out for now.
The data exposed to userspace also includes min and max length
for binary/string data, I've done that instead of letting the
userspace tools figure out whether min/max is intended based
on the type so that we can extend this later in the kernel, we
might want to just use the range data for example.
Because of this, I opted to not directly expose the NLA_*
values, even if some of them are already exposed via BPF, as
with min/max length we don't need to have different types here
for NLA_BINARY/NLA_MIN_LEN/NLA_EXACT_LEN, we just make them
all NL_ATTR_TYPE_BINARY with min/max length optionally set.
Similarly, we don't really need NLA_MSECS, and perhaps can
remove it in the future - but not if we encode it into the
userspace API now. It gets mapped to NL_ATTR_TYPE_U64 here.
Note that the exposing here corresponds to the strict policy
interpretation, and NLA_UNSPEC items are omitted entirely.
To get those, change them to NLA_MIN_LEN which behaves in
exactly the same way, but is exposed.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +
include/uapi/linux/genetlink.h | 2 +
include/uapi/linux/netlink.h | 103 +++
net/netlink/Makefile | 2 +-
net/netlink/genetlink.c| 77 +
net/netlink/policy.c | 308 +
6 files changed, 497 insertions(+), 1 deletion(-)
create mode 100644 net/netlink/policy.c
diff --git a/include/net/netlink.h b/include/net/netlink.h
index b131dd77390d..0c2231bbadc5 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1884,4 +1884,10 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
void nla_get_range_signed(const struct nla_policy *pt,
struct netlink_range_validation_signed *range);
+int netlink_policy_dump_start(const struct nla_policy *policy,
+ unsigned int maxtype,
+ unsigned long *state);
+bool netlink_policy_dump_loop(unsigned long *state);
+int netlink_policy_dump_write(struct sk_buff *skb, unsigned long state);
+
#endif
diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
index 877f7fa95466..9c0636ec2286 100644
--- a/include/uapi/linux/genetlink.h
+++ b/include/uapi/linux/genetlink.h
@@ -48,6 +48,7 @@ enum {
CTRL_CMD_NEWMCAST_GRP,
CTRL_CMD_DELMCAST_GRP,
CTRL_CMD_GETMCAST_GRP, /* unused */
+ CTRL_CMD_GETPOLICY,
__CTRL_CMD_MAX,
};
@@ -62,6 +63,7 @@ enum {
CTRL_ATTR_MAXATTR,
CTRL_ATTR_OPS,
CTRL_ATTR_MCAST_GROUPS,
+ CTRL_ATTR_POLICY,
__CTRL_ATTR_MAX,
};
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 0a4d73317759..eac8a6a648ea 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -249,4 +249,107 @@ struct nla_bitfield32 {
__u32 selector;
};
+/*
+ * policy descriptions - it's specific to each family how this is used
+ * Normally, it should be retrieved via a dump inside another attribute
+ * specifying where it applies.
+ */
+
+/**
+ * enum netlink_attribute_type - type of an attribute
+ * @NL_ATTR_TYPE_INVALID: unused
+ * @NL_ATTR_TYPE_FLAG: flag attribute (present/not present)
+ * @NL_ATTR_TYPE_U8: 8-bit unsigned attribute
+ * @NL_ATTR_TYPE_U16: 16-bit unsigned attribute
+ * @NL_ATTR_TYPE_U32: 32-bit unsigned attribute
+ * @NL_ATTR_TYPE_U64: 64-bit unsigned attribute
+ * @NL_ATTR_TYPE_S8: 8-bit signed attribute
+ * @NL_ATTR_TYPE_S16: 16-bit signed attribute
+ * @NL_ATTR_TYPE_S32: 32-bit signed attribute
+ * @NL_ATTR_TYPE_S64: 64-bit signed attribute
+ * @NL_ATTR_TYPE_BINARY: binary data, min/max length may be specified
+ * @NL_ATTR_TYPE_STRING: string, min/max length may be specified
+ * @NL_ATTR_TYPE_NUL_STRING: NUL-terminated string,
+ * min/max length may be specified
+ * @NL_ATTR_TYPE_NESTED: nested, i.e. the content of this attribute
+ * consists of sub-attributes. The nested policy and maxtype
+ * inside may be specified.
+ * @NL_ATTR_TYPE_NESTED_ARRAY: nested array, i.e. the content of this
+ * attribute contains sub-attributes whose type is irrelevant
+ * (just used to separate the array entries) and each such array
+ * entry has attributes again, the policy for those inner ones
+ * and the corresponding maxtype may be specified.
+ * @NL_ATTR_TYPE_BITFIELD32: &struct nla_bitfield32 attribute
[RFC 2/7] netlink: remove type-unsafe validation_data pointer
From: Johannes Berg
In the netlink policy, we currently have a void *validation_data
that's pointing to different things:
* a u32 value for bitfield32,
* the netlink policy for nested/nested array
* the string for NLA_REJECT
Remove the pointer and place appropriate type-safe items in the
union instead.
While at it, completely dissolve the pointer for the bitfield32
case and just put the value there directly.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 55 ---
lib/nlattr.c | 20
net/sched/act_api.c | 4 +---
3 files changed, 42 insertions(+), 37 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index a33117e730bb..22984933bc36 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -217,7 +217,7 @@ enum nla_policy_validation {
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
- * validation_data is also used, for the max attr
+ * nested_policy is also used, for the max attr
* number in the nested policy.
*NLA_U8, NLA_U16,
*NLA_U32, NLA_U64,
@@ -235,27 +235,25 @@ enum nla_policy_validation {
*NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
- * Meaning of `validation_data' field:
+ * Meaning of validation union:
*NLA_BITFIELD32 This is a 32-bit bitmap/bitselector attribute and
- * validation data must point to a u32 value of valid
- * flags
- *NLA_REJECT This attribute is always rejected and validation
data
+ * `bitfield32_valid' is the u32 value of valid flags
+ *NLA_REJECT This attribute is always rejected and
`reject_message'
* may point to a string to report as the error instead
* of the generic one in extended ACK.
- *NLA_NESTED Points to a nested policy to validate, must also set
- * `len' to the max attribute number.
+ *NLA_NESTED `nested_policy' to a nested policy to validate, must
+ * also set `len' to the max attribute number. Use the
+ * provided NLA_POLICY_NESTED() macro.
* Note that nla_parse() will validate, but of course
not
* parse, the nested sub-policies.
- *NLA_NESTED_ARRAY Points to a nested policy to validate, must also set
- * `len' to the max attribute number. The difference to
- * NLA_NESTED is the structure - NLA_NESTED has the
- * nested attributes directly inside, while an array
has
- * the nested attributes at another level down and the
- * attributes directly in the nesting don't matter.
- *All otherUnused - but note that it's a union
- *
- * Meaning of `min' and `max' fields, use via NLA_POLICY_MIN, NLA_POLICY_MAX
- * and NLA_POLICY_RANGE:
+ *NLA_NESTED_ARRAY `nested_policy' points to a nested policy to
validate,
+ * must also set `len' to the max attribute number. Use
+ * the provided NLA_POLICY_NESTED_ARRAY() macro.
+ * The difference to NLA_NESTED is the structure:
+ * NLA_NESTED has the nested attributes directly inside
+ * while an array has the nested attributes at another
+ * level down and the attribute types directly in the
+ * nesting don't matter.
*NLA_U8,
*NLA_U16,
*NLA_U32,
@@ -263,14 +261,16 @@ enum nla_policy_validation {
*NLA_S8,
*NLA_S16,
*NLA_S32,
- *NLA_S64 These are used depending on the validation_type
- * field, if that is min/max/range then the minimum,
- * maximum and both are used (respectively) to check
+ *NLA_S64 The `min' and `max' fields are used depending on the
+ * validation_type field, if that is min/max/range then
+ * the min, max or both are used (respectively) to
check
* the value of the integer attribute.
* Note that in the interest of code simplicity and
* struct size both limits are s16, so you cannot
* enforce a range that doesn't fall within the range
* of s16 - do that as usual in th
[RFC 3/7] netlink: extend policy range validation
From: Johannes Berg
Using a pointer to a struct indicating the min/max values,
extend the ability to do range validation for arbitrary
values. Small values in the s16 range can be kept in the
policy directly.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 45 +
lib/nlattr.c | 112 ++
2 files changed, 136 insertions(+), 21 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 22984933bc36..12de018ec7b8 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -189,11 +189,20 @@ enum {
#define NLA_TYPE_MAX (__NLA_TYPE_MAX - 1)
+struct netlink_range_validation {
+ u64 min, max;
+};
+
+struct netlink_range_validation_signed {
+ s64 min, max;
+};
+
enum nla_policy_validation {
NLA_VALIDATE_NONE,
NLA_VALIDATE_RANGE,
NLA_VALIDATE_MIN,
NLA_VALIDATE_MAX,
+ NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
};
@@ -271,6 +280,22 @@ enum nla_policy_validation {
* of s16 - do that as usual in the code instead.
* Use the NLA_POLICY_MIN(), NLA_POLICY_MAX() and
* NLA_POLICY_RANGE() macros.
+ *NLA_U8,
+ *NLA_U16,
+ *NLA_U32,
+ *NLA_U64 If the validation_type field instead is set to
+ * NLA_VALIDATE_RANGE_PTR, `range' must be a pointer
+ * to a struct netlink_range_validation that indicates
+ * the min/max values.
+ * Use NLA_POLICY_FULL_RANGE().
+ *NLA_S8,
+ *NLA_S16,
+ *NLA_S32,
+ *NLA_S64 If the validation_type field instead is set to
+ * NLA_VALIDATE_RANGE_PTR, `range_signed' must be a
+ * pointer to a struct netlink_range_validation_signed
+ * that indicates the min/max values.
+ * Use NLA_POLICY_FULL_RANGE_SIGNED().
*All otherUnused - but note that it's a union
*
* Meaning of `validate' field, use via NLA_POLICY_VALIDATE_FN:
@@ -299,6 +324,8 @@ struct nla_policy {
const u32 bitfield32_valid;
const char *reject_message;
const struct nla_policy *nested_policy;
+ struct netlink_range_validation *range;
+ struct netlink_range_validation_signed *range_signed;
struct {
s16 min, max;
};
@@ -345,6 +372,12 @@ struct nla_policy {
{ .type = NLA_BITFIELD32, .bitfield32_valid = valid }
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
+#define NLA_ENSURE_UINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
+ tp == NLA_U32 || tp == NLA_U64) + tp)
+#define NLA_ENSURE_SINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_S8 || tp == NLA_S16 || \
+ tp == NLA_S32 || tp == NLA_S64) + tp)
#define NLA_ENSURE_INT_TYPE(tp)\
(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 || \
tp == NLA_S16 || tp == NLA_U16 || \
@@ -363,6 +396,18 @@ struct nla_policy {
.max = _max \
}
+#define NLA_POLICY_FULL_RANGE(tp, _range) {\
+ .type = NLA_ENSURE_UINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_RANGE_PTR, \
+ .range = _range,\
+}
+
+#define NLA_POLICY_FULL_RANGE_SIGNED(tp, _range) { \
+ .type = NLA_ENSURE_SINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_RANGE_PTR, \
+ .range = _range,\
+}
+
#define NLA_POLICY_MIN(tp, _min) { \
.type = NLA_ENSURE_INT_TYPE(tp),\
.validation_type = NLA_VALIDATE_MIN,\
diff --git a/lib/nlattr.c b/lib/nlattr.c
index c97e5a505c9f..b11079a9c97e 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,17 +96,33 @@ static int nla_validate_array(const struct nlattr *head,
int len, int maxtype,
return 0;
}
-static int nla_validate_int_range(const struct nla_policy *pt,
- const struct nlattr *nla,
- struct netlink_ext_ack *extack)
+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 ||
-
[RFC 6/7] netlink: factor out policy range helpers
From: Johannes Berg
Add helpers to get the policy's signed/unsigned range
validation data.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 5 +++
lib/nlattr.c | 95 +--
2 files changed, 79 insertions(+), 21 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 8104d935a827..b131dd77390d 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1879,4 +1879,9 @@ static inline bool nla_is_last(const struct nlattr *nla,
int rem)
return nla->nla_len == rem;
}
+void nla_get_range_unsigned(const struct nla_policy *pt,
+ struct netlink_range_validation *range);
+void nla_get_range_signed(const struct nla_policy *pt,
+ struct netlink_range_validation_signed *range);
+
#endif
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 5fe1e7baf371..baf27844ecc8 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,25 +96,39 @@ static int nla_validate_array(const struct nlattr *head,
int len, int maxtype,
return 0;
}
-static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
- const struct nlattr *nla,
- struct netlink_ext_ack *extack)
+void nla_get_range_unsigned(const struct nla_policy *pt,
+ struct netlink_range_validation *range)
{
- struct netlink_range_validation _range = {
- .min = 0,
- .max = U64_MAX,
- }, *range = &_range;
- u64 value;
-
WARN_ON_ONCE(pt->min < 0 || pt->max < 0);
+ range->min = 0;
+
+ switch (pt->type) {
+ case NLA_U8:
+ range->max = U8_MAX;
+ break;
+ case NLA_U16:
+ range->max = U16_MAX;
+ break;
+ case NLA_U32:
+ range->max = U32_MAX;
+ break;
+ case NLA_U64:
+ case NLA_MSECS:
+ range->max = U64_MAX;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
+
switch (pt->validation_type) {
case NLA_VALIDATE_RANGE:
range->min = pt->min;
range->max = pt->max;
break;
case NLA_VALIDATE_RANGE_PTR:
- range = pt->range;
+ *range = *pt->range;
break;
case NLA_VALIDATE_MIN:
range->min = pt->min;
@@ -122,7 +136,17 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
case NLA_VALIDATE_MAX:
range->max = pt->max;
break;
+ default:
+ break;
}
+}
+
+static int nla_validate_int_range_unsigned(const struct nla_policy *pt,
+ const struct nlattr *nla,
+ struct netlink_ext_ack *extack)
+{
+ struct netlink_range_validation range;
+ u64 value;
switch (pt->type) {
case NLA_U8:
@@ -142,7 +166,9 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
return -EINVAL;
}
- if (value < range->min || value > range->max) {
+ nla_get_range_unsigned(pt, &range);
+
+ if (value < range.min || value > range.max) {
NL_SET_ERR_MSG_ATTR(extack, nla,
"integer out of range");
return -ERANGE;
@@ -151,15 +177,30 @@ static int nla_validate_int_range_unsigned(const struct
nla_policy *pt,
return 0;
}
-static int nla_validate_int_range_signed(const struct nla_policy *pt,
-const struct nlattr *nla,
-struct netlink_ext_ack *extack)
+void nla_get_range_signed(const struct nla_policy *pt,
+ struct netlink_range_validation_signed *range)
{
- struct netlink_range_validation_signed _range = {
- .min = S64_MIN,
- .max = S64_MAX,
- }, *range = &_range;
- s64 value;
+ switch (pt->type) {
+ case NLA_S8:
+ range->min = S8_MIN;
+ range->max = S8_MAX;
+ break;
+ case NLA_S16:
+ range->min = S16_MIN;
+ range->max = S16_MAX;
+ break;
+ case NLA_S32:
+ range->min = S32_MIN;
+ range->max = S32_MAX;
+ break;
+ case NLA_S64:
+ range->min = S64_MIN;
+ range->max = S64_MAX;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
switch (pt->validation_type) {
case NLA_VALIDATE_RANGE:
@@ -167,7 +208,7 @@
[RFC 5/7] netlink: remove NLA_EXACT_LEN_WARN
From: Johannes Berg
Use a validation type instead, so we can later expose
the NLA_* values to userspace for policy descriptions.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 15 ---
lib/nlattr.c | 16 ++--
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 6c79a1d0e30f..8104d935a827 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -182,7 +182,6 @@ enum {
NLA_BITFIELD32,
NLA_REJECT,
NLA_EXACT_LEN,
- NLA_EXACT_LEN_WARN,
NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -204,6 +203,7 @@ enum nla_policy_validation {
NLA_VALIDATE_MAX,
NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
+ NLA_VALIDATE_WARN_TOO_LONG,
};
/**
@@ -237,10 +237,10 @@ enum nla_policy_validation {
* just like "All other"
*NLA_BITFIELD32 Unused
*NLA_REJECT Unused
- *NLA_EXACT_LENAttribute must have exactly this length, otherwise
- * it is rejected.
- *NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
- * is logged if it is longer, shorter is rejected.
+ *NLA_EXACT_LENAttribute should have exactly this length, otherwise
+ * it is rejected or warned about, the latter happening
+ * if and only if the `validation_type' is set to
+ * NLA_VALIDATE_WARN_TOO_LONG.
*NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
@@ -353,8 +353,9 @@ struct nla_policy {
};
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
-#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
- .len = _len }
+#define NLA_POLICY_EXACT_LEN_WARN(_len) \
+ { .type = NLA_EXACT_LEN, .len = _len, \
+ .validation_type = NLA_VALIDATE_WARN_TOO_LONG, }
#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 2e6e6e29efda..5fe1e7baf371 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -245,7 +245,9 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
BUG_ON(pt->type > NLA_TYPE_MAX);
if ((nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) ||
- (pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) {
+ (pt->type == NLA_EXACT_LEN &&
+pt->validation_type == NLA_VALIDATE_WARN_TOO_LONG &&
+attrlen != pt->len)) {
pr_warn_ratelimited("netlink: '%s': attribute type %d has an
invalid length.\n",
current->comm, type);
if (validate & NL_VALIDATE_STRICT_ATTRS) {
@@ -256,11 +258,6 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
switch (pt->type) {
- case NLA_EXACT_LEN:
- if (attrlen != pt->len)
- goto out_err;
- break;
-
case NLA_REJECT:
if (extack && pt->reject_message) {
NL_SET_BAD_ATTR(extack, nla);
@@ -373,6 +370,13 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
goto out_err;
break;
+ case NLA_EXACT_LEN:
+ if (pt->validation_type != NLA_VALIDATE_WARN_TOO_LONG) {
+ if (attrlen != pt->len)
+ goto out_err;
+ break;
+ }
+ /* fall through */
default:
if (pt->len)
minlen = pt->len;
--
2.17.2
[RFC 1/7] netlink: expose policy to userspace
Now that it's working, here's the first draft of the code to expose the netlink policy to userspace. One thing I didn't think I would do but did anyway now is to map the NLA_* type to a new attribute, and translate it for userspace. This may not be the most convenient, but I think we would prefer to have more flexibility with the NLA_* types in the future. For example, there's no real reason to have NLA_MSECS vs. NLA_U64, and we may want to remove one. Similarly, we have a lot of types for binary: * NLA_UNSPEC- min length * NLA_BINARY- max length * NLA_MIN_LEN - min length * NLA_EXACT_LEN - min & max length are equal And obviously we may want to expand that in the future to have *both* max and min length (we could easily do it using the range we have now already, in fact.) As we can expose both min and max length to userspace in optional attributes, these can just be the same NL_ATTR_TYPE_BINARY. I have a very hacky (and full of warnings) change to iproute2, I've put it here but don't look closely: https://p.sipsolutions.net/4c674acaf8d6ca71.txt It will print out things like (for nl80211): ID: 0x18 policy[0]:attr[4]: type=NUL_STRING max len:15 ID: 0x18 policy[0]:attr[5]: type=U32 range:[0,12] ID: 0x18 policy[0]:attr[15]: type=BINARY max len:2304 ID: 0x18 policy[0]:attr[16]: type=U16 range:[1,2007] ID: 0x18 policy[0]:attr[273]: type=NESTED policy:2 maxattr:5 ID: 0x18 policy[2]:attr[5]: type=NESTED_ARRAY policy:3 maxattr:4 ID: 0x18 policy[3]:attr[1]: type=BINARY min len:6 max len:6 ID: 0x18 policy[3]:attr[2]: type=NESTED ID: 0x18 policy[3]:attr[3]: type=NESTED policy:4 maxattr:2 [...] I've omitted lots of lines, I get close to 200 entries for the current nl80211 policy. As far as mechanics go, this is based on my previous patchset to allow making validation strict. In principle, it's orthogonal, but I suspect it would have some conflicts to apply. The combined code is also available in mac80211-next (kernel.org) in the `netlink-policy-export' branch. johannes
[RFC 1/7] nl80211: fix NL80211_ATTR_FTM_RESPONDER policy
From: Johannes Berg
The nested policy here should be established using the
NLA_POLICY_NESTED() macro so the length is properly
filled in.
Fixes: 81e54d08d9d8 ("cfg80211: support FTM responder configuration/statistics")
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 28249317aa98..e5c0e693a1b2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -543,10 +543,8 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] =
{
[NL80211_ATTR_HE_CAPABILITY] = { .type = NLA_BINARY,
.len = NL80211_HE_MAX_CAPABILITY_LEN },
- [NL80211_ATTR_FTM_RESPONDER] = {
- .type = NLA_NESTED,
- .validation_data = nl80211_ftm_responder_policy,
- },
+ [NL80211_ATTR_FTM_RESPONDER] =
+ NLA_POLICY_NESTED(nl80211_ftm_responder_policy),
[NL80211_ATTR_TIMEOUT] = NLA_POLICY_MIN(NLA_U32, 1),
[NL80211_ATTR_PEER_MEASUREMENTS] =
NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
--
2.17.2
Re: cellular modem driver APIs
On Thu, 2019-04-04 at 22:38 +0200, Johannes Berg wrote: > > The bridge mode is used only for testing by sending frames > > without de-muxing to some other driver such as a USB netdev so packets > > can be parsed on a tethered PC. > > Yeah, I get it, it's just done in a strange way. You'd think adding a > tcpdump or some small application that just resends the packets directly > received from the underlying "real_dev" using a ptype_all socket would > be sufficient? Though perhaps not quite the same performance, but then > you could easily not use an application but a dev_add_pack() thing? Or > probably even tc's mirred? And to extend that thought, tc's ife action would let you encapsulate the things you have in ethernet headers... I think. johannes
Re: cellular modem driver APIs
Hi,
> The normal mode of operation of rmnet is using the rmnet netdevices
> in an embedded device.
Sure. Can you say what driver this would typically live on top of? I'm
actually a bit surprised to find out this isn't really a driver :-)
In my view right now, I'd recommend splitting rmnet into two pieces:
1) The netlink API, be it called "rmnet" or something else, probably
I'd call it something else like "wwan-mux" or so and leave "rmnet"
as an alias.
This part is certainly going to be generic. I'm not sure where
exactly to draw the line here, but I'd rather start out drawing it
more over to the API side (i.e. keep it just the API) instead of
including some of the RX handlers etc.
2) The actual layering protocol implementation, with things like
rmnet_map_ingress_handler() and rmnet_map_add_map_header(),
basically all the stuff handling MAP headers.
This is clearly device specific, but I wonder what device.
> The bridge mode is used only for testing by sending frames
> without de-muxing to some other driver such as a USB netdev so packets
> can be parsed on a tethered PC.
Yeah, I get it, it's just done in a strange way. You'd think adding a
tcpdump or some small application that just resends the packets directly
received from the underlying "real_dev" using a ptype_all socket would
be sufficient? Though perhaps not quite the same performance, but then
you could easily not use an application but a dev_add_pack() thing? Or
probably even tc's mirred?
> I was planning to refactor qmi_wwan to reuse rmnet as much as possible.
> Unfortunately, I wasn't able to get qmi_wwan / modem manager
> configured and never got to this.
OK.
> Having a single channel is not common
Depends what kind of system you're looking at, but I hear you :)
> so rmnet doesn't support a default
> channel mode. Most of the uses cases we have require multiple PDNs so
> this translates to multiple rmnet devices.
Right, sure. I just wonder then what happens with the underlying netdev.
Is that just dead?
In fact, is the underlying netdev even usable without having rmnet on
top? If not, why is it even a netdev?
Like I said, I'm thinking that the whole wiphy/vif abstraction makes a
bit more sense here too, like in wifi, although it requires all new
userspace or at least not using IFLA_LINK but something else ...
Then again, I suppose we could have an IFLA_WWAN_DEVICE and have that be
something else, just like I suppose we could create a wifi virtual
device by having IFLA_WIPHY and using rtnl_link_ops, just didn't do it.
Would or wouldn't that make more sense?
We can still create a default channel netdev on top ...
My gut feeling is that it would in fact make more sense.
So, straw man proposal:
* create net/wwan/ with some API like
- register_wwan_device()/unregister_wwan_device()
- struct wwan_device_ops {
int (*wdo_add_channel)(struct wwan_device *dev,
struct wwan_channel_cfg *cfg);
int (*wdo_rm_channel)(...);
/* ... */
}
* implement there a rtnl_link_ops that can create new links but needs
the wwan device (IFLA_WWAN) instead of underlying netdev (IFLA_LINK)
* strip the rtnl_link_ops from rmnet and use that infra instead
Now, OTOH, this loses a bunch of benefits. We may want to be able to use
ethtool to flash a modem, start tcpdump on the underlying netdev
directly to see everything, etc.?
So the alternative would be to still have the underlying wwan device
represent itself as a netdev. But is that the right thing to do when
that never really transports data frames in the real use cases?
For wifi we actually had this ('master' netdev), but we decided to get
rid of it, the abstraction just didn't make sense, and the frames moving
through those queues etc. also didn't really make sense. But the
downside is building more APIs here, and for some parts also custom
userspace.
Then again, we could also have a 'monitor wwan' device type, right? You
say you don't add a specific channel, but a sniffer, and then you can
use tcpdump etc. on the sniffer netdev. Same in wireless, really.
So anyway. Can you say what rmnet is used on top of, and what use the
underlying netdev is when you say it's not really of much use since you
need multiple channels?
johannes
Re: cellular modem driver APIs
Hi, > Thanks a lot for doing this! Being responsible for most of the issues > you point out, I can only say that you have my full support if you want > to change any of it. :-) > My pathetic excuses below are just meant to clarify why things are the > way they are. They are not a defense for status quo ;-) Thanks! > > Here's the current things we seem to be doing: > > > > (1) Channels are created/encoded as VLANs (cdc_mbim) > > > > This is ... strange at best, it requires creating fake ethernet > > headers on the frames, just to be able to have a VLAN tag. If you > > could rely on VLAN acceleration it wouldn't be _so_ bad, but of > > course you can't, so you have to detect an in-band VLAN tag and > > decode/remove it, before taking the VLAN ID into the virtual > > channel number. > > No, the driver sets NETIF_F_HW_VLAN_CTAG_TX. There is no in-band VLAN > tag for any normal use. The tag is taken directly from skb metadata and > mapped to the appropriate MBIM session ID. Right, I saw this. > But this failed when cooking raw frames with an in-line tag using packet > sockets, so I added a fallback to in-line tags for that use case. But this still means that the fallback for in-line has to be supported, so you can't really fully rely on VLAN acceleration. Maybe my wording here was incomplete, but I was aware of this. Nevertheless, it means to replicate this in another driver you don't just need the VLAN acceleration handling, but also the fallback, so it's a bunch of extra code. > > Creating channels is hooked on VLAN operations, which is about the > > only thing that makes sense here? > > Well, that was why I did this, to avoid requiring som new set of > userspace tools to manage these links. I looked for some existing tools > for adding virtual netdevs, and I thought I could make VLANs fit the > scheme. Right. > In hindsight, I should have created a new netlink based API for cellular > modem virtual links instead. But I don't think it ever struck me as a > choice I had at the time. I just wasn't experienced enough to realize > how the Linux kernel APIs are developed ;-) :-) And likely really it wasn't all as fleshed out as today with the plethora of virtual links supported. This seems fairly old. > > > (2) Channels are created using sysfs (qmi_wwan) > > > > This feels almost worse - channels are created using sysfs and > > just *bam* new netdev shows up, no networking APIs are used to > > create them at all, and I suppose you can't even query the channel > > ID for each netdev if you rename them or so. Actually, maybe you > > can in sysfs, not sure I understand the code fully. > > This time I was, and I tried to learn from the MBIM mistake. So I asked > the users (ModemManager developers++), proposing a netlink API as a > possible solution: > > https://lists.freedesktop.org/archives/libqmi-devel/2017-January/001900.html > > The options I presented were those I saw at the time: VLANs like > cdc_mbim, a new netlink API, or sysfs. There wasn't much feedback, but > sysfs "won". So this was a decision made by the users of the API, FWIW. Fair point. Dan pointed out that no (default) userspace actually exists to do this though, and users kinda of have to do it manually - he says modem manager and libmbim all just use the default channel today. So not sure they really went on to become users of this ;-) > > (3) Channels are created using a new link type (rmnet) > > > > To me this sort of feels the most natural, but this particular > > implementation has at least two issues: > > > > (a) The implementation is basically driver-specific now, the link > > type is called 'rmnet' etc. > > (b) The bridge enslave thing there is awful. > > > This driver showed up right after the sysfs based implementation in > qmi_wwan. Too bad we didn't know about this work then. I don't think > anyone would have been interested in the qmi_wwan sysfs thing if we had > known about the plans for this driver. But what's done is done. Sure. > > It seems to me that there really is space here for some common > > framework, probably modelled on rmnet - that seems the most reasonable > > approach of all three. > > > > The only question I have there is whether the 'netdev model' they all > > have actually makes sense. What I mean by that is that they all assume > > they have a default channel (using untagged frames, initial netdev, > > initial netdev respectively for (1) - (3)). > > Good question. I guess the main argument for the 'netdev model' is that > it makes the device directly usable with no extra setup or tools. Most > users won't ever want or need more than one channel anyway. They use > the modem for a single IP session. You can do that with both models, really. I mean, with wifi we just create a single virtual interface by default and you can then go and use it. But you can a
cellular modem driver APIs
Hi all, I've been looking at modem drivers, to see what the APIs are to interact with them, and while I originally thought I had the story sorted out ... not at all. Here's the current things we seem to be doing: (1) Channels are created/encoded as VLANs (cdc_mbim) This is ... strange at best, it requires creating fake ethernet headers on the frames, just to be able to have a VLAN tag. If you could rely on VLAN acceleration it wouldn't be _so_ bad, but of course you can't, so you have to detect an in-band VLAN tag and decode/remove it, before taking the VLAN ID into the virtual channel number. Creating channels is hooked on VLAN operations, which is about the only thing that makes sense here? (2) Channels are created using sysfs (qmi_wwan) This feels almost worse - channels are created using sysfs and just *bam* new netdev shows up, no networking APIs are used to create them at all, and I suppose you can't even query the channel ID for each netdev if you rename them or so. Actually, maybe you can in sysfs, not sure I understand the code fully. (3) Channels are created using a new link type (rmnet) To me this sort of feels the most natural, but this particular implementation has at least two issues: (a) The implementation is basically driver-specific now, the link type is called 'rmnet' etc. (b) The bridge enslave thing there is awful. It seems to me that there really is space here for some common framework, probably modelled on rmnet - that seems the most reasonable approach of all three. The only question I have there is whether the 'netdev model' they all have actually makes sense. What I mean by that is that they all assume they have a default channel (using untagged frames, initial netdev, initial netdev respectively for (1) - (3)). In 802.11, we don't have such a default channel - you can add/remove virtual netdevs on the fly. But if you want to do that, then you can't use IFLA_LINK and the normal link type, which means custom netlink and custom userspace etc. which, while we do it in wifi, is bothersome. Here I guess the question would be whether it makes sense to even remove the default channel, or retag it, or something like that. If no, then to me it all makes sense to just model rmnet. And even if it *is* something that could theoretically be done, it seems well possible to me that the benefits (using rtnl_link_register() etc.) outweigh the deficits of the approach. I'm tempted to take a stab at breaking out rmnet_link_ops from the rmnet driver, somehow giving it an alias of 'wwan-channel' or something like that, and putting it into some sort of small infrastructure. Anyone else have any thoughts? Thanks, johannes
[RFC v3 3/6] netlink: re-add parse/validate functions in strict mode
From: Johannes Berg
This re-adds the parse and validate functions like nla_parse()
that are now actually strict after the previous rename and were
just split out to make sure everything is converted (and if not
compilation of the previous patch would fail.)
Signed-off-by: Johannes Berg
---
include/net/genetlink.h | 19
include/net/netlink.h | 65 +
2 files changed, 84 insertions(+)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 897cdba13569..68de579cfe5e 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -183,6 +183,25 @@ static inline int genlmsg_parse_deprecated(const struct
nlmsghdr *nlh,
policy, NL_VALIDATE_LIBERAL, extack);
}
+/**
+ * genlmsg_parse - parse attributes of a genetlink message
+ * @nlh: netlink message header
+ * @family: genetlink message family
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ */
+static inline int genlmsg_parse(const struct nlmsghdr *nlh,
+ const struct genl_family *family,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nlmsg_parse(nlh, family->hdrsize + GENL_HDRLEN, tb, maxtype,
+policy, NL_VALIDATE_STRICT, extack);
+}
+
/**
* genl_dump_check_consistent - check if sequence is consistent and advertise
if not
* @cb: netlink callback structure that stores the sequence number
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 926fafb5d501..a1db8dcb4f73 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -542,6 +542,31 @@ nlmsg_next(const struct nlmsghdr *nlh, int *remaining)
return (struct nlmsghdr *) ((unsigned char *) nlh + totlen);
}
+/**
+ * nla_parse - Parse a stream of attributes into a tb buffer
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @policy: validation policy
+ * @extack: extended ACK pointer
+ *
+ * Parses a stream of attributes and stores a pointer to each attribute in
+ * the tb array accessible via the attribute type. Attributes with a type
+ * exceeding maxtype will be rejected, policy must be specified, attributes
+ * will be validated in the strictest way possible.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+static inline int nla_parse(struct nlattr **tb, int maxtype,
+ const struct nlattr *head, int len,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, head, len, policy,
+ NL_VALIDATE_STRICT, extack);
+}
+
/**
* nla_parse_deprecated - Parse a stream of attributes into a tb buffer
* @tb: destination array with maxtype+1 elements
@@ -595,6 +620,27 @@ static inline int __nlmsg_parse(const struct nlmsghdr
*nlh, int hdrlen,
extack);
}
+/**
+ * nlmsg_parse - parse attributes of a netlink message
+ * @nlh: netlink message header
+ * @hdrlen: length of family specific header
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @validate: validation strictness
+ * @extack: extended ACK report struct
+ *
+ * See nla_parse()
+ */
+static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, nlmsg_attrdata(nlh, hdrlen),
+ nlmsg_attrlen(nlh, hdrlen), policy,
+ NL_VALIDATE_STRICT, extack);
+}
+
/**
* nlmsg_parse_deprecated - parse attributes of a netlink message
* @nlh: netlink message header
@@ -984,6 +1030,25 @@ nla_find_nested(const struct nlattr *nla, int attrtype)
return nla_find(nla_data(nla), nla_len(nla), attrtype);
}
+/**
+ * nla_parse_nested - parse nested attributes
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @nla: attribute containing the nested attributes
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ *
+ * See nla_parse()
+ */
+static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
+ const struct nlattr *nla,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, nla_d
[RFC v3 4/6] netlink: add strict parsing for future attributes
From: Johannes Berg
Unfortunately, we cannot add strict parsing for all attributes, as
that would break existing userspace. We currently warn about it, but
that's about all we can do.
For new attributes, however, the story is better: nobody is using
them, so we can reject bad sizes.
Also, for new attributes, we need not accept them when the policy
doesn't declare their usage.
David Ahern and I went back and forth on how to best encode this, and
the best way we found was to have a "boundary type", from which point
on new attributes have all possible validation applied, and NLA_UNSPEC
is rejected.
As we didn't want to add another argument to all functions that get a
netlink policy, the workaround is to encode that boundary in the first
entry of the policy array (which is for type 0 and thus probably not
really valid anyway). I put it into the validation union for the rare
possibility that somebody is actually using attribute 0, which would
continue to work fine unless they tried to use the extended validation,
which isn't likely. We also didn't find any in-tree users with type 0.
The reason for setting the "start strict here" attribute is that we
never really need to start strict from 0, which is invalid anyway (or
in legacy families where that isn't true, it cannot be set to strict),
so we can thus reserve the value 0 for "don't do this check" and don't
have to add the tag to all policies right now.
Thus, policies can now opt in to this validation, which we should do
for all existing policies, at least when adding new attributes.
Note that entirely *new* policies won't need to set it, as the use
of that should be using nla_parse()/nlmsg_parse() etc. which anyway
do fully strict validation now, regardless of this.
So in effect, this patch only covers the "existing command with new
attribute" case.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 18 ++
lib/nlattr.c | 4
2 files changed, 22 insertions(+)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index a1db8dcb4f73..91491608d869 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -299,6 +299,24 @@ struct nla_policy {
};
int (*validate)(const struct nlattr *attr,
struct netlink_ext_ack *extack);
+ /* This entry is special, and used for the attribute at index 0
+* only, and specifies special data about the policy, namely it
+* specifies the "boundary type" where strict length validation
+* starts for any attribute types >= this value, also, strict
+* nesting validation starts here.
+*
+* Additionally, it means that NLA_UNSPEC is actually NLA_REJECT
+* for any types >= this, so need to use NLA_MIN_LEN to get the
+* previous pure { .len = xyz } behaviour. The advantage of this
+* is that types not specified in the policy will be rejected.
+*
+* For completely new families it should be set to 1 so that the
+* validation is enforced for all attributes. For existing ones
+* it should be set at least when new attributes are added to
+* the enum used by the policy, and be set to the new value that
+* was added to enforce strict validation from thereon.
+*/
+ u16 strict_start_type;
};
};
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 95bf2fc82c81..46110c314438 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -158,10 +158,14 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
const struct nla_policy *policy, unsigned int validate,
struct netlink_ext_ack *extack)
{
+ u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
int err = -ERANGE;
+ if (strict_start_type && type >= strict_start_type)
+ validate |= NL_VALIDATE_STRICT;
+
if (type <= 0 || type > maxtype)
return 0;
--
2.17.2
[RFC v3 5/6] genetlink: optionally validate strictly/dumps
From: Johannes Berg
Add options to strictly validate messages and dump messages,
sometimes perhaps validating dump messages non-strictly may
be required, so add an option for that as well.
Since none of this can really be applied to existing commands,
set the options everwhere using the following spatch:
@@
identifier ops;
expression X;
@@
struct genl_ops ops[] = {
...,
{
.cmd = X,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
...
},
...
};
For new commands one should just not copy the .validate 'opt-out'
flags and thus get strict validation.
Signed-off-by: Johannes Berg
---
drivers/block/nbd.c | 4 +
drivers/net/gtp.c| 3 +
drivers/net/ieee802154/mac802154_hwsim.c | 6 ++
drivers/net/macsec.c | 10 +++
drivers/net/team/team.c | 4 +
drivers/net/wireless/mac80211_hwsim.c| 6 ++
drivers/target/target_core_user.c| 4 +
fs/dlm/netlink.c | 1 +
include/net/genetlink.h | 12 +++
kernel/taskstats.c | 2 +
net/batman-adv/netlink.c | 18
net/core/devlink.c | 38 +
net/core/drop_monitor.c | 3 +
net/hsr/hsr_netlink.c| 2 +
net/ieee802154/nl802154.c| 29 +++
net/ipv4/fou.c | 3 +
net/ipv4/tcp_metrics.c | 2 +
net/ipv6/ila/ila_main.c | 4 +
net/ipv6/seg6.c | 4 +
net/l2tp/l2tp_netlink.c | 9 ++
net/ncsi/ncsi-netlink.c | 6 ++
net/netfilter/ipvs/ip_vs_ctl.c | 16
net/netlabel/netlabel_calipso.c | 4 +
net/netlabel/netlabel_cipso_v4.c | 4 +
net/netlabel/netlabel_mgmt.c | 8 ++
net/netlabel/netlabel_unlabeled.c| 8 ++
net/netlink/genetlink.c | 46 +++---
net/nfc/netlink.c| 19 +
net/openvswitch/conntrack.c | 3 +
net/openvswitch/datapath.c | 13 +++
net/openvswitch/meter.c | 4 +
net/psample/psample.c| 1 +
net/smc/smc_pnet.c | 4 +
net/tipc/netlink.c | 21 +
net/tipc/netlink_compat.c| 1 +
net/wimax/stack.c| 4 +
net/wireless/nl80211.c | 104 +++
37 files changed, 420 insertions(+), 10 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 91cff60f2839..569c92520150 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2003,18 +2003,22 @@ static int nbd_genl_reconfigure(struct sk_buff *skb,
struct genl_info *info)
static const struct genl_ops nbd_connect_genl_ops[] = {
{
.cmd= NBD_CMD_CONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_connect,
},
{
.cmd= NBD_CMD_DISCONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_disconnect,
},
{
.cmd= NBD_CMD_RECONFIGURE,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_reconfigure,
},
{
.cmd= NBD_CMD_STATUS,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_status,
},
};
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index c06e31747288..eaf4311b4004 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1270,16 +1270,19 @@ static const struct nla_policy gtp_genl_policy[GTPA_MAX
+ 1] = {
static const struct genl_ops gtp_genl_ops[] = {
{
.cmd = GTP_CMD_NEWPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_new_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_DELPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_del_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_GETPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_get_pdp,
.dumpit = gtp_genl_dump_pdp,
.flags = GENL_ADMIN_PERM,
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c
b/drivers/net/ieee802154/mac802154_hwsim.c
index bb781820b879..e3963fc50fcc 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -592,31 +592,37 @@ st
[RFC v3 6/6] nl80211: tag policies with strict_start_type
From: Johannes Berg
Tag all the nl80211 policies with strict_start_type so that
strict validation is done for all types that we have a policy
for.
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d8eaa161c308..5bad1d19c905 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -221,6 +221,7 @@ static int validate_ie_attr(const struct nlattr *attr,
/* policy for the attributes */
static const struct nla_policy
nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_FTM_RESP_ATTR_CIVICLOC + 1 },
[NL80211_FTM_RESP_ATTR_ENABLED] = { .type = NLA_FLAG, },
[NL80211_FTM_RESP_ATTR_LCI] = { .type = NLA_BINARY,
.len = U8_MAX },
@@ -230,6 +231,10 @@ nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX +
1] = {
static const struct nla_policy
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
+ [0] = {
+ .strict_start_type =
+ NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC + 1
+ },
[NL80211_PMSR_FTM_REQ_ATTR_ASAP] = { .type = NLA_FLAG },
[NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE] = { .type = NLA_U32 },
[NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP] =
@@ -246,12 +251,14 @@
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
static const struct nla_policy
nl80211_pmsr_req_data_policy[NL80211_PMSR_TYPE_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_TYPE_FTM + 1 },
[NL80211_PMSR_TYPE_FTM] =
NLA_POLICY_NESTED(nl80211_pmsr_ftm_req_attr_policy),
};
static const struct nla_policy
nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_REQ_ATTR_GET_AP_TSF + 1 },
[NL80211_PMSR_REQ_ATTR_DATA] =
NLA_POLICY_NESTED(nl80211_pmsr_req_data_policy),
[NL80211_PMSR_REQ_ATTR_GET_AP_TSF] = { .type = NLA_FLAG },
@@ -259,6 +266,7 @@ nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1]
= {
static const struct nla_policy
nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_PEER_ATTR_RESP + 1 },
[NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR,
/*
* we could specify this again to be the top-level policy,
@@ -272,6 +280,7 @@ nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX +
1] = {
static const struct nla_policy
nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
+ [0] { .strict_start_type = NL80211_PMSR_ATTR_PEERS + 1 },
[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
@@ -281,6 +290,7 @@ nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
+ [0] = { .strict_start_type = NL80211_ATTR_AIRTIME_WEIGHT + 1 },
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
[NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
.len = 20-1 },
@@ -545,6 +555,7 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
/* policy for the key attributes */
static const struct nla_policy nl80211_key_policy[NL80211_KEY_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_KEY_DEFAULT_TYPES + 1 },
[NL80211_KEY_DATA] = { .type = NLA_BINARY, .len = WLAN_MAX_KEY_LEN },
[NL80211_KEY_IDX] = { .type = NLA_U8 },
[NL80211_KEY_CIPHER] = { .type = NLA_U32 },
@@ -558,6 +569,7 @@ static const struct nla_policy
nl80211_key_policy[NL80211_KEY_MAX + 1] = {
/* policy for the key default flags */
static const struct nla_policy
nl80211_key_default_policy[NUM_NL80211_KEY_DEFAULT_TYPES] = {
+ [0] = { .strict_start_type = NL80211_KEY_DEFAULT_TYPE_MULTICAST + 1 },
[NL80211_KEY_DEFAULT_TYPE_UNICAST] = { .type = NLA_FLAG },
[NL80211_KEY_DEFAULT_TYPE_MULTICAST] = { .type = NLA_FLAG },
};
@@ -566,6 +578,7 @@ nl80211_key_default_policy[NUM_NL80211_KEY_DEFAULT_TYPES] =
{
/* policy for WoWLAN attributes */
static const struct nla_policy
nl80211_wowlan_policy[NUM_NL80211_WOWLAN_TRIG] = {
+ [0] = { .strict_start_type = NL80211_WOWLAN_TRIG_NET_DETECT + 1 },
[NL80211_WOWLAN_TRIG_ANY] = { .type = NLA_FLAG },
[NL80211_WOWLAN_TRIG_DISCONNECT] = { .type = NLA_FLAG },
[NL80211_WOWLAN_TRIG_MAGIC_PKT] = { .type = NLA_FLAG },
@@ -580,6 +593,7 @@ nl80211_wowlan_policy[NUM_NL80211_WOWLAN_TRIG] = {
static const struct nla_policy
nl80211_wowlan_tcp_policy[NUM_NL80211_WOWLAN_TCP] = {
+ [0] = { .strict_start_type = NL80211_WOWLAN_TCP_WAKE_MASK + 1 },
[NL80211_WOWLAN_TCP_SRC_IPV4] = { .type = NLA_U32
[RFC v3 1/6] netlink: add NLA_MIN_LEN
From: Johannes Berg
Rather than using NLA_UNSPEC for this type of thing, use NLA_MIN_LEN
so we can make NLA_UNSPEC be NLA_REJECT under certain conditions for
future attributes.
While at it, also use NLA_EXACT_LEN for the struct example.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +-
lib/nlattr.c | 9 -
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 23f27b0b3cef..06f8605b740c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -183,6 +183,7 @@ enum {
NLA_REJECT,
NLA_EXACT_LEN,
NLA_EXACT_LEN_WARN,
+ NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -212,6 +213,7 @@ enum nla_policy_validation {
*NLA_NUL_STRING Maximum length of string (excluding NUL)
*NLA_FLAG Unused
*NLA_BINARY Maximum length of attribute payload
+ *NLA_MIN_LEN Minimum length of attribute payload
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
@@ -230,6 +232,7 @@ enum nla_policy_validation {
* it is rejected.
*NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
* is logged if it is longer, shorter is rejected.
+ *NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
* Meaning of `validation_data' field:
@@ -281,7 +284,7 @@ enum nla_policy_validation {
* static const struct nla_policy my_policy[ATTR_MAX+1] = {
* [ATTR_FOO] = { .type = NLA_U16 },
* [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
- * [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * [ATTR_BAZ] = { .type = NLA_EXACT_LEN, .len = sizeof(struct mystruct) },
* [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data = &myvalidflags
},
* };
*/
@@ -302,6 +305,7 @@ struct nla_policy {
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
.len = _len }
+#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index d26de6156b97..465c9e8ef8a5 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -278,10 +278,17 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
}
break;
+
+ case NLA_UNSPEC:
+ case NLA_MIN_LEN:
+ if (attrlen < pt->len)
+ goto out_err;
+ break;
+
default:
if (pt->len)
minlen = pt->len;
- else if (pt->type != NLA_UNSPEC)
+ else
minlen = nla_attr_minlen[pt->type];
if (attrlen < minlen)
--
2.17.2
[RFC v3 0/6] netlink strict validation
Here's yet another respin. The big change since v2 is replacing the 2nd patch by one that actually makes each validation bit configurable. The old "strict" mode ends up setting 2 of the 5 bits, the new default (and very strict) mode ends up with all 5 bits, of course. This was inspired by a side discussion with Pablo, who still thinks that accepting unknown attributes is a good thing. I obviously disagree, as such attributes might modify behaviour and thus the user intent is no longer guaranteed when ignored, but there are some rare cases where it may be acceptable (such as a power-saving filter), though even in those it's probably better to let the user know. Anyway, inspired by that discussion I split out the NL_VALIDATE_UNSPEC as that might actually be useful for even *old* code, seeing how you can fill in all the policies, using NLA_MIN_LEN instead of NLA_UNSPEC for all existing attributes, and then new attributes are forced to have a policy. This is really the only change, but in order to do it users still need to use the __nlmsg_parse(), __nla_parse() (and other double-underscore) versions of the functions. If Pablo (and Jamal to some extent, I think) end up convincing us that the liberal validation is actually desired (sometimes), it may make some sense to rename the *_deprecated() functions to something less shouting, perhaps use *_old(). I just think that the default nla_parse() should be defaulting to the stricter behaviour, and most people agree that the default behaviour is desirable. FWIW, Pablo also more or less agrees, but would like to have query capabilities in the kernel first, so his userspace can figure out what would be accepted w/o going to a "try this, remove if fails" dance. I'll take a look at his old "netlink bus descriptions" patch to see if that matches what I want to do wrt. this and see how we can merge the two approaches. johannes
[RFC v2 3/6] netlink: re-add parse/validate functions in strict mode
From: Johannes Berg
This re-adds the parse and validate functions like nla_parse()
that are now actually strict after the previous rename and were
just split out to make sure everything is converted (and if not
compilation of the previous patch would fail.)
Signed-off-by: Johannes Berg
---
include/net/genetlink.h | 19
include/net/netlink.h | 65 +
2 files changed, 84 insertions(+)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 897cdba13569..68de579cfe5e 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -183,6 +183,25 @@ static inline int genlmsg_parse_deprecated(const struct
nlmsghdr *nlh,
policy, NL_VALIDATE_LIBERAL, extack);
}
+/**
+ * genlmsg_parse - parse attributes of a genetlink message
+ * @nlh: netlink message header
+ * @family: genetlink message family
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ */
+static inline int genlmsg_parse(const struct nlmsghdr *nlh,
+ const struct genl_family *family,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nlmsg_parse(nlh, family->hdrsize + GENL_HDRLEN, tb, maxtype,
+policy, NL_VALIDATE_STRICT, extack);
+}
+
/**
* genl_dump_check_consistent - check if sequence is consistent and advertise
if not
* @cb: netlink callback structure that stores the sequence number
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 5dc719511eb7..7f368dc13e3d 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -527,6 +527,31 @@ nlmsg_next(const struct nlmsghdr *nlh, int *remaining)
return (struct nlmsghdr *) ((unsigned char *) nlh + totlen);
}
+/**
+ * nla_parse - Parse a stream of attributes into a tb buffer
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @policy: validation policy
+ * @extack: extended ACK pointer
+ *
+ * Parses a stream of attributes and stores a pointer to each attribute in
+ * the tb array accessible via the attribute type. Attributes with a type
+ * exceeding maxtype will be rejected, policy must be specified, attributes
+ * will be validated in the strictest way possible.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+static inline int nla_parse(struct nlattr **tb, int maxtype,
+ const struct nlattr *head, int len,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, head, len, policy,
+ NL_VALIDATE_STRICT, extack);
+}
+
/**
* nla_parse_deprecated - Parse a stream of attributes into a tb buffer
* @tb: destination array with maxtype+1 elements
@@ -580,6 +605,27 @@ static inline int __nlmsg_parse(const struct nlmsghdr
*nlh, int hdrlen,
extack);
}
+/**
+ * nlmsg_parse - parse attributes of a netlink message
+ * @nlh: netlink message header
+ * @hdrlen: length of family specific header
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @validate: validation strictness
+ * @extack: extended ACK report struct
+ *
+ * See nla_parse()
+ */
+static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, nlmsg_attrdata(nlh, hdrlen),
+ nlmsg_attrlen(nlh, hdrlen), policy,
+ NL_VALIDATE_STRICT, extack);
+}
+
/**
* nlmsg_parse_deprecated - parse attributes of a netlink message
* @nlh: netlink message header
@@ -973,6 +1019,25 @@ nla_find_nested(const struct nlattr *nla, int attrtype)
return nla_find(nla_data(nla), nla_len(nla), attrtype);
}
+/**
+ * nla_parse_nested - parse nested attributes
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @nla: attribute containing the nested attributes
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ *
+ * See nla_parse()
+ */
+static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
+ const struct nlattr *nla,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return __nla_parse(tb, maxtype, nla_d
[RFC v2 5/6] genetlink: optionally validate strictly/dumps
From: Johannes Berg
Add options to strictly validate messages and dump messages,
sometimes perhaps validating dump messages non-strictly may
be required, so add an option for that as well.
Since none of this can really be applied to existing commands,
set the options everwhere using the following spatch:
@@
identifier ops;
expression X;
@@
struct genl_ops ops[] = {
...,
{
.cmd = X,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
...
},
...
};
For new commands one should just not copy the .validate 'opt-out'
flags and thus get strict validation.
Signed-off-by: Johannes Berg
---
drivers/block/nbd.c | 4 +
drivers/net/gtp.c| 3 +
drivers/net/ieee802154/mac802154_hwsim.c | 6 ++
drivers/net/macsec.c | 10 +++
drivers/net/team/team.c | 4 +
drivers/net/wireless/mac80211_hwsim.c| 6 ++
drivers/target/target_core_user.c| 4 +
fs/dlm/netlink.c | 1 +
include/net/genetlink.h | 12 +++
kernel/taskstats.c | 2 +
net/batman-adv/netlink.c | 18
net/core/devlink.c | 38 +
net/core/drop_monitor.c | 3 +
net/hsr/hsr_netlink.c| 2 +
net/ieee802154/nl802154.c| 29 +++
net/ipv4/fou.c | 3 +
net/ipv4/tcp_metrics.c | 2 +
net/ipv6/ila/ila_main.c | 4 +
net/ipv6/seg6.c | 4 +
net/l2tp/l2tp_netlink.c | 9 ++
net/ncsi/ncsi-netlink.c | 6 ++
net/netfilter/ipvs/ip_vs_ctl.c | 16
net/netlabel/netlabel_calipso.c | 4 +
net/netlabel/netlabel_cipso_v4.c | 4 +
net/netlabel/netlabel_mgmt.c | 8 ++
net/netlabel/netlabel_unlabeled.c| 8 ++
net/netlink/genetlink.c | 46 +++---
net/nfc/netlink.c| 19 +
net/openvswitch/conntrack.c | 3 +
net/openvswitch/datapath.c | 13 +++
net/openvswitch/meter.c | 4 +
net/psample/psample.c| 1 +
net/smc/smc_pnet.c | 4 +
net/tipc/netlink.c | 21 +
net/tipc/netlink_compat.c| 1 +
net/wimax/stack.c| 4 +
net/wireless/nl80211.c | 104 +++
37 files changed, 420 insertions(+), 10 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0ed58440443f..47cd5dd2bddf 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2003,18 +2003,22 @@ static int nbd_genl_reconfigure(struct sk_buff *skb,
struct genl_info *info)
static const struct genl_ops nbd_connect_genl_ops[] = {
{
.cmd= NBD_CMD_CONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_connect,
},
{
.cmd= NBD_CMD_DISCONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_disconnect,
},
{
.cmd= NBD_CMD_RECONFIGURE,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_reconfigure,
},
{
.cmd= NBD_CMD_STATUS,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_status,
},
};
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index c06e31747288..eaf4311b4004 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1270,16 +1270,19 @@ static const struct nla_policy gtp_genl_policy[GTPA_MAX
+ 1] = {
static const struct genl_ops gtp_genl_ops[] = {
{
.cmd = GTP_CMD_NEWPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_new_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_DELPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_del_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_GETPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_get_pdp,
.dumpit = gtp_genl_dump_pdp,
.flags = GENL_ADMIN_PERM,
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c
b/drivers/net/ieee802154/mac802154_hwsim.c
index bb781820b879..e3963fc50fcc 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -592,31 +592,37 @@ st
[RFC v2 0/6] strict netlink validation
This version gets us to where I wanted to be: strict parsing commandattribute attribute message oldold -- oldnew X- new*XX Additionally, it does lots of cross-tree renames so that we can redefine the "default" nla_parse() etc. to be strict now. Note that I split the patches so we can use full build to validate I actually caught everything, but since I haven't done that right now it probably won't compile and thus needs more changes. johannes
[RFC v2 6/6] nl80211: tag policies with strict_start_type
From: Johannes Berg
Tag all the nl80211 policies with strict_start_type so that
strict validation is done for all types that we have a policy
for.
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d8eaa161c308..5bad1d19c905 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -221,6 +221,7 @@ static int validate_ie_attr(const struct nlattr *attr,
/* policy for the attributes */
static const struct nla_policy
nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_FTM_RESP_ATTR_CIVICLOC + 1 },
[NL80211_FTM_RESP_ATTR_ENABLED] = { .type = NLA_FLAG, },
[NL80211_FTM_RESP_ATTR_LCI] = { .type = NLA_BINARY,
.len = U8_MAX },
@@ -230,6 +231,10 @@ nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX +
1] = {
static const struct nla_policy
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
+ [0] = {
+ .strict_start_type =
+ NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC + 1
+ },
[NL80211_PMSR_FTM_REQ_ATTR_ASAP] = { .type = NLA_FLAG },
[NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE] = { .type = NLA_U32 },
[NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP] =
@@ -246,12 +251,14 @@
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
static const struct nla_policy
nl80211_pmsr_req_data_policy[NL80211_PMSR_TYPE_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_TYPE_FTM + 1 },
[NL80211_PMSR_TYPE_FTM] =
NLA_POLICY_NESTED(nl80211_pmsr_ftm_req_attr_policy),
};
static const struct nla_policy
nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_REQ_ATTR_GET_AP_TSF + 1 },
[NL80211_PMSR_REQ_ATTR_DATA] =
NLA_POLICY_NESTED(nl80211_pmsr_req_data_policy),
[NL80211_PMSR_REQ_ATTR_GET_AP_TSF] = { .type = NLA_FLAG },
@@ -259,6 +266,7 @@ nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1]
= {
static const struct nla_policy
nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_PEER_ATTR_RESP + 1 },
[NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR,
/*
* we could specify this again to be the top-level policy,
@@ -272,6 +280,7 @@ nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX +
1] = {
static const struct nla_policy
nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
+ [0] { .strict_start_type = NL80211_PMSR_ATTR_PEERS + 1 },
[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
@@ -281,6 +290,7 @@ nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
+ [0] = { .strict_start_type = NL80211_ATTR_AIRTIME_WEIGHT + 1 },
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
[NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
.len = 20-1 },
@@ -545,6 +555,7 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
/* policy for the key attributes */
static const struct nla_policy nl80211_key_policy[NL80211_KEY_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_KEY_DEFAULT_TYPES + 1 },
[NL80211_KEY_DATA] = { .type = NLA_BINARY, .len = WLAN_MAX_KEY_LEN },
[NL80211_KEY_IDX] = { .type = NLA_U8 },
[NL80211_KEY_CIPHER] = { .type = NLA_U32 },
@@ -558,6 +569,7 @@ static const struct nla_policy
nl80211_key_policy[NL80211_KEY_MAX + 1] = {
/* policy for the key default flags */
static const struct nla_policy
nl80211_key_default_policy[NUM_NL80211_KEY_DEFAULT_TYPES] = {
+ [0] = { .strict_start_type = NL80211_KEY_DEFAULT_TYPE_MULTICAST + 1 },
[NL80211_KEY_DEFAULT_TYPE_UNICAST] = { .type = NLA_FLAG },
[NL80211_KEY_DEFAULT_TYPE_MULTICAST] = { .type = NLA_FLAG },
};
@@ -566,6 +578,7 @@ nl80211_key_default_policy[NUM_NL80211_KEY_DEFAULT_TYPES] =
{
/* policy for WoWLAN attributes */
static const struct nla_policy
nl80211_wowlan_policy[NUM_NL80211_WOWLAN_TRIG] = {
+ [0] = { .strict_start_type = NL80211_WOWLAN_TRIG_NET_DETECT + 1 },
[NL80211_WOWLAN_TRIG_ANY] = { .type = NLA_FLAG },
[NL80211_WOWLAN_TRIG_DISCONNECT] = { .type = NLA_FLAG },
[NL80211_WOWLAN_TRIG_MAGIC_PKT] = { .type = NLA_FLAG },
@@ -580,6 +593,7 @@ nl80211_wowlan_policy[NUM_NL80211_WOWLAN_TRIG] = {
static const struct nla_policy
nl80211_wowlan_tcp_policy[NUM_NL80211_WOWLAN_TCP] = {
+ [0] = { .strict_start_type = NL80211_WOWLAN_TCP_WAKE_MASK + 1 },
[NL80211_WOWLAN_TCP_SRC_IPV4] = { .type = NLA_U32
[RFC v2 1/6] netlink: add NLA_MIN_LEN
From: Johannes Berg
Rather than using NLA_UNSPEC for this type of thing, use NLA_MIN_LEN
so we can make NLA_UNSPEC be NLA_REJECT under certain conditions for
future attributes.
While at it, also use NLA_EXACT_LEN for the struct example.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +-
lib/nlattr.c | 9 -
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 23f27b0b3cef..06f8605b740c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -183,6 +183,7 @@ enum {
NLA_REJECT,
NLA_EXACT_LEN,
NLA_EXACT_LEN_WARN,
+ NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -212,6 +213,7 @@ enum nla_policy_validation {
*NLA_NUL_STRING Maximum length of string (excluding NUL)
*NLA_FLAG Unused
*NLA_BINARY Maximum length of attribute payload
+ *NLA_MIN_LEN Minimum length of attribute payload
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
@@ -230,6 +232,7 @@ enum nla_policy_validation {
* it is rejected.
*NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
* is logged if it is longer, shorter is rejected.
+ *NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
* Meaning of `validation_data' field:
@@ -281,7 +284,7 @@ enum nla_policy_validation {
* static const struct nla_policy my_policy[ATTR_MAX+1] = {
* [ATTR_FOO] = { .type = NLA_U16 },
* [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
- * [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * [ATTR_BAZ] = { .type = NLA_EXACT_LEN, .len = sizeof(struct mystruct) },
* [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data = &myvalidflags
},
* };
*/
@@ -302,6 +305,7 @@ struct nla_policy {
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
.len = _len }
+#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index d26de6156b97..465c9e8ef8a5 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -278,10 +278,17 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
}
break;
+
+ case NLA_UNSPEC:
+ case NLA_MIN_LEN:
+ if (attrlen < pt->len)
+ goto out_err;
+ break;
+
default:
if (pt->len)
minlen = pt->len;
- else if (pt->type != NLA_UNSPEC)
+ else
minlen = nla_attr_minlen[pt->type];
if (attrlen < minlen)
--
2.17.2
[RFC v2 4/6] netlink: add strict parsing for future attributes
From: Johannes Berg
Unfortunately, we cannot add strict parsing for all attributes, as
that would break existing userspace. We currently warn about it, but
that's about all we can do.
For new attributes, however, the story is better: nobody is using
them, so we can reject bad sizes.
Also, for new attributes, we need not accept them when the policy
doesn't declare their usage.
David Ahern and I went back and forth on how to best encode this, and
the best way we found was to have a "boundary type", from which point
on new attributes
* are strictly checked for length etc.
* NLA_UNSPEC is rejected as invalid, rather than accepting all.
As we didn't want to add another argument to all functions that get a
netlink policy, the workaround is to encode that boundary in the first
entry of the policy array (which is for type 0 and thus probably not
really valid anyway). I put it into the validation union for the rare
possibility that somebody is actually using attribute 0, which would
continue to work fine unless they tried to use the extended validation,
which isn't likely. We also didn't find any in-tree users with type 0.
The reason for setting the "start strict here" attribute is that we
never really need to start strict from 0, which is invalid anyway (or
in legacy families where that isn't true, it cannot be set to strict),
so we can thus reserve the value 0 for "don't do this check" and don't
have to add the tag to all policies right now.
Thus, policies can now opt in to this validation, which we should do
for all existing policies, at least when adding new attributes.
Note that entirely *new* policies won't need to set it, as the use
of that should be using nla_parse()/nlmsg_parse() etc. which anyway
do fully strict validation now, regardless of this.
So in effect, this patch only covers the "existing command with new
attribute" case.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 18 ++
lib/nlattr.c | 7 +--
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 7f368dc13e3d..c6c0f689292a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -299,6 +299,24 @@ struct nla_policy {
};
int (*validate)(const struct nlattr *attr,
struct netlink_ext_ack *extack);
+ /* This entry is special, and used for the attribute at index 0
+* only, and specifies special data about the policy, namely it
+* specifies the "boundary type" where strict length validation
+* starts for any attribute types >= this value, also, strict
+* nesting validation starts here.
+*
+* Additionally, it means that NLA_UNSPEC is actually NLA_REJECT
+* for any types >= this, so need to use NLA_MIN_LEN to get the
+* previous pure { .len = xyz } behaviour. The advantage of this
+* is that types not specified in the policy will be rejected.
+*
+* For completely new families it should be set to 1 so that the
+* validation is enforced for all attributes. For existing ones
+* it should be set at least when new attributes are added to
+* the enum used by the policy, and be set to the new value that
+* was added to enforce strict validation from thereon.
+*/
+ u16 strict_start_type;
};
};
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 17c94a518e8a..28ac8f83996a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -158,12 +158,15 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
const struct nla_policy *policy, bool strict,
struct netlink_ext_ack *extack)
{
+ u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
int err = -ERANGE;
+ enum netlink_validation validate;
+
+ strict = strict || (strict_start_type && type >= strict_start_type);
/* We don't need to worry about MSG_STRICT here, it was never done */
- enum netlink_validation validate =
- strict ? NL_VALIDATE_STRICT : NL_VALIDATE_LIBERAL;
+ validate = strict ? NL_VALIDATE_STRICT : NL_VALIDATE_LIBERAL;
if (type <= 0 || type > maxtype)
return 0;
--
2.17.2
Re: [RFC 1/6] netlink: add nlmsg_validate_strict() & nla_validate_strict()
On Thu, 2019-03-21 at 23:50 +0100, Florian Westphal wrote:
> Johannes Berg wrote:
> > + * nla_validate_strict - Strictly validate a stream of attributes
> > + * @head: head of attribute stream
> > + * @len: length of attribute stream
> > + * @maxtype: maximum attribute type to be expected
> > + * @policy: validation policy
> > + * @extack: extended ACK report struct
> > + *
> > + * Validates all attributes in the specified attribute stream against the
> > + * specified policy. Attributes with a type exceeding maxtype will be
> > + * ignored.
>
>
> rejected?
Oops, right, I didn't pay attentino to the docs at all.
But anyway, I don't think I want to do this.
I'm tempted to do the following:
* add an
enum netlink_validation {
NETLINK_VALIDATION_LIBERAL,// old behaviour
NETLINK_VALIDATION_STRICT_MSG, // current strict
NETLINK_VALIDATION_STRICT, // strict message & attribute
};
* add __*_parse()/__*_validate() that get a new argument from this enum
* for all existing callers of *_parse()/*_validate() add a new inline
*_parse_liberal()/*_validate_liberal() and replace all calls, using
_LIBERAL
* change all existing *_parse_strict() to a new *_parse_strict_msg()
inline using _STRICT_MSG
* re-introduce *_parse()/*_validate() as being fully _STRICT
Also, do this before the generic netlink changes, so generic netlink
never gets the intermediate "STRICT_MSG" level.
That addresses two things:
1) my table from the cover letter would be - at least for genl - what
I want it to be, for some rtnetlink commands we'd have "strict_msg"
semantics
2) Default of *_parse()/*_validate() becomes to be strict for new
code, so we don't need to pay as much attention to it - it'll be
easier to see if somebody adds a call explicitly calling the more
liberal versions. I'm tempted to not even add inline wrappers for
this reason but to just open-code the __*() versions with the enum
value instead (it's spatch, after all).
PS: STRICT_MSG (currently _strict()) semantics are a bit strange because
an attribute type that's out of range is rejected, while one that's in
range but has no policy is accepted; yet the range is prone to change
all the time... The "strict_start_type" fixes that though, if applied.
johannes
[RFC 5/6] netlink: add strict parsing for future attributes
From: Johannes Berg
Unfortunately, we cannot add strict parsing for all attributes, as
that would break existing userspace. We currently warn about it, but
that's about all we can do.
For new attributes, however, the story is better: nobody is using
them, so we can reject bad sizes.
Also, for new attributes, we need not accept them when the policy
doesn't declare their usage.
David Ahern and I went back and forth on how to best encode this, and
the best way we found was to have a "boundary type", from which point
on new attributes
* are strictly checked for length etc.
* NLA_UNSPEC is rejected as invalid, rather than accepting all.
As we didn't want to add another argument to all functions that get a
netlink policy, the workaround is to encode that boundary in the first
entry of the policy array (which is for type 0 and thus probably not
really valid anyway). I put it into the validation union for the rare
possibility that somebody is actually using attribute 0, which would
continue to work fine unless they tried to use the extended validation,
which isn't likely. We also didn't find any in-tree users with type 0.
The reason for setting the "start strict here" attribute is that we
never really need to start strict from 0, which is invalid anyway (or
in legacy families where that isn't true, it cannot be set to strict),
so we can thus reserve the value 0 for "don't do this check" and don't
have to add the tag to all policies right now.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 18 ++
lib/nlattr.c | 35 +--
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index ba79dd0cd23c..2f759867a264 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -301,6 +301,24 @@ struct nla_policy {
};
int (*validate)(const struct nlattr *attr,
struct netlink_ext_ack *extack);
+ /* This entry is special, and used for the attribute at index 0
+* only, and specifies special data about the policy, namely it
+* specifies the "boundary type" where strict length validation
+* starts for any attribute types >= this value, also, strict
+* nesting validation starts here.
+*
+* Additionally, it means that NLA_UNSPEC is actually NLA_REJECT
+* for any types >= this, so need to use NLA_MIN_LEN to get the
+* previous pure { .len = xyz } behaviour. The advantage of this
+* is that types not specified in the policy will be rejected.
+*
+* For completely new families it should be set to 1 so that the
+* validation is enforced for all attributes. For existing ones
+* it should be set at least when new attributes are added to
+* the enum used by the policy, and be set to the new value that
+* was added to enforce strict validation from thereon.
+*/
+ u16 strict_start_type;
};
};
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 34d53e772779..439ac33d5692 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
const struct nla_policy *policy,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack,
+ bool strict)
{
const struct nlattr *entry;
int rem;
@@ -86,8 +87,13 @@ static int nla_validate_array(const struct nlattr *head, int
len, int maxtype,
return -ERANGE;
}
- ret = nla_validate(nla_data(entry), nla_len(entry),
- maxtype, policy, extack);
+ if (strict)
+ ret = nla_validate_strict(nla_data(entry),
+ nla_len(entry),
+ maxtype, policy, extack);
+ else
+ ret = nla_validate(nla_data(entry), nla_len(entry),
+ maxtype, policy, extack);
if (ret < 0)
return ret;
}
@@ -157,9 +163,11 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack)
{
+ u16 strict_start_type = policy[0].strict_start_type;
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
[RFC 3/6] netlink: add NLA_MIN_LEN
From: Johannes Berg
Rather than using NLA_UNSPEC for this type of thing, use NLA_MIN_LEN
so we can make NLA_UNSPEC be NLA_REJECT under certain conditions for
future attributes.
While at it, also use NLA_EXACT_LEN for the struct example.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 6 +-
lib/nlattr.c | 9 -
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index e211b745008a..ba79dd0cd23c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -185,6 +185,7 @@ enum {
NLA_REJECT,
NLA_EXACT_LEN,
NLA_EXACT_LEN_WARN,
+ NLA_MIN_LEN,
__NLA_TYPE_MAX,
};
@@ -214,6 +215,7 @@ enum nla_policy_validation {
*NLA_NUL_STRING Maximum length of string (excluding NUL)
*NLA_FLAG Unused
*NLA_BINARY Maximum length of attribute payload
+ *NLA_MIN_LEN Minimum length of attribute payload
*NLA_NESTED,
*NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
@@ -232,6 +234,7 @@ enum nla_policy_validation {
* it is rejected.
*NLA_EXACT_LEN_WARN Attribute should have exactly this length, a warning
* is logged if it is longer, shorter is rejected.
+ *NLA_MIN_LEN Minimum length of attribute payload
*All otherMinimum length of attribute payload
*
* Meaning of `validation_data' field:
@@ -283,7 +286,7 @@ enum nla_policy_validation {
* static const struct nla_policy my_policy[ATTR_MAX+1] = {
* [ATTR_FOO] = { .type = NLA_U16 },
* [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
- * [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * [ATTR_BAZ] = { .type = NLA_EXACT_LEN, .len = sizeof(struct mystruct) },
* [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data = &myvalidflags
},
* };
*/
@@ -304,6 +307,7 @@ struct nla_policy {
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len }
#define NLA_POLICY_EXACT_LEN_WARN(_len){ .type = NLA_EXACT_LEN_WARN, \
.len = _len }
+#define NLA_POLICY_MIN_LEN(_len) { .type = NLA_MIN_LEN, .len = _len }
#define NLA_POLICY_ETH_ADDRNLA_POLICY_EXACT_LEN(ETH_ALEN)
#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 3ccef39b89de..34d53e772779 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -278,10 +278,17 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
}
}
break;
+
+ case NLA_UNSPEC:
+ case NLA_MIN_LEN:
+ if (attrlen < pt->len)
+ goto out_err;
+ break;
+
default:
if (pt->len)
minlen = pt->len;
- else if (pt->type != NLA_UNSPEC)
+ else
minlen = nla_attr_minlen[pt->type];
if (attrlen < minlen)
--
2.17.2
[RFC 0/7] netlink/genetlink: stricter parsing
It seems that everytime David and I meet, we have further ideas for netlink ;-) This time, we talked about attribute parsing and making policy parsing (even) stricter. I'm not entirely happy with this patchset, due to the mistake David made when he added the strict parsing. At that time, the strict parsing *should* have extended to all the attributes, in the way that I'm doing it here with the strict_start_type (in the last patch). The reason I'm still not happy with this is that it seems to me that even for *existing* attributes, we should apply the strict checks for *new* commands. So, in effect, I think we should have strict parsing commandattribute attribute message oldold -- oldnew X- new*XX However, what we get with these patches, is only strict parsing commandattribute attribute message oldold -- oldnew X- newold - (!)X newnew XX Note the marked difference. I see two ways to fix this now: 1) Replace all existing usage of *_strict() calls again with the regular ones, and change the *_strict() calls to pass strict through to the validate_nla() function. This would be sort of a regression since all things that are actually strict now would no longer be. I think that would even be an API break. 2) Add another level of "very_strict" to use in new commands, which then passes the strict through to validate_nla(). This probably means we want to pass the "strictness" as an enum argument instead. I'll take a shot at this later. Further ideas we discussed: * Have ways to expose the policies to userspace, with generic netlink that's easy, with others perhaps not quite so easy; this would allow userspace to discover what the kernel supports. * Validate messages created by the kernel against the policy, for debug purposes. This is a bit harder because the policy is sometimes not even symmetric (input and output have different data), but that could be managed (e.g. have a list of override policy entries) and it'd help us validate things too. johannes
[RFC 4/6] genetlink: optionally validate strictly/dumps
From: Johannes Berg
Add options to strictly validate messages and dump messages,
sometimes perhaps validating dump messages non-strictly may
be required, so add an option for that as well.
Since none of this can really be applied to existing commands,
set the options everwhere using the following spatch:
@@
identifier ops;
expression X;
@@
struct genl_ops ops[] = {
...,
{
.cmd = X,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
...
},
...
};
Signed-off-by: Johannes Berg
---
drivers/block/nbd.c | 4 +
drivers/net/gtp.c| 3 +
drivers/net/ieee802154/mac802154_hwsim.c | 6 ++
drivers/net/macsec.c | 10 +++
drivers/net/team/team.c | 4 +
drivers/net/wireless/mac80211_hwsim.c| 6 ++
drivers/target/target_core_user.c| 4 +
fs/dlm/netlink.c | 1 +
include/net/genetlink.h | 12 +++
kernel/taskstats.c | 2 +
net/batman-adv/netlink.c | 18
net/core/devlink.c | 38 +
net/core/drop_monitor.c | 3 +
net/hsr/hsr_netlink.c| 2 +
net/ieee802154/nl802154.c| 29 +++
net/ipv4/fou.c | 3 +
net/ipv4/tcp_metrics.c | 2 +
net/ipv6/ila/ila_main.c | 4 +
net/ipv6/seg6.c | 4 +
net/l2tp/l2tp_netlink.c | 9 ++
net/ncsi/ncsi-netlink.c | 6 ++
net/netfilter/ipvs/ip_vs_ctl.c | 16
net/netlabel/netlabel_calipso.c | 4 +
net/netlabel/netlabel_cipso_v4.c | 4 +
net/netlabel/netlabel_mgmt.c | 8 ++
net/netlabel/netlabel_unlabeled.c| 8 ++
net/netlink/genetlink.c | 43 --
net/nfc/netlink.c| 19 +
net/openvswitch/conntrack.c | 3 +
net/openvswitch/datapath.c | 13 +++
net/openvswitch/meter.c | 4 +
net/psample/psample.c| 1 +
net/smc/smc_pnet.c | 4 +
net/tipc/netlink.c | 21 +
net/tipc/netlink_compat.c| 1 +
net/wimax/stack.c| 4 +
net/wireless/nl80211.c | 104 +++
37 files changed, 418 insertions(+), 9 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index d3bd1b097a94..7c85fc5c9837 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1999,18 +1999,22 @@ static int nbd_genl_reconfigure(struct sk_buff *skb,
struct genl_info *info)
static const struct genl_ops nbd_connect_genl_ops[] = {
{
.cmd= NBD_CMD_CONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_connect,
},
{
.cmd= NBD_CMD_DISCONNECT,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_disconnect,
},
{
.cmd= NBD_CMD_RECONFIGURE,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_reconfigure,
},
{
.cmd= NBD_CMD_STATUS,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = nbd_genl_status,
},
};
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index c06e31747288..eaf4311b4004 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1270,16 +1270,19 @@ static const struct nla_policy gtp_genl_policy[GTPA_MAX
+ 1] = {
static const struct genl_ops gtp_genl_ops[] = {
{
.cmd = GTP_CMD_NEWPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_new_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_DELPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_del_pdp,
.flags = GENL_ADMIN_PERM,
},
{
.cmd = GTP_CMD_GETPDP,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.doit = gtp_genl_get_pdp,
.dumpit = gtp_genl_dump_pdp,
.flags = GENL_ADMIN_PERM,
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c
b/drivers/net/ieee802154/mac802154_hwsim.c
index 49866737f138..8dc8a154dcb8 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -598,31 +598,37 @@ static const struct nla_policy
hwsim_genl_policy[MAC802154_HWSIM_ATTR_MAX + 1] =
static const struct genl_ops
[RFC 6/6] nl80211: tag policies with strict_start_type
From: Johannes Berg
Tag all the nl80211 policies with strict_start_type so that
strict validation is done for all types that we have a policy
for.
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d9145bb2b3dc..236cec87d367 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -221,6 +221,7 @@ static int validate_ie_attr(const struct nlattr *attr,
/* policy for the attributes */
static const struct nla_policy
nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_FTM_RESP_ATTR_CIVICLOC + 1 },
[NL80211_FTM_RESP_ATTR_ENABLED] = { .type = NLA_FLAG, },
[NL80211_FTM_RESP_ATTR_LCI] = { .type = NLA_BINARY,
.len = U8_MAX },
@@ -230,6 +231,10 @@ nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX +
1] = {
static const struct nla_policy
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
+ [0] = {
+ .strict_start_type =
+ NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC + 1
+ },
[NL80211_PMSR_FTM_REQ_ATTR_ASAP] = { .type = NLA_FLAG },
[NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE] = { .type = NLA_U32 },
[NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP] =
@@ -246,12 +251,14 @@
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
static const struct nla_policy
nl80211_pmsr_req_data_policy[NL80211_PMSR_TYPE_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_TYPE_FTM + 1 },
[NL80211_PMSR_TYPE_FTM] =
NLA_POLICY_NESTED(nl80211_pmsr_ftm_req_attr_policy),
};
static const struct nla_policy
nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_REQ_ATTR_GET_AP_TSF + 1 },
[NL80211_PMSR_REQ_ATTR_DATA] =
NLA_POLICY_NESTED(nl80211_pmsr_req_data_policy),
[NL80211_PMSR_REQ_ATTR_GET_AP_TSF] = { .type = NLA_FLAG },
@@ -259,6 +266,7 @@ nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1]
= {
static const struct nla_policy
nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_PMSR_PEER_ATTR_RESP + 1 },
[NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR,
/*
* we could specify this again to be the top-level policy,
@@ -272,6 +280,7 @@ nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX +
1] = {
static const struct nla_policy
nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
+ [0] { .strict_start_type = NL80211_PMSR_ATTR_PEERS + 1 },
[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
@@ -281,6 +290,7 @@ nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
+ [0] = { .strict_start_type = NL80211_ATTR_AIRTIME_WEIGHT + 1 },
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
[NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
.len = 20-1 },
@@ -545,6 +555,7 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
/* policy for the key attributes */
static const struct nla_policy nl80211_key_policy[NL80211_KEY_MAX + 1] = {
+ [0] = { .strict_start_type = NL80211_KEY_DEFAULT_TYPES + 1 },
[NL80211_KEY_DATA] = { .type = NLA_BINARY, .len = WLAN_MAX_KEY_LEN },
[NL80211_KEY_IDX] = { .type = NLA_U8 },
[NL80211_KEY_CIPHER] = { .type = NLA_U32 },
@@ -558,6 +569,7 @@ static const struct nla_policy
nl80211_key_policy[NL80211_KEY_MAX + 1] = {
/* policy for the key default flags */
static const struct nla_policy
nl80211_key_default_policy[NUM_NL80211_KEY_DEFAULT_TYPES] = {
+ [0] = { .strict_start_type = NL80211_KEY_DEFAULT_TYPE_MULTICAST + 1 },
[NL80211_KEY_DEFAULT_TYPE_UNICAST] = { .type = NLA_FLAG },
[NL80211_KEY_DEFAULT_TYPE_MULTICAST] = { .type = NLA_FLAG },
};
@@ -566,6 +578,7 @@ nl80211_key_default_policy[NUM_NL80211_KEY_DEFAULT_TYPES] =
{
/* policy for WoWLAN attributes */
static const struct nla_policy
nl80211_wowlan_policy[NUM_NL80211_WOWLAN_TRIG] = {
+ [0] = { .strict_start_type = NL80211_WOWLAN_TRIG_NET_DETECT + 1 },
[NL80211_WOWLAN_TRIG_ANY] = { .type = NLA_FLAG },
[NL80211_WOWLAN_TRIG_DISCONNECT] = { .type = NLA_FLAG },
[NL80211_WOWLAN_TRIG_MAGIC_PKT] = { .type = NLA_FLAG },
@@ -580,6 +593,7 @@ nl80211_wowlan_policy[NUM_NL80211_WOWLAN_TRIG] = {
static const struct nla_policy
nl80211_wowlan_tcp_policy[NUM_NL80211_WOWLAN_TCP] = {
+ [0] = { .strict_start_type = NL80211_WOWLAN_TCP_WAKE_MASK + 1 },
[NL80211_WOWLAN_TCP_SRC_IPV4] = { .type = NLA_U32
[RFC 2/6] genetlink: add genlmsg_parse_strict()
From: Johannes Berg
Signed-off-by: Johannes Berg
---
include/net/genetlink.h | 19 +++
1 file changed, 19 insertions(+)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 6850c7b1a3a6..4a25c341244c 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -183,6 +183,25 @@ static inline int genlmsg_parse(const struct nlmsghdr *nlh,
policy, extack);
}
+/**
+ * genlmsg_parse_strict - strictly parse attributes of a genetlink message
+ * @nlh: netlink message header
+ * @family: genetlink message family
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ */
+static inline int genlmsg_parse_strict(const struct nlmsghdr *nlh,
+ const struct genl_family *family,
+ struct nlattr *tb[], int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ return nlmsg_parse_strict(nlh, family->hdrsize + GENL_HDRLEN, tb,
+ maxtype, policy, extack);
+}
+
/**
* genl_dump_check_consistent - check if sequence is consistent and advertise
if not
* @cb: netlink callback structure that stores the sequence number
--
2.17.2
[RFC 1/6] netlink: add nlmsg_validate_strict() & nla_validate_strict()
From: Johannes Berg
These are needed since we want to separate validation and parsing
in some cases, e.g. in generic netlink to ensure that dump messages
are valid, but we don't typically parse them.
Signed-off-by: Johannes Berg
---
include/net/netlink.h | 26 +
lib/nlattr.c | 45 +++
2 files changed, 71 insertions(+)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 23f27b0b3cef..e211b745008a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -66,6 +66,7 @@
* nlmsg_find_attr() find an attribute in a message
* nlmsg_for_each_msg() loop over all messages
* nlmsg_validate() validate netlink message incl. attrs
+ * nlmsg_validate_strict() strictly validate netlink message/attrs
* nlmsg_for_each_attr() loop over all attributes
*
* Misc:
@@ -149,6 +150,7 @@
* nla_ok(nla, remaining)does nla fit into remaining bytes?
* nla_next(nla, remaining) get next netlink attribute
* nla_validate()validate a stream of attributes
+ * nla_validate_strict() strictly validate a stream of attributes
* nla_validate_nested() validate a stream of nested attributes
* nla_find()find attribute in stream of
attributes
* nla_find_nested() find attribute in nested attributes
@@ -374,6 +376,9 @@ int nlmsg_notify(struct sock *sk, struct sk_buff *skb, u32
portid,
int nla_validate(const struct nlattr *head, int len, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack);
+int nla_validate_strict(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack);
int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
int len, const struct nla_policy *policy,
struct netlink_ext_ack *extack);
@@ -582,6 +587,27 @@ static inline int nlmsg_validate(const struct nlmsghdr
*nlh,
extack);
}
+/**
+ * nlmsg_validate_strict - strictly validate a netlink message including attrs
+ * @nlh: netlinket message header
+ * @hdrlen: length of familiy specific header
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ */
+static inline int nlmsg_validate_strict(const struct nlmsghdr *nlh,
+ int hdrlen, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
+ return -EINVAL;
+
+ return nla_validate_strict(nlmsg_attrdata(nlh, hdrlen),
+ nlmsg_attrlen(nlh, hdrlen), maxtype,
+ policy, extack);
+}
+
/**
* nlmsg_report - need to report back to application?
* @nlh: netlink message header
diff --git a/lib/nlattr.c b/lib/nlattr.c
index d26de6156b97..3ccef39b89de 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -347,6 +347,51 @@ int nla_validate(const struct nlattr *head, int len, int
maxtype,
}
EXPORT_SYMBOL(nla_validate);
+/**
+ * nla_validate_strict - Strictly validate a stream of attributes
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ * @extack: extended ACK report struct
+ *
+ * Validates all attributes in the specified attribute stream against the
+ * specified policy. Attributes with a type exceeding maxtype will be
+ * ignored. See documenation of struct nla_policy for more details.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+int nla_validate_strict(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ const struct nlattr *nla;
+ int rem;
+
+ nla_for_each_attr(nla, head, len, rem) {
+ u16 type = nla_type(nla);
+ int err;
+
+ if (type == 0 || type > maxtype) {
+ NL_SET_ERR_MSG(extack, "Unknown attribute type");
+ return -EINVAL;
+ }
+
+ err = validate_nla(nla, maxtype, policy, extack);
+
+ if (err < 0)
+ return err;
+ }
+
+ if (unlikely(rem > 0)) {
+ NL_SET_ERR_MSG(extack, "bytes leftover after parsing
attributes");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(nla_validate_strict);
+
/**
* nla_policy_len - Determin the max. length of a policy
* @policy: policy to use
--
2.17.2
[PATCH] genetlink: make policy common to family
From: Johannes Berg
Since maxattr is common, the policy can't really differ sanely,
so make it common as well.
The only user that did in fact manage to make a non-common policy
is taskstats, which has to be really careful about it (since it's
still using a common maxattr!). This is no longer supported, but
we can fake it using pre_doit.
This reduces the size of e.g. nl80211.o (which has lots of commands):
textdata bss dec hex filename
398745 143232240 415308 6564c net/wireless/nl80211.o (before)
397913 143312240 414484 65314 net/wireless/nl80211.o (after)
-832 +8 0-824
Which is obviously just 8 bytes for each command, and an added 8
bytes for the new policy pointer. I'm not sure why the ops list is
counted as .text though.
Most of the code transformations were done using the following spatch:
@ops@
identifier OPS;
expression POLICY;
@@
struct genl_ops OPS[] = {
...,
{
- .policy = POLICY,
},
...
};
@@
identifier ops.OPS;
expression ops.POLICY;
identifier fam;
expression M;
@@
struct genl_family fam = {
.ops = OPS,
.maxattr = M,
+ .policy = POLICY,
...
};
This also gets rid of devlink_nl_cmd_region_read_dumpit() accessing
the cb->data as ops, which we want to change in a later genl patch.
Signed-off-by: Johannes Berg
---
drivers/block/nbd.c | 5 +-
drivers/net/gtp.c| 4 +-
drivers/net/ieee802154/mac802154_hwsim.c | 7 +-
drivers/net/macsec.c | 11 +--
drivers/net/team/team.c | 5 +-
drivers/net/wireless/mac80211_hwsim.c| 7 +-
drivers/target/target_core_user.c| 5 +-
include/linux/genl_magic_func.h | 4 +-
include/net/genetlink.h | 4 +-
kernel/taskstats.c | 28 +-
net/batman-adv/netlink.c | 19 +---
net/core/devlink.c | 43 +-
net/hsr/hsr_netlink.c| 3 +-
net/ieee802154/ieee802154.h | 2 -
net/ieee802154/netlink.c | 1 +
net/ieee802154/nl802154.c| 30 +--
net/ipv4/fou.c | 4 +-
net/ipv4/tcp_metrics.c | 3 +-
net/ipv6/ila/ila_main.c | 5 +-
net/ipv6/seg6.c | 5 +-
net/l2tp/l2tp_netlink.c | 10 +--
net/ncsi/ncsi-netlink.c | 7 +-
net/netfilter/ipvs/ip_vs_ctl.c | 13 +--
net/netlabel/netlabel_calipso.c | 5 +-
net/netlabel/netlabel_cipso_v4.c | 5 +-
net/netlabel/netlabel_mgmt.c | 9 +-
net/netlabel/netlabel_unlabeled.c| 9 +-
net/netlink/genetlink.c | 6 +-
net/nfc/netlink.c| 20 +
net/openvswitch/conntrack.c | 4 +-
net/openvswitch/datapath.c | 17 +---
net/openvswitch/meter.c | 5 +-
net/smc/smc_pnet.c | 5 +-
net/tipc/netlink.c | 22 +
net/wimax/stack.c| 5 +-
net/wireless/nl80211.c | 105 +--
36 files changed, 68 insertions(+), 374 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7c9a949e876b..d3bd1b097a94 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1999,22 +1999,18 @@ static int nbd_genl_reconfigure(struct sk_buff *skb,
struct genl_info *info)
static const struct genl_ops nbd_connect_genl_ops[] = {
{
.cmd= NBD_CMD_CONNECT,
- .policy = nbd_attr_policy,
.doit = nbd_genl_connect,
},
{
.cmd= NBD_CMD_DISCONNECT,
- .policy = nbd_attr_policy,
.doit = nbd_genl_disconnect,
},
{
.cmd= NBD_CMD_RECONFIGURE,
- .policy = nbd_attr_policy,
.doit = nbd_genl_reconfigure,
},
{
.cmd= NBD_CMD_STATUS,
- .policy = nbd_attr_policy,
.doit = nbd_genl_status,
},
};
@@ -2031,6 +2027,7 @@ static struct genl_family nbd_genl_family __ro_after_init
= {
.ops= nbd_connect_genl_ops,
.n_ops = ARRAY_SIZE(nbd_connect_genl_ops),
.maxattr= NBD_ATTR_MAX,
+ .policy = nbd_attr_policy,
.mcgrps = nbd_mcast_grps,
.n_mcgrps = ARRAY_SIZE(nbd_mcast_grps),
};
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 7a145172d503..c06e31747288 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1271,20 +1271,17 @@ static const struct genl_ops gtp_genl_ops[] = {
{
.cmd = GTP_CMD_NEWPDP,
Re: [PATCH] rsi: Fix NULL pointer dereference in kmalloc
On Sat, 2019-03-02 at 14:31 -0600, Aditya Pakki wrote:
> kmalloc can fail in rsi_register_rates_channels but memcpy still attempts
> to write to channels. The patch checks and avoids such a situation.
>
> Signed-off-by: Aditya Pakki
> ---
> drivers/net/wireless/rsi/rsi_91x_mac80211.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
> b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
> index e56fc83faf0e..59eb1f533d0e 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
> @@ -197,6 +197,11 @@ static void rsi_register_rates_channels(struct rsi_hw
> *adapter, int band)
>
> if (band == NL80211_BAND_2GHZ) {
> channels = kmalloc(sizeof(rsi_2ghz_channels), GFP_KERNEL);
> + if (!channels) {
> + rsi_dbg(ERR_ZONE, "Failed to allocate memory\n");
> + return;
> + }
> +
> memcpy(channels,
> rsi_2ghz_channels,
> sizeof(rsi_2ghz_channels));
Should probably be kmemdup() anyway though.
johannes
Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames
> Let us briefly describe our test setup to ensure everyone on this mailing > list is one the same page. > > Our general setup looks like this: > 1 $ iw wlp1s0 info > Interface wlp1s0 > ifindex 5 > wdev 0x1 > addr 4c:5e:0c:11:43:ac > type managed > wiphy 0 > txpower 30.00 dBm > 1 $ iw phy phy0 interface add mon0 type monitor > 1 $ iw phy phy0 interface add mon1 type monitor > > When we send (raw) packets on mon0 using packetspammer [1] and listen on > the _other_ monitor mode interface mon1, we receive frames that were sent > on the first one: > 1 $ packetspammer mon0 > 2 $ tcpdump -i mon1 'wlan addr2 13:22:33:44:55:66' > > This is due to the fact that frames sent on mon0 are echoed back as TX > status frames, because REQ_TX_STATUS is always set for frames sent from > monitor mode interfaces. Yes, I understand :-) > But when we replace mon0 with an interface in managed mode (wlp1s0), the > receipt of frames stops, because in managed mode REQ_TX_STATUS is cleared > in most frames: > 1 $ ifup wlp1s0 > 1 $ ping -I wlp1s0 192.168.254.1 # this address is not assigned to any host > 2 $ tcpdump -i mon1 ‚wlan addr2 4c:5e:0c:11:43:ac‘ Yes, also understand. > > What you're proposing is to use IFF_ECHO to show frames transmitted > > through *other* interfaces on the monitor interface. > > > > I don’t think the IFF_ECHO semantics really match this. > > What we propose is to use IFF_ECHO to force REQ_TX_STATUS being set for all > frames sent on the interface. But you are right: The goal is that frames > transmitted through the other interface show up on the monitor interface > (but only after passing the driver). However, this is exactly how we > understand the semantics of IFF_ECHO in the kernel documentation. I disagree. First of all, IFF_ECHO is only documented/used *inside* the kernel, and cannot be set by userspace today. It's documented by CAN as such: Documentation/networking/can.rst: Local Loopback of Sent Frames - As described in :ref:`socketcan-local-loopback1` the CAN network device driver should support a local loopback functionality similar to the local echo e.g. of tty devices. In this case the driver flag IFF_ECHO has to be set to prevent the PF_CAN core from locally echoing sent frames (aka loopback) as fallback solution:: dev->flags = (IFF_NOARP | IFF_ECHO); Note that everything here is specific to a single interface. Also note that it's a signal from the *driver* to the *stack* to not do the loopback itself, because the driver will do it. I think in the case of all other sockets/interfaces, the stack will do the echo anyway, for tcpdump etc. purposes. The documentation in the uapi just states: @IFF_ECHO: echo sent packets. Volatile. and makes no representation about which interface, but I'd argue that all the flags are specific to a single interface and thus you'd expect this to also be. Thus, I don't think this was ever intended for any cross-interface behaviour, even if it may be on the same physical NIC. > As far as we know, drivers must return a TX status frame, if REQ_TX_STATUS > is set, but can do whatever they want, if it is clear. Not all drivers can and do this, I believe. Some things don't work very well if they don't do it, but I _think_ you've just been lucky and used hardware that does in fact support it. Also note that for some hardware that does support this, there's sometimes significant overhead - not just the performance overhead of actually reporting the frames, but sometimes also overhead in how the hardware is programmed and used, and how TX status is extracted. > This is no problem for our > functionality, because we force the delivery of TX status frames by > permanently setting REQ_TX_STATUS. As long as the semantics of > REQ_TX_STATUS remains like it is now, the functionality will always be > as expected from our API. Sure, for now, for your specific case of ath9k :-) > We could also achieve the functionality by modifying the drivers but this > would mean that we had to add this functionality to every driver. > Moreover, the feature of TX status frames, how it is implemented currently > for monitor mode interfaces, is part of the mac80211 implementation. The > decision to force TX status frames for monitor mode interfaces is made in > the common mac80211 implementation. I suppose it could be in mac80211 (perhaps debugfs?) too. I just really don't think IFF_ECHO is the right approach. johannes
Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames
On Tue, 2019-02-26 at 14:13 +0100, Julius Niedworok wrote:
>
> Thank you for the explanation - I can adjust the comment, if you like to.
>
> > So what are you getting back after you enabled IFF_ECHO on your mac80211
> > device?
> >
> > Is it just a 'status' about a sent packet, or is it the packet ('full
> > content') itself?
>
> We are actually getting back the full content of the packet. So it
> matches the behaviour of the 'echo' in CAN.
I don't think it does, really.
In CAN, if I understand correctly, this is used for regular operation
interfaces, where you might want to run 'tcpdump', on wifi the
equivalent would be 'tcpdump -i wlan0'. This *already* implements full
visibility of outgoing and incoming frames.
Not sure how CAN even manages *not to*, but I don't really need to care
:-)
You're proposing to add this to the *monitor* interfaces and you really
should have made the flag conditional on that to make that clear.
However, even on monitor interfaces, you typically *already* see the
frames you transmitted there (as raw frames, which is the only thing you
can do).
What you're proposing is to use IFF_ECHO to show frames transmitted
through *other* interfaces on the monitor interface.
I don't think the IFF_ECHO semantics really match this.
Additionally, drivers are sort of free to ignore the REQ_TX_STATUS, or
we could in the future add ways of using the _noskb to feed back TX
status to the state machines where needed, so I'm not really sure I even
_want_ this to be set in stone in such an API.
Now, I can also see how this can be useful for debugging, but it feels
to me like this should be a driver (debug) option?
johannes
pull-request: mac80211-next 2019-02-22
Hi Dave, Here's the promised update with the big multi-BSSID changes, which are related to HE (802.11ax) for which we also have some updates. Note that I took the liberty of including Herbert's rhashtable API removal, I merged net-next for that and verified that the API he removed is also not used in anything new, but if you apply/applied any patches after I pulled you should probably double check that again. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit 7a25c6c0aac85bbc50d3ce49cd0adb14508b: rocker: Add missing break for PRE_BRIDGE_FLAGS (2019-02-21 21:29:23 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git tags/mac80211-next-for-davem-2019-02-22 for you to fetch changes up to 6c4128f658571b2dc7e01058ad09a8e947bc0159: rhashtable: Remove obsolete rhashtable_walk_init function (2019-02-22 13:49:00 +0100) This time we have, of note: * the massive patch series for multi-BSSID support, I ended up applying that through a side branch to record some details * CSA improvements * HE (802.11ax) updates to Draft 3.3 * strongly typed element iteration/etc. to make such code more readable - this came up in particular in multi-BSSID * rhashtable conversion patches from Herbert Along, as usual, with various fixes and improvements. Cody Schuffelen (1): virt_wifi: Remove REGULATORY_WIPHY_SELF_MANAGED Herbert Xu (2): mac80211: Use rhashtable_lookup_get_insert_fast instead of racy code rhashtable: Remove obsolete rhashtable_walk_init function Johannes Berg (13): cfg80211: add and use strongly typed element iteration macros cfg80211: use for_each_element() for multi-bssid parsing mac80211: use element iteration macro in parsing cfg80211: add various struct element finding helpers nl80211: use for_each_element() in validate_ie_attr() cfg80211: add missing kernel-doc for multi-BSSID fields Merge branch 'cfg80211-mac80211-multi-bssid' into mac80211-next ieee80211: fix for_each_element_extid() cfg80211: restore regulatory without calling userspace cfg80211: fix and clean up cfg80211_gen_new_bssid() radiotap: add 0-length PSDU "not captured" type cfg80211: allow sending vendor events unicast Merge remote-tracking branch 'net-next/master' into mac80211-next Jouni Malinen (4): mac80211_hwsim: Support boottime in scan results mac80211_hwsim: Declare support for Multi-BSSID cfg80211: Use const more consistently in for_each_element macros cfg80211: Report Association Request frame IEs in association events Liad Kaufman (2): mac80211: fix position of vendor_data read mac80211: update HE IEs to D3.3 Mao Wenan (1): cfg80211: pmsr: use eth_broadcast_addr() to assign broadcast address Peng Xu (1): cfg80211: Parsing of Multiple BSSID information in scanning Sara Sharon (16): mac80211: pass bssids to elements parsing function mac80211: move the bss update from elements to an helper cfg80211: Properly track transmitting and non-transmitting BSS cfg80211: Move Multiple BSS info to struct cfg80211_bss to be visible cfg80211: parse multi-bssid only if HW supports it cfg80211: make BSSID generation function inline cfg80211: save multi-bssid properties mac80211: support multi-bssid mac80211: indicate support for multiple BSSID cfg80211: fix the IE inheritance of extension IEs cfg80211: fix memory leak of new_ie mac80211: support max channel switch time element mac80211: abort CSA if beacon does not include CSA IEs mac80211: notify driver on subsequent CSA beacons mac80211: allow CSA to self with immediate quiet mac80211: ignore quiet mode in probe drivers/net/wireless/intel/iwlwifi/fw/api/mac.h| 26 +- drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c | 58 +- drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 40 ++ drivers/net/wireless/mac80211_hwsim.c | 7 +- drivers/net/wireless/virt_wifi.c | 1 - include/linux/ieee80211.h | 115 +++- include/linux/rhashtable.h | 8 - include/net/cfg80211.h | 233 +++- include/net/ieee80211_radiotap.h | 3 +- include/net/mac80211.h | 40 +- lib/rhashtable.c | 2 +- lib/test_rhashtable.c | 9 +- net/ipv6/ila/ila_xlat.c| 15 +- net/mac80211/debugfs.c | 4 +- net/mac80211/debugfs_sta.c | 35 +- net/mac80211/driver
pull-request: mac80211 2019-02-22
Hi Dave, So ... I thought there weren't going to be any more patches when I asked you to apply that mac80211 one, but now a few days later I still ended up with three that are current and possibly even stable material. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit 6321aa197547da397753757bd84c6ce64b3e3d89: phonet: fix building with clang (2019-02-21 16:23:56 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2019-02-22 for you to fetch changes up to 51d0af222f6fa43134c6187ab4f374630f6e0d96: mac80211: allocate tailroom for forwarded mesh packets (2019-02-22 14:00:40 +0100) Three more fixes: * mac80211 mesh code wasn't allocating SKB tailroom properly in some cases * tx_sk_pacing_shift should be 7 for better performance * mac80211_hwsim wasn't propagating genlmsg_reply() errors Felix Fietkau (1): mac80211: allocate tailroom for forwarded mesh packets Li RongQing (1): mac80211_hwsim: propagate genlmsg_reply return code Toke Høiland-Jørgensen (1): mac80211: Change default tx_sk_pacing_shift to 7 drivers/net/wireless/mac80211_hwsim.c | 2 +- net/mac80211/main.c | 4 ++-- net/mac80211/rx.c | 7 ++- 3 files changed, 9 insertions(+), 4 deletions(-)
Re: [PATCH] cfg80211: reg: Fix use-after-free in call_crda
Hi, > In function reg_query_database, query_regdb_file call > request_firmware_nowait to do request_firmware asynchronously, > which need the caller hold the reference of dev, otherwise it will > do put_device freeing '®_pdev->dev'. After that, call_crda access > the dev will trigger use-after-free bug. So ... OK, but how does that then only fix the firmware file loading, rather than CRDA calling? > This patch fix this by holding a reference of dev in regulatory_init > after platform_device_register_simple registered successly, which > releasing in platform_device_unregister. This doesn't make sense? You just add a new reference and don't release it? If there was a bug then just loading & unloading would trigger an underflow now? platform_device_register_full() (to which _simple is a wrapper) will evidently return the pdev with a reference held, because it does platform_device_put() in the error path? johannes
Re: bug report: iwlwifi: mvm: support mac80211 TXQs model
On Wed, 2019-02-20 at 14:40 +, Colin Ian King wrote:
>
> ..when the used_hw_queues initialization was removed:
>
> @@ -360,8 +300,6 @@ int iwl_mvm_mac_ctxt_init(struct iwl_mvm *mvm,
> struct ieee80211_vif *vif)
> mvm->hw, IEEE80211_IFACE_ITER_RESUME_ALL,
> iwl_mvm_mac_iface_iterator, &data);
>
> - used_hw_queues = iwl_mvm_get_used_hw_queues(mvm, vif);
> -
> /*
> * In the case we're getting here during resume, it's similar to
> * firmware restart, and with RESUME_ALL the iterator will find
>
>
> I'm not 100% sure if the right thing to do is just to now initialize
> used_hw_queues to zero; it's not entirely clear what the initial value
> should be now.
My gut feeling says that we should just remove all of the code - we no
longer use the vif->hw_queue stuff. Which also explains why we didn't
notice any problems from this :)
Something like
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
index aa308f7e7989..f90383943c62 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
@@ -263,8 +263,7 @@ int iwl_mvm_mac_ctxt_init(struct iwl_mvm *mvm, struct
ieee80211_vif *vif)
.found_vif = false,
};
u32 ac;
- int ret, i, queue_limit;
- unsigned long used_hw_queues;
+ int ret, i;
lockdep_assert_held(&mvm->mutex);
@@ -341,38 +340,9 @@ int iwl_mvm_mac_ctxt_init(struct iwl_mvm *mvm, struct
ieee80211_vif *vif)
INIT_LIST_HEAD(&mvmvif->time_event_data.list);
mvmvif->time_event_data.id = TE_MAX;
- /* No need to allocate data queues to P2P Device MAC and NAN.*/
if (vif->type == NL80211_IFTYPE_P2P_DEVICE ||
- vif->type == NL80211_IFTYPE_NAN) {
- for (ac = 0; ac < IEEE80211_NUM_ACS; ac++)
- vif->hw_queue[ac] = IEEE80211_INVAL_HW_QUEUE;
-
+ vif->type == NL80211_IFTYPE_NAN)
return 0;
- }
-
- /*
-* queues in mac80211 almost entirely independent of
-* the ones here - no real limit
-*/
- queue_limit = IEEE80211_MAX_QUEUES;
-
- /*
-* Find available queues, and allocate them to the ACs. When in
-* DQA-mode they aren't really used, and this is done only so the
-* mac80211 ieee80211_check_queues() function won't fail
-*/
- for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
- u8 queue = find_first_zero_bit(&used_hw_queues, queue_limit);
-
- if (queue >= queue_limit) {
- IWL_ERR(mvm, "Failed to allocate queue\n");
- ret = -EIO;
- goto exit_fail;
- }
-
- __set_bit(queue, &used_hw_queues);
- vif->hw_queue[ac] = queue;
- }
/* Allocate the CAB queue for softAP and GO interfaces */
if (vif->type == NL80211_IFTYPE_AP ||
I'll think about it a bit more and test it out, thanks for the report!
johannes
[PATCH net] mac80211: mesh: fix missing unlock on error in table_path_del()
From: Wei Yongjun
spin_lock_bh() is used in table_path_del() but rcu_read_unlock()
is used for unlocking. Fix it by using spin_unlock_bh() instead
of rcu_read_unlock() in the error handling case.
Fixes: b4c3fbe63601 ("mac80211: Use linked list instead of rhashtable walk for
mesh tables")
Acked-by: Herbert Xu
Signed-off-by: Wei Yongjun
Signed-off-by: Johannes Berg
---
Dave, can you take this directly? I see little value in sending a
pull request for this one patch (but if you prefer can do so.)
---
net/mac80211/mesh_pathtbl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index c3a7396fb955..88a6d5e18ccc 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -627,7 +627,7 @@ static int table_path_del(struct mesh_table *tbl,
spin_lock_bh(&tbl->walk_lock);
mpath = rhashtable_lookup_fast(&tbl->rhead, addr, mesh_rht_params);
if (!mpath) {
- rcu_read_unlock();
+ spin_unlock_bh(&tbl->walk_lock);
return -ENXIO;
}
--
2.17.2
pull-request: mac80211 2019-02-15
Hi Dave, It's clear things are winding down, this is basically just the stuff from Herbert that we've been discussing. I threw in a simple error path fix, mostly because it's simple :-) Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit f9bcc9f3ee4fbbe8f11dfec76745476f5780517e: net: ethernet: freescale: set FEC ethtool regs version (2019-02-14 12:45:35 -0500) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2019-02-15 for you to fetch changes up to 83e37e0bdd1470bbe6612250b745ad39b1a7b130: mac80211: Restore vif beacon interval if start ap fails (2019-02-15 13:30:24 +0100) Just a few fixes this time: * mesh rhashtable fixes from Herbert * a small error path fix when starting AP interfaces Herbert Xu (2): mac80211: Use linked list instead of rhashtable walk for mesh tables mac80211: Free mpath object when rhashtable insertion fails Rakesh Pillai (1): mac80211: Restore vif beacon interval if start ap fails net/mac80211/cfg.c | 6 +- net/mac80211/mesh.h | 6 ++ net/mac80211/mesh_pathtbl.c | 155 +--- 3 files changed, 57 insertions(+), 110 deletions(-)
Re: [PATCH AUTOSEL 3.18 15/16] cfg80211: extend range deviation for DMG
On Thu, 2019-02-14 at 21:15 -0500, Sasha Levin wrote:
> From: Chaitanya Tata
>
> [ Upstream commit 93183bdbe73bbdd03e9566c8dc37c9d06b0d0db6 ]
>
> Recently, DMG frequency bands have been extended till 71GHz, so extend
> the range check till 20GHz (45-71GHZ), else some channels will be marked
> as disabled.
There's not really any danger in picking this up for old kernels, but
also practically no value since those kernels wouldn't have supoprt for
the higher ranges ("recently, ...") part :)
johannes
Re: [v3 PATCH 0/4] mac80211: Fix incorrect usage of rhashtable walk API
> The first two patches in this series are bug fixes and should be > backported to stable. > > They fixes a number of issues with the use of the rhashtable API > in mac80211. First of all it converts the use of rashtable walks over > to a simple linked list. This is because an rhashtable walk is > inherently unstable and not meant for uses that require stability, > e.g., when you're trying to lookup an object to delete. Thanks a lot, Herbert. Applied those now, I'll send a pull request to Dave with them. Once that trickles back into net-next I'll apply the third patch (it doesn't apply without the others), and then Dave you can take the rhashtable one. Let me know if you'd prefer I take the rhashtable one through my tree, which really would be only so you don't have to track the dependency. NB: it'd be easier in patchwork if you tagged all the patches with v3 in their own PATCH tag, or put the "v3" tag into the actual subject (not the "[PATCH 0/4]" tag because evidently patchwork drops the tags and doesn't track them for the *series* just each *patch* ... so with what you did nothing is visible in patchwork, even just appending "(v3)" to the subject of the cover letter would've fixed that... johannes
Re: [PATCH 2/4] mac80211: Free mpath object when rhashtable insertion fails
On Wed, 2019-02-13 at 22:39 +0800, Herbert Xu wrote:
> + if (ret != -EEXIST)
> return ERR_PTR(ret);
Surely that should still be "if (ret && ret != -EEXIST)" otherwise you
return for the success case too? or "else if"?
I'd probably say we should combine all those ifs into something like this?
if (ret == 0) {
sdata->u.mesh.mesh_paths_generation++;
return new_mpath;
} else if (ret == -EEXIST) {
kfree(new_mpath);
return mpath;
} else {
kfree(new_mpath);
return NULL;
}
Yes, that's a small change in behaviour as to when the generation
counter is updated, but I'm pretty sure it really only needs updating
when we inserted something, not if we found an old mpath.
johannes
Re: [v2 PATCH 0/4] mac80211: Fix incorrect usage of rhashtable walk API
On Wed, 2019-02-13 at 22:38 +0800, Herbert Xu wrote: > Hi: > > The first two patches in this series are bug fixes and should be > backported to stable. > > They fixes a number of issues with the use of the rhashtable API > in mac80211. First of all it converts the use of rashtable walks over > to a simple linked list. This is because an rhashtable walk is > inherently unstable and not meant for uses that require stability, > e.g., when you're trying to lookup an object to delete. > > It also fixes a potential memory leak when the rhashtable insertion > fails (which can occur due to OOM). Thanks a lot Herbert! That looks simpler than I thought it would be. johannes
Re: linux-next: Tree for Feb 11 (wireless/80211)
Thanks for the heads-up, Randy. We'd seen separate build bot reports on this as well. > ERROR: "__umoddi3" [net/wireless/cfg80211.ko] undefined! > ERROR: "__umoddi3" [net/mac80211/mac80211.ko] undefined! Should be fixed as of yesterday, let me know if it reappears. johannes
Re: [PATCH v2] rhashtable: make walk safe from softirq context
On Tue, 2019-02-12 at 10:43 -0800, David Miller wrote: > Herbert and Johannes, I need some guidance. > > It seems Herbert wants the softirq usage of rhashtables removed, Well, specifically of rhashtable walkers. I can only concede that he's right in that a hashtable walk during softirq (or even with softirqs disabled) was maybe a bad idea. At the same time, it's likely going to be pretty deep surgery in this code, and I'm not sure I can do that right now. Maybe Bob has some thoughts if it can be achieved more easily, but I think it'd require adding a new list to each station that tracks which mesh paths it is the next_hop for, and making sure that's maintained correctly, which feels tricky but maybe it's not (I could be more familiar with mesh ...) Evidently this goes back to commit 60854fd94573f0d3b80b55b40cf0140a0430f3ab Author: Bob Copeland Date: Wed Mar 2 10:09:20 2016 -0500 mac80211: mesh: convert path table to rhashtable which is kinda old. Not sure why this didn't surface before, because the spinlock was introduced *before*, otherwise certainly the mutex would've caused us to not be able to do this code to start with (commit c6ff5268293 - rhashtable: Fix walker list corruption). That commit also just converted an existing hashtable walk to rhashtable, so not sure that counts as having introduced the problem :-) I guess that's not really guidance. If it were my call I'd apply the patch and issue a stern warning to myself to remove this ASAP ;-) But sadly, mesh isn't exactly a priority to most, so not sure when that "P" would be. But I guess we should also ask Bob first: 1) do you think it'd be easy to maintain a separate list or avoid the iteration in some otherway, and make that a small enough patch to be applicable for stable? 2) or do you think maybe the mesh_plink_broken() call could just be lifted into a workqueue instead? johannes
pull-request: mac80211 2019-02-12
Hi Dave, We have few more fixes, mostly one-liners; two are bigger: * the speculation one, only because the function had multiple return points and that had to change, and * the peer measurement locking one, because I had to refactor a function to be able to call it with or without locking (depending on context). Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit f09bef61f1ed72869b231e5cff16e73a06505cfb: Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf (2019-02-05 11:23:23 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2019-02-12 for you to fetch changes up to 6157ca0d6bfe437691b1e98a62e2efe12b6714da: mac80211: Fix Tx aggregation session tear down with ITXQs (2019-02-11 15:50:56 +0100) Just a few fixes: * aggregation session teardown with internal TXQs was continuing to send some frames marked as aggregation, fix from Ilan * IBSS join was missed during firmware restart, should such a thing happen * speculative execution based on the return value of cfg80211_classify8021d() - which is controlled by the sender of the packet - could be problematic in some code using it, prevent it * a few peer measurement fixes Aviya Erenfeld (1): nl80211: Fix FTM per burst maximum value Ilan Peer (1): mac80211: Fix Tx aggregation session tear down with ITXQs Johannes Berg (5): cfg80211: pmsr: fix MAC address setting cfg80211: pmsr: fix abort locking mac80211: call drv_ibss_join() on restart cfg80211: pmsr: record netlink port ID cfg80211: prevent speculation on cfg80211_classify8021d() return net/mac80211/agg-tx.c | 4 +++- net/mac80211/util.c| 6 +- net/wireless/core.c| 2 ++ net/wireless/nl80211.c | 2 +- net/wireless/pmsr.c| 26 +- net/wireless/util.c| 35 --- 6 files changed, 52 insertions(+), 23 deletions(-)
Re: [PATCH v2] rhashtable: make walk safe from softirq context
On Fri, 2019-02-08 at 05:48 +0800, Herbert Xu wrote: > On Thu, Feb 07, 2019 at 02:50:54PM +0100, Johannes Berg wrote: > > > > > This interface wasn't designed for use in softirq contexts. > > > > Well, it clearly was used there. You even gave it a gfp_t argument in > > rhashtable_walk_init(), so you can't really claim it wasn't designed for > > this. I see now that it's ignored, but still? > > I see. This was added behind my back so I wasn't aware of it. It's not used and actually I was wrong anyway: this would have also allowed doing the walk while holding a spinlock or with softirqs disabled, rather than from IRQ/softirq context. In any case, it's clearly working to iterate from this context, and doing a spinlock_bh vs. a spinlock in the rhashtable code isn't really that big a deal? Not sure I really understand your objection. johannes
Re: [PATCH v2] rhashtable: make walk safe from softirq context
> This interface wasn't designed for use in softirq contexts. Well, it clearly was used there. You even gave it a gfp_t argument in rhashtable_walk_init(), so you can't really claim it wasn't designed for this. I see now that it's ignored, but still? > Could you please show me who is doing this so I can review that > to see whether it's a legitimate use of this API? I'm sure you'll say it's not legitimate, but it still exists ;-) mesh_plink_broken() gets called from the TX status path, via ieee80211s_update_metric(). johannes
[PATCH v2] rhashtable: make walk safe from softirq context
From: Johannes Berg
When an rhashtable walk is done from softirq context, we rightfully
get a lockdep complaint saying that we could get a softirq in the
middle of a rehash, and thus deadlock on &ht->lock. This happened
e.g. in mac80211 as it does a walk in softirq context.
Fix this by using spin_lock_bh() wherever we use the &ht->lock.
Initially, I thought it would be sufficient to do this only in the
rehash (rhashtable_rehash_table), but I changed my mind:
* the caller doesn't really need to disable softirqs across all
of the rhashtable_walk_* functions, only those parts that they
actually do within the lock need it
* maybe more importantly, it would still lead to massive lockdep
complaints - false positives, but hard to fix - because lockdep
wouldn't know about different ht->lock instances, and thus one
user of the code doing a walk w/o any locking (when it only ever
uses process context this is fine) vs. another user like in wifi
where we noticed this problem would still cause it to complain.
Cc: sta...@vger.kernel.org
Reported-by: Jouni Malinen
Signed-off-by: Johannes Berg
---
lib/rhashtable.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 852ffa5160f1..30d14f8d9985 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -327,10 +327,10 @@ static int rhashtable_rehash_table(struct rhashtable *ht)
/* Publish the new table pointer. */
rcu_assign_pointer(ht->tbl, new_tbl);
- spin_lock(&ht->lock);
+ spin_lock_bh(&ht->lock);
list_for_each_entry(walker, &old_tbl->walkers, list)
walker->tbl = NULL;
- spin_unlock(&ht->lock);
+ spin_unlock_bh(&ht->lock);
/* Wait for readers. All new readers will see the new
* table, and thus no references to the old table will
@@ -670,11 +670,11 @@ void rhashtable_walk_enter(struct rhashtable *ht, struct
rhashtable_iter *iter)
iter->skip = 0;
iter->end_of_table = 0;
- spin_lock(&ht->lock);
+ spin_lock_bh(&ht->lock);
iter->walker.tbl =
rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock));
list_add(&iter->walker.list, &iter->walker.tbl->walkers);
- spin_unlock(&ht->lock);
+ spin_unlock_bh(&ht->lock);
}
EXPORT_SYMBOL_GPL(rhashtable_walk_enter);
@@ -686,10 +686,10 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_enter);
*/
void rhashtable_walk_exit(struct rhashtable_iter *iter)
{
- spin_lock(&iter->ht->lock);
+ spin_lock_bh(&iter->ht->lock);
if (iter->walker.tbl)
list_del(&iter->walker.list);
- spin_unlock(&iter->ht->lock);
+ spin_unlock_bh(&iter->ht->lock);
}
EXPORT_SYMBOL_GPL(rhashtable_walk_exit);
@@ -719,10 +719,10 @@ int rhashtable_walk_start_check(struct rhashtable_iter
*iter)
rcu_read_lock();
- spin_lock(&ht->lock);
+ spin_lock_bh(&ht->lock);
if (iter->walker.tbl)
list_del(&iter->walker.list);
- spin_unlock(&ht->lock);
+ spin_unlock_bh(&ht->lock);
if (iter->end_of_table)
return 0;
@@ -938,12 +938,12 @@ void rhashtable_walk_stop(struct rhashtable_iter *iter)
ht = iter->ht;
- spin_lock(&ht->lock);
+ spin_lock_bh(&ht->lock);
if (tbl->rehash < tbl->size)
list_add(&iter->walker.list, &tbl->walkers);
else
iter->walker.tbl = NULL;
- spin_unlock(&ht->lock);
+ spin_unlock_bh(&ht->lock);
out:
rcu_read_unlock();
--
2.17.2
Re: [PATCH] rhashtable: use irq-safe spinlock in rhashtable_rehash_table()
On Tue, 2019-02-05 at 15:37 +0100, Johannes Berg wrote: > From: Johannes Berg > > When an rhashtabl walk is done from irq/bh context, we rightfully > get a lockdep complaint saying that we could get a (soft-)IRQ in > the middle of a rehash. This happened e.g. in mac80211 as it does > a walk in soft-irq context. > > Fix this by using irq-safe locking here. We don't need _irqsave() > as we know this will be called only in process context from the > workqueue. We could get away with _bh() but that seems a bit less > generic, though I'm not sure anyone would want to do a walk from > a real IRQ handler. Please drop this, it doesn't make sense. I'll resend with all the spinlock usage changed to either _bh or _irqsave(), since it makes no sense to enforce any kind of outside BH/irq disabling for purposes of the inner lock. johannes
[PATCH] rhashtable: use irq-safe spinlock in rhashtable_rehash_table()
From: Johannes Berg When an rhashtabl walk is done from irq/bh context, we rightfully get a lockdep complaint saying that we could get a (soft-)IRQ in the middle of a rehash. This happened e.g. in mac80211 as it does a walk in soft-irq context. Fix this by using irq-safe locking here. We don't need _irqsave() as we know this will be called only in process context from the workqueue. We could get away with _bh() but that seems a bit less generic, though I'm not sure anyone would want to do a walk from a real IRQ handler. Reported-by: Jouni Malinen Signed-off-by: Johannes Berg --- lib/rhashtable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 852ffa5160f1..ad3c1da15475 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -327,10 +327,10 @@ static int rhashtable_rehash_table(struct rhashtable *ht) /* Publish the new table pointer. */ rcu_assign_pointer(ht->tbl, new_tbl); - spin_lock(&ht->lock); + spin_lock_irq(&ht->lock); list_for_each_entry(walker, &old_tbl->walkers, list) walker->tbl = NULL; - spin_unlock(&ht->lock); + spin_unlock_irq(&ht->lock); /* Wait for readers. All new readers will see the new * table, and thus no references to the old table will -- 2.17.2
pull-request: mac80211-next 2019-02-01
Hi Dave,
Here's my next pull request for net-next. I wanted to include more,
notably the multi-BSSID work that's going on now (multi-BSSID lets
a single AP with a single beacon have multiple networks, rather than
requiring a beacon for each), but we're still finding small issues
and bugs in it, so I haven't included it. As always, I'll send it
when it's ready :-)
As noted below, this contains the NLA_POLICY_NESTED{,_ARRAY} changes
I showed you earlier, no new users have been added to net-next as
of today when I merged your tree to mine (to get the net changes to
not have conflicts over this in wireless code.)
Please pull and let me know if there's any problem.
Thanks,
johannes
The following changes since commit fa6821cbf1d9724284ef0906c9a01a5fbf13a35c:
r8169: improve WoL handling (2019-01-31 12:50:42 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git
tags/mac80211-next-for-davem-2019-02-01
for you to fetch changes up to 7d4194633b29342d93501b53accebf638da134ad:
mac80211: fix missing/malformed documentation (2019-02-01 12:11:13 +0100)
New features for the wifi stack:
* airtime fairness scheduling in mac80211, so we can share
* more authentication offloads to userspace - this is for
SAE which is part of WPA3 and is hard to do in firmware
* documentation fixes
* various mesh improvements
* various other small improvements/cleanups
This also contains the NLA_POLICY_NESTED{,_ARRAY} change we
discussed, which affects everyone but there's no other user
yet.
Felix Fietkau (1):
mac80211: minstrel_ht: add flag to indicate missing/inaccurate tx A-MPDU
length
Gustavo A. R. Silva (1):
cfg80211: mark expected switch fall-throughs
Johannes Berg (5):
mac80211: remove unused variable
Merge remote-tracking branch 'net-next/master' into mac80211-next
netlink: reduce NLA_POLICY_NESTED{,_ARRAY} arguments
cfg80211: add missing documentation that kernel-doc warns about
mac80211: fix missing/malformed documentation
Julan Hsu (4):
nl80211/mac80211: mesh: add hop count to mpath info
nl80211/mac80211: mesh: add mesh path change count to mpath info
mac80211: mesh: use average bitrate for link metric calculation
mac80211: mesh: only switch path when new metric is at least 10% better
Liangwei Dong (1):
nl80211: Allow set/del pmksa operations for AP
Matteo Croce (1):
cfg80211: fix typo
Sergey Matyukevich (1):
mac80211: allow overriding HT STBC capabilities
Srinivas Dasari (1):
cfg80211: Authentication offload to user space in AP mode
Sriram R (1):
cfg80211: Notify all User Hints To self managed wiphys
Toke Høiland-Jørgensen (5):
mac80211: Add TXQ scheduling API
cfg80211: Add airtime statistics and settings
mac80211: Add airtime accounting and scheduling to TXQs
mac80211: Expose ieee80211_schedule_txq() function
mac80211: Fix documentation strings for airtime-related variables
Veerendranath Jakkam (1):
cfg80211: Allow drivers to advertise supported AKM suites
YueHaibing (2):
virt_wifi: remove duplicated include from virt_wifi.c
virt_wifi: remove set but not used variable 'w_priv'
Documentation/driver-api/80211/mac80211.rst | 3 +
drivers/net/wireless/virt_wifi.c| 7 --
include/net/cfg80211.h | 55 -
include/net/mac80211.h | 183 ++--
include/net/netlink.h | 8 +-
include/uapi/linux/nl80211.h| 44 ++-
net/mac80211/agg-tx.c | 2 +-
net/mac80211/cfg.c | 14 ++-
net/mac80211/debugfs.c | 4 +
net/mac80211/debugfs_sta.c | 68 ++-
net/mac80211/driver-ops.h | 7 ++
net/mac80211/ht.c | 8 ++
net/mac80211/ieee80211_i.h | 11 ++
net/mac80211/main.c | 11 ++
net/mac80211/mesh.h | 2 +
net/mac80211/mesh_hwmp.c| 29 -
net/mac80211/rc80211_minstrel_ht.c | 25 +++-
net/mac80211/rc80211_minstrel_ht_debugfs.c | 7 +-
net/mac80211/sta_info.c | 46 ++-
net/mac80211/sta_info.h | 38 ++
net/mac80211/status.c | 6 +
net/mac80211/tx.c | 154 ++-
net/wireless/nl80211.c | 86 ++---
net/wireless/reg.c | 4 +-
net/wireless/wext-compat.c | 2 +
25 files changed, 749 insertions(+), 75 deletions(-)
pull-request: mac80211 2019-02-01
Hi Dave, Two more fixes for the current cycle, one is a new bug introduced in 4.20, the other one seems to be much older. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit 5e66e35aab335b83d9ffb220d8a3a13986a7a60e: bnxt_en: Disable interrupts when allocating CP rings or NQs. (2019-01-31 12:55:28 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2019-02-01 for you to fetch changes up to e005bd7ddea06784c1eb91ac5bb6b171a94f3b05: cfg80211: call disconnect_wk when AP stops (2019-02-01 11:12:50 +0100) Two more fixes: * sometimes, not enough tailroom was allocated for software-encrypted management frames in mac80211 * cfg80211 regulatory restore got an additional condition, needs to rerun the checks after that condition changes Felix Fietkau (1): mac80211: ensure that mgmt tx skbs have tailroom for encryption Johannes Berg (1): cfg80211: call disconnect_wk when AP stops net/mac80211/tx.c | 12 +--- net/wireless/ap.c | 2 ++ net/wireless/core.h | 2 ++ net/wireless/sme.c | 2 +- 4 files changed, 14 insertions(+), 4 deletions(-)
Re: [PATCH net 0/4] various compat ioctl fixes
On Mon, 2019-01-28 at 11:22 -0800, David Miller wrote: > I see some back and forth between you and Al, where do we stand at > this point? I don't really know. I think neither of us _likes_ this code, in particular the whole copy_in_user() thing is quite a mess. The copy_in_user() also means that decnet (and similar things, if they exist, I didn't see any but didn't audit all protocols carefully) have no way of working in compat - it's not even clear to me if that'd return -EFAULT or just do something really stupid, and maybe even dangerous? (Dangerous because at least on x86, compat_alloc_user_space() uses stack space, and if we alloc 40 bytes but decnet writes up to 42 (?) then we could overwrite some stack by that? Maybe the 16-byte alignment in compat_alloc_user_space() saves us, but it's all very fragile. Even with the previous patch fixed, decnet's idea of "struct ifreq" is bigger than "struct ifreq" actually is because sockaddr_dn is bigger, if I'm counting it right then that's 42 in total) At the same time, fixing all this _completely_ is not very realistic, it would require passing the ifreq size through to lots of places and making the user copy there take the size rather than sizeof(ifreq), obviously the very least to the method decnet uses, i.e. sock->ioctl() I think, but clearly that affects every other protocol too. This was what my previous patch had done partially for the directly handled ioctls (the revert of which is the first patch in this series). > From what I can see this looks like probably the simplest way to > fix this in net and -stable currently. I tend to agree, at least to fix the regression. We can still deliberate separately if we want to fix decnet for compat or if nobody cares now. But perhaps better decnet broken (quite obviously and detectably) like it basically always was, than IP broken (subtly, if your struct ends up landing at the end of a page). Al, care to speak up about this here? johannes
[PATCH] decnet: fix DN_IFREQ_SIZE
From: Johannes Berg
Digging through the ioctls with Al because of the previous
patches, we found that on 64-bit decnet's dn_dev_ioctl()
is wrong, because struct ifreq::ifr_ifru is actually 24
bytes (not 16 as expected from struct sockaddr) due to the
ifru_map and ifru_settings members.
Clearly, decnet expects the ioctl to be called with a struct
like
struct ifreq_dn {
char ifr_name[IFNAMSIZ];
struct sockaddr_dn ifr_addr;
};
since it does
struct ifreq *ifr = ...;
struct sockaddr_dn *sdn = (struct sockaddr_dn *)&ifr->ifr_addr;
This means that DN_IFREQ_SIZE is too big for what it wants on
64-bit, as it is
sizeof(struct ifreq) - sizeof(struct sockaddr) +
sizeof(struct sockaddr_dn)
This assumes that sizeof(struct sockaddr) is the size of ifr_ifru
but that isn't true.
Fix this to use offsetof(struct ifreq, ifr_ifru).
This indeed doesn't really matter much - the result is that we
copy in/out 8 bytes more than we should on 64-bit platforms. In
case the "struct ifreq_dn" lands just on the end of a page though
it might lead to faults.
As far as I can tell, it has been like this forever, so it seems
very likely that nobody cares.
Signed-off-by: Johannes Berg
---
net/decnet/dn_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/decnet/dn_dev.c b/net/decnet/dn_dev.c
index d0b3e69c6b39..0962f9201baa 100644
--- a/net/decnet/dn_dev.c
+++ b/net/decnet/dn_dev.c
@@ -56,7 +56,7 @@
#include
#include
-#define DN_IFREQ_SIZE (sizeof(struct ifreq) - sizeof(struct sockaddr) +
sizeof(struct sockaddr_dn))
+#define DN_IFREQ_SIZE (offsetof(struct ifreq, ifr_ifru) + sizeof(struct
sockaddr_dn))
static char dn_rt_all_end_mcast[ETH_ALEN] = {0xAB,0x00,0x00,0x04,0x00,0x00};
static char dn_rt_all_rt_mcast[ETH_ALEN] = {0xAB,0x00,0x00,0x03,0x00,0x00};
--
2.17.2
Re: [PATCH net 2/4] Revert "kill dev_ifsioc()"
On Sat, 2019-01-26 at 18:49 +0100, Johannes Berg wrote:
> On Sat, 2019-01-26 at 18:45 +0100, Johannes Berg wrote:
> >
> > Yes and no. It *sometimes* (actually rarely, since we don't really have
> > dev_ioctls that much, afaict) hits this, but it could also just hit
>
> Actually, no, I'm wrong. We do mostly hit dev_ioctl(), since that's the
> common case for things like SIOCGIFNAME.
>
> However, e.g. for SIOCGIFADDR we do go into
>
> > static long sock_do_ioctl(struct net *net, struct socket *sock,
> > unsigned int cmd, unsigned long arg)
> > {
> > [...]
> > err = sock->ops->ioctl(sock, cmd, arg);
> > [...]
> > if (err != -ENOIOCTLCMD)
> > return err;
>
> This, and like I said, plumbing the whole compat stuff through to the
> sock->ops->ioctl() there doesn't seem like a great idea.
So - discussing this further on IRC I thought we could get away with
making struct ifreq just not include the members that are too big
(ifr_map and ifr_settings), but that's also a non-starter because we
need to copy.
Al also points out that all of these reverts break decnet, because that
does some really messy things in dn_dev_ioctl(). Turns out though that
on 64-bit decnet is already broken anyway, because DN_IFREQ_SIZE is
actually wrong. It should presumably be equivalent to something like
struct ifreq_dn {
charifrn_name[IFNAMSIZ];
struct sockaddr_dn ifru_dnaddr;
};
but in fact *isn't* because
#define DN_IFREQ_SIZE (sizeof(struct ifreq) - sizeof(struct sockaddr) +
sizeof(struct sockaddr_dn))
wouldn't be sizeof(struct ifreq_dn), because in this expression
"sizeof(struct ifreq) - sizeof(struct sockaddr)" isn't the same as
"offsetof(struct ifreq, ifr_ifru)" which would be the right thing.
So with these patches we go back to the original state before Al's
patches, but that does mean:
* decnet doesn't work right on 64-bit (kernel & userland) because it
will attempt to copy a bigger buffer than the user would presumably
be expected to provide a struct ifreq_dn like I defined above to
SIOCGIFADDR, and if this is at the end of a page boundary it faults
* 32-bit userland on 64-bit kernel is completely broken here for decnet
ioctls because we copy too little data (struct ifreq, while struct
ifreq_dn is bigger)
The first of these isn't that hard to fix, just fix the DN_IFREQ_SIZE.
The second one I don't see a good way to fix at all.
johannes
