* Re: [PATCH net] netlink: Relax attr validation for fixed length types
2017-12-05 19:55 [PATCH net] netlink: Relax attr validation for fixed length types David Ahern
@ 2017-12-05 22:58 ` David Miller
2017-12-06 8:06 ` [net] " Johannes Berg
2017-12-06 19:12 ` [PATCH net] " David Miller
2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2017-12-05 22:58 UTC (permalink / raw)
To: dsahern; +Cc: netdev, johannes
From: David Ahern <dsahern@gmail.com>
Date: Tue, 5 Dec 2017 12:55:40 -0700
> Commit 28033ae4e0f5 ("net: netlink: Update attr validation to require
> exact length for some types") requires attributes using types NLA_U* and
> NLA_S* to have an exact length. This change is exposing bugs in various
> userspace commands that are sending attributes with an invalid length
> (e.g., attribute has type NLA_U8 and userspace sends NLA_U32). While
> the commands are clearly broken and need to be fixed, users are arguing
> that the sudden change in enforcement is breaking older commands on
> newer kernels for use cases that otherwise "worked".
>
> Relax the validation to print a warning mesage similar to what is done
> for messages containing extra bytes after parsing.
>
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Johannes, please review.
> ---
> lib/nlattr.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 8bf78b4b78f0..6122662906c8 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -28,8 +28,16 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
> };
>
> static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
> + [NLA_U8] = sizeof(u8),
> + [NLA_U16] = sizeof(u16),
> + [NLA_U32] = sizeof(u32),
> + [NLA_U64] = sizeof(u64),
> [NLA_MSECS] = sizeof(u64),
> [NLA_NESTED] = NLA_HDRLEN,
> + [NLA_S8] = sizeof(s8),
> + [NLA_S16] = sizeof(s16),
> + [NLA_S32] = sizeof(s32),
> + [NLA_S64] = sizeof(s64),
> };
>
> static int validate_nla_bitfield32(const struct nlattr *nla,
> @@ -70,10 +78,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> BUG_ON(pt->type > NLA_TYPE_MAX);
>
> /* for data types NLA_U* and NLA_S* require exact length */
> - if (nla_attr_len[pt->type]) {
> - if (attrlen != nla_attr_len[pt->type])
> - return -ERANGE;
> - return 0;
> + if (nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) {
> + pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
> + current->comm, type);
> }
>
> switch (pt->type) {
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [net] netlink: Relax attr validation for fixed length types
2017-12-05 19:55 [PATCH net] netlink: Relax attr validation for fixed length types David Ahern
2017-12-05 22:58 ` David Miller
@ 2017-12-06 8:06 ` Johannes Berg
2017-12-06 19:12 ` [PATCH net] " David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Johannes Berg @ 2017-12-06 8:06 UTC (permalink / raw)
To: David Ahern, netdev
On Tue, 2017-12-05 at 12:55 -0700, David Ahern wrote:
> @@ -70,10 +78,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> BUG_ON(pt->type > NLA_TYPE_MAX);
>
> /* for data types NLA_U* and NLA_S* require exact length */
You should update the comment now :-)
And the comment on nla_attr_len as well.
With the comments fixed, this looks fine to me.
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Since we already have two tables now and may want to add attribute
types with exact enforcement in the future (like the BITFIELD32 one),
I'd actually do things a bit more data driven, but I haven't tested it
right now, and it's better done in net-next after this fix.
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..e65eb5400a1a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -15,21 +15,67 @@
#include <linux/types.h>
#include <net/netlink.h>
-/* for these data types attribute length must be exactly given size */
-static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
- [NLA_U8] = sizeof(u8),
- [NLA_U16] = sizeof(u16),
- [NLA_U32] = sizeof(u32),
- [NLA_U64] = sizeof(u64),
- [NLA_S8] = sizeof(s8),
- [NLA_S16] = sizeof(s16),
- [NLA_S32] = sizeof(s32),
- [NLA_S64] = sizeof(s64),
-};
-
-static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
- [NLA_MSECS] = sizeof(u64),
- [NLA_NESTED] = NLA_HDRLEN,
+/* netlink validation flags */
+#define NLVF_WARN_WRONG_LEN BIT(0)
+#define NLVF_REJECT_WRONG_LEN BIT(1)
+
+static const struct {
+ u8 len, flags;
+} nla_attr_val[NLA_TYPE_MAX + 1] = {
+ [NLA_FLAG] = {
+ .len = 0,
+ .flags = NLVF_REJECT_WRONG_LEN,
+ },
+ [NLA_BITFIELD32] = {
+ /* further checks below */
+ .len = sizeof(struct nla_bitfield32),
+ .flags = NLVF_REJECT_WRONG_LEN,
+ },
+ [NLA_U8] = {
+ .len = sizeof(u8),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_U16] = {
+ .len = sizeof(u16),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_U32] = {
+ .len = sizeof(u32),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_U64] = {
+ .len = sizeof(u64),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S8] = {
+ .len = sizeof(s8),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S16] = {
+ .len = sizeof(s16),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S32] = {
+ .len = sizeof(s32),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S64] = {
+ .len = sizeof(s64),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_MSECS] = {
+ .len = sizeof(s64),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_NESTED] = {
+ /* minimum length */
+ .len = NLA_HDRLEN,
+ },
+ [NLA_STRING] = {
+ /* minimum length, further checks below */
+ .len = 1,
+ },
+ /* others have .len = 0 and no flags */
};
static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -60,7 +106,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy)
{
const struct nla_policy *pt;
- int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+ int minlen, attrlen = nla_len(nla), type = nla_type(nla);
if (type <= 0 || type > maxtype)
return 0;
@@ -69,23 +115,51 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
BUG_ON(pt->type > NLA_TYPE_MAX);
- /* for data types NLA_U* and NLA_S* require exact length */
- if (nla_attr_len[pt->type]) {
- if (attrlen != nla_attr_len[pt->type])
+ switch (pt->type) {
+ case NLA_BINARY:
+ if (pt->len && attrlen > pt->len)
return -ERANGE;
- return 0;
- }
+ break;
- switch (pt->type) {
- case NLA_FLAG:
- if (attrlen > 0)
+ case NLA_NESTED_COMPAT:
+ if (attrlen < pt->len)
+ return -ERANGE;
+ if (attrlen < NLA_ALIGN(pt->len))
+ break;
+ if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
+ return -ERANGE;
+ nla = nla_data(nla) + NLA_ALIGN(pt->len);
+ if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
return -ERANGE;
break;
- case NLA_BITFIELD32:
- if (attrlen != sizeof(struct nla_bitfield32))
+ case NLA_NESTED:
+ /* a nested attributes is allowed to be empty; if its not,
+ * it must have a size of at least NLA_HDRLEN.
+ */
+ if (attrlen == 0)
+ break;
+ /* fall through */
+ default:
+ minlen = nla_attr_val[pt->type].len;
+
+ if (nla_attr_val[pt->type].flags & NLVF_REJECT_WRONG_LEN &&
+ minlen != attrlen)
+ return -ERANGE;
+ if (nla_attr_val[pt->type].flags & NLVF_WARN_WRONG_LEN &&
+ minlen != attrlen)
+ pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
+ current->comm, type);
+
+ if (pt->len)
+ minlen = pt->len;
+
+ if (attrlen < minlen)
return -ERANGE;
+ }
+ switch (pt->type) {
+ case NLA_BITFIELD32:
return validate_nla_bitfield32(nla, pt->validation_data);
case NLA_NUL_STRING:
@@ -99,9 +173,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
/* fall through */
case NLA_STRING:
- if (attrlen < 1)
- return -ERANGE;
-
if (pt->len) {
char *buf = nla_data(nla);
@@ -112,37 +183,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
return -ERANGE;
}
break;
-
- case NLA_BINARY:
- if (pt->len && attrlen > pt->len)
- return -ERANGE;
- break;
-
- case NLA_NESTED_COMPAT:
- if (attrlen < pt->len)
- return -ERANGE;
- if (attrlen < NLA_ALIGN(pt->len))
- break;
- if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
- return -ERANGE;
- nla = nla_data(nla) + NLA_ALIGN(pt->len);
- if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
- return -ERANGE;
- break;
- case NLA_NESTED:
- /* a nested attributes is allowed to be empty; if its not,
- * it must have a size of at least NLA_HDRLEN.
- */
- if (attrlen == 0)
- break;
- default:
- if (pt->len)
- minlen = pt->len;
- else if (pt->type != NLA_UNSPEC)
- minlen = nla_attr_minlen[pt->type];
-
- if (attrlen < minlen)
- return -ERANGE;
}
return 0;
johannes
^ permalink raw reply related [flat|nested] 4+ messages in thread