From 568b742a9d9888aca876b6ad9fa45490f18bee0a Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 17 Sep 2018 11:57:28 +0200 Subject: netlink: add NLA_REJECT policy type In some situations some netlink attributes may be used for output only (kernel->userspace) or may be reserved for future use. It's then helpful to be able to prevent userspace from using them in messages sent to the kernel, since they'd otherwise be ignored and any future will become impossible if this happens. Add NLA_REJECT to the policy which does nothing but reject (with EINVAL) validation of any messages containing this attribute. Allow for returning a specific extended ACK error message in the validation_data pointer. While at it clear up the documentation a bit - the NLA_BITFIELD32 documentation was added to the list of len field descriptions. Also, use NL_SET_BAD_ATTR() in one place where it's open-coded. The specific case I have in mind now is a shared nested attribute containing request/response data, and it would be pointless and potentially confusing to have userspace include response data in the messages that actually contain a request. Signed-off-by: Johannes Berg Reviewed-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- lib/nlattr.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) (limited to 'lib') diff --git a/lib/nlattr.c b/lib/nlattr.c index e335bcafa9e4..36d74b079151 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla, } static int validate_nla(const struct nlattr *nla, int maxtype, - const struct nla_policy *policy) + const struct nla_policy *policy, + const char **error_msg) { const struct nla_policy *pt; int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla); @@ -87,6 +88,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype, } switch (pt->type) { + case NLA_REJECT: + if (pt->validation_data && error_msg) + *error_msg = pt->validation_data; + return -EINVAL; + case NLA_FLAG: if (attrlen > 0) return -ERANGE; @@ -180,11 +186,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype, int rem; nla_for_each_attr(nla, head, len, rem) { - int err = validate_nla(nla, maxtype, policy); + int err = validate_nla(nla, maxtype, policy, NULL); if (err < 0) { - if (extack) - extack->bad_attr = nla; + NL_SET_BAD_ATTR(extack, nla); return err; } } @@ -250,11 +255,15 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, u16 type = nla_type(nla); if (type > 0 && type <= maxtype) { + static const char _msg[] = "Attribute failed policy validation"; + const char *msg = _msg; + if (policy) { - err = validate_nla(nla, maxtype, policy); + err = validate_nla(nla, maxtype, policy, &msg); if (err < 0) { - NL_SET_ERR_MSG_ATTR(extack, nla, - "Attribute failed policy validation"); + NL_SET_BAD_ATTR(extack, nla); + if (extack) + extack->_msg = msg; goto errout; } } -- cgit v1.2.3 From b60b87fc2996240e298529a46e122ef62ef9c27f Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 17 Sep 2018 11:57:29 +0200 Subject: netlink: add ethernet address policy types Commonly, ethernet addresses are just using a policy of { .len = ETH_ALEN } which leaves userspace free to send more data than it should, which may hide bugs. Introduce NLA_EXACT_LEN which checks for exact size, rejecting the attribute if it's not exactly that length. Also add NLA_EXACT_LEN_WARN which requires the minimum length and will warn on longer attributes, for backward compatibility. Use these to define NLA_POLICY_ETH_ADDR (new strict policy) and NLA_POLICY_ETH_ADDR_COMPAT (compatible policy with warning); these are used like this: static const struct nla_policy [...] = { [NL_ATTR_NAME] = NLA_POLICY_ETH_ADDR, ... }; Signed-off-by: Johannes Berg Reviewed-by: Marcelo Ricardo Leitner Signed-off-by: David S. Miller --- include/net/netlink.h | 13 +++++++++++++ lib/nlattr.c | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index b318b0a9f6c3..318b1ded3833 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -181,6 +181,8 @@ enum { NLA_S64, NLA_BITFIELD32, NLA_REJECT, + NLA_EXACT_LEN, + NLA_EXACT_LEN_WARN, __NLA_TYPE_MAX, }; @@ -211,6 +213,10 @@ enum { * just like "All other" * NLA_BITFIELD32 Unused * NLA_REJECT Unused + * NLA_EXACT_LEN Attribute 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. * All other Minimum length of attribute payload * * Meaning of `validation_data' field: @@ -236,6 +242,13 @@ struct nla_policy { void *validation_data; }; +#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_ETH_ADDR NLA_POLICY_EXACT_LEN(ETH_ALEN) +#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN) + /** * struct nl_info - netlink source information * @nlh: Netlink message header of original request diff --git a/lib/nlattr.c b/lib/nlattr.c index 36d74b079151..bb6fe5ed4ecf 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -82,12 +82,18 @@ 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]) { + if ((nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) || + (pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) { pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n", current->comm, type); } switch (pt->type) { + case NLA_EXACT_LEN: + if (attrlen != pt->len) + return -ERANGE; + break; + case NLA_REJECT: if (pt->validation_data && error_msg) *error_msg = pt->validation_data; -- cgit v1.2.3 From 100811936f89fd455eda1984810c09003550555b Mon Sep 17 00:00:00 2001 From: Song Liu Date: Thu, 27 Sep 2018 09:34:41 -0700 Subject: bpf: test_bpf: add init_net to dev for flow_dissector Latest changes in __skb_flow_dissect() assume skb->dev has valid nd_net. However, this is not true for test_bpf. As a result, test_bpf.ko crashes the system with the following stack trace: [ 1133.716622] BUG: unable to handle kernel paging request at 0000000000001030 [ 1133.716623] PGD 8000001fbf7ee067 [ 1133.716624] P4D 8000001fbf7ee067 [ 1133.716624] PUD 1f6c1cf067 [ 1133.716625] PMD 0 [ 1133.716628] Oops: 0000 [#1] SMP PTI [ 1133.716630] CPU: 7 PID: 40473 Comm: modprobe Kdump: loaded Not tainted 4.19.0-rc5-00805-gca11cc92ccd2 #1167 [ 1133.716631] Hardware name: Wiwynn Leopard-Orv2/Leopard-DDR BW, BIOS LBM12.5 12/06/2017 [ 1133.716638] RIP: 0010:__skb_flow_dissect+0x83/0x1680 [ 1133.716639] Code: 04 00 00 41 0f b7 44 24 04 48 85 db 4d 8d 14 07 0f 84 01 02 00 00 48 8b 43 10 48 85 c0 0f 84 e5 01 00 00 48 8b 80 a8 04 00 00 <48> 8b 90 30 10 00 00 48 85 d2 0f 84 dd 01 00 00 31 c0 b9 05 00 00 [ 1133.716640] RSP: 0018:ffffc900303c7a80 EFLAGS: 00010282 [ 1133.716642] RAX: 0000000000000000 RBX: ffff881fea0b7400 RCX: 0000000000000000 [ 1133.716643] RDX: ffffc900303c7bb4 RSI: ffffffff8235c3e0 RDI: ffff881fea0b7400 [ 1133.716643] RBP: ffffc900303c7b80 R08: 0000000000000000 R09: 000000000000000e [ 1133.716644] R10: ffffc900303c7bb4 R11: ffff881fb6840400 R12: ffffffff8235c3e0 [ 1133.716645] R13: 0000000000000008 R14: 000000000000001e R15: ffffc900303c7bb4 [ 1133.716646] FS: 00007f54e75d3740(0000) GS:ffff881fff5c0000(0000) knlGS:0000000000000000 [ 1133.716648] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1133.716649] CR2: 0000000000001030 CR3: 0000001f6c226005 CR4: 00000000003606e0 [ 1133.716649] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1133.716650] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 1133.716651] Call Trace: [ 1133.716660] ? sched_clock_cpu+0xc/0xa0 [ 1133.716662] ? sched_clock_cpu+0xc/0xa0 [ 1133.716665] ? log_store+0x1b5/0x260 [ 1133.716667] ? up+0x12/0x60 [ 1133.716669] ? skb_get_poff+0x4b/0xa0 [ 1133.716674] ? __kmalloc_reserve.isra.47+0x2e/0x80 [ 1133.716675] skb_get_poff+0x4b/0xa0 [ 1133.716680] bpf_skb_get_pay_offset+0xa/0x10 [ 1133.716686] ? test_bpf_init+0x578/0x1000 [test_bpf] [ 1133.716690] ? netlink_broadcast_filtered+0x153/0x3d0 [ 1133.716695] ? free_pcppages_bulk+0x324/0x600 [ 1133.716696] ? 0xffffffffa0279000 [ 1133.716699] ? do_one_initcall+0x46/0x1bd [ 1133.716704] ? kmem_cache_alloc_trace+0x144/0x1a0 [ 1133.716709] ? do_init_module+0x5b/0x209 [ 1133.716712] ? load_module+0x2136/0x25d0 [ 1133.716715] ? __do_sys_finit_module+0xba/0xe0 [ 1133.716717] ? __do_sys_finit_module+0xba/0xe0 [ 1133.716719] ? do_syscall_64+0x48/0x100 [ 1133.716724] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9 This patch fixes tes_bpf by using init_net in the dummy dev. Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook") Reported-by: Eric Dumazet Cc: Willem de Bruijn Cc: Petar Penkov Signed-off-by: Song Liu Reviewed-by: Eric Dumazet Acked-by: Willem de Bruijn Signed-off-by: Daniel Borkmann --- lib/test_bpf.c | 1 + 1 file changed, 1 insertion(+) (limited to 'lib') diff --git a/lib/test_bpf.c b/lib/test_bpf.c index 08d3d59dca17..aa22bcaec1dc 100644 --- a/lib/test_bpf.c +++ b/lib/test_bpf.c @@ -6494,6 +6494,7 @@ static struct sk_buff *populate_skb(char *buf, int size) skb->queue_mapping = SKB_QUEUE_MAP; skb->vlan_tci = SKB_VLAN_TCI; skb->vlan_proto = htons(ETH_P_IP); + dev_net_set(&dev, &init_net); skb->dev = &dev; skb->dev->ifindex = SKB_DEV_IFINDEX; skb->dev->type = SKB_DEV_TYPE; -- cgit v1.2.3 From fe3b30ddb90face841b2ede3b73ed2e9cfece6ba Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 26 Sep 2018 11:15:30 +0200 Subject: netlink: remove NLA_NESTED_COMPAT This isn't used anywhere, so we might as well get rid of it. Reviewed-by: David Ahern Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- include/net/netlink.h | 2 -- lib/nlattr.c | 11 ----------- 2 files changed, 13 deletions(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index 318b1ded3833..b680fe365e91 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -172,7 +172,6 @@ enum { NLA_FLAG, NLA_MSECS, NLA_NESTED, - NLA_NESTED_COMPAT, NLA_NUL_STRING, NLA_BINARY, NLA_S8, @@ -203,7 +202,6 @@ enum { * NLA_BINARY Maximum length of attribute payload * NLA_NESTED Don't use `len' field -- length verification is * done by checking len of nested header (or empty) - * NLA_NESTED_COMPAT Minimum length of structure payload * NLA_U8, NLA_U16, * NLA_U32, NLA_U64, * NLA_S8, NLA_S16, diff --git a/lib/nlattr.c b/lib/nlattr.c index bb6fe5ed4ecf..120ad569e13d 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -140,17 +140,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype, 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. -- cgit v1.2.3 From 48fde90a78f8c67e2bec5061f9725fe363519feb Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 26 Sep 2018 11:15:31 +0200 Subject: netlink: make validation_data const The validation data is only used within the policy that should usually already be const, and isn't changed in any code that uses it. Therefore, make the validation_data pointer const. While at it, remove the duplicate variable in the bitfield validation that I'd otherwise have to change to const. Reviewed-by: David Ahern Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- include/net/netlink.h | 2 +- lib/nlattr.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index b680fe365e91..0d698215d4d9 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -237,7 +237,7 @@ enum { struct nla_policy { u16 type; u16 len; - void *validation_data; + const void *validation_data; }; #define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len } diff --git a/lib/nlattr.c b/lib/nlattr.c index 120ad569e13d..e2e5b38394d5 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -45,12 +45,11 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = { }; static int validate_nla_bitfield32(const struct nlattr *nla, - u32 *valid_flags_allowed) + const u32 *valid_flags_mask) { const struct nla_bitfield32 *bf = nla_data(nla); - u32 *valid_flags_mask = valid_flags_allowed; - if (!valid_flags_allowed) + if (!valid_flags_mask) return -EINVAL; /*disallow invalid bit selector */ -- cgit v1.2.3 From c29f1845b2b22974411278bad3a2ac0b7815dfb4 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 26 Sep 2018 11:15:32 +0200 Subject: netlink: move extack setting into validate_nla() This unifies the code between nla_parse() which sets the bad attribute pointer and an error message, and nla_validate() which only sets the bad attribute pointer. It also cleans up the code for NLA_REJECT and paves the way for nested policy validation, as it will allow us to easily skip setting the "generic" message without any extra args like the **error_msg now, just passing the extack through is now enough. While at it, remove the unnecessary label in nla_parse(). Suggested-by: David Ahern Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- lib/nlattr.c | 68 ++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 32 deletions(-) (limited to 'lib') diff --git a/lib/nlattr.c b/lib/nlattr.c index e2e5b38394d5..6e03d650bec4 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -69,10 +69,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla, static int validate_nla(const struct nlattr *nla, int maxtype, const struct nla_policy *policy, - const char **error_msg) + struct netlink_ext_ack *extack) { const struct nla_policy *pt; int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla); + int err = -ERANGE; if (type <= 0 || type > maxtype) return 0; @@ -90,24 +91,31 @@ static int validate_nla(const struct nlattr *nla, int maxtype, switch (pt->type) { case NLA_EXACT_LEN: if (attrlen != pt->len) - return -ERANGE; + goto out_err; break; case NLA_REJECT: - if (pt->validation_data && error_msg) - *error_msg = pt->validation_data; - return -EINVAL; + if (extack && pt->validation_data) { + NL_SET_BAD_ATTR(extack, nla); + extack->_msg = pt->validation_data; + return -EINVAL; + } + err = -EINVAL; + goto out_err; case NLA_FLAG: if (attrlen > 0) - return -ERANGE; + goto out_err; break; case NLA_BITFIELD32: if (attrlen != sizeof(struct nla_bitfield32)) - return -ERANGE; + goto out_err; - return validate_nla_bitfield32(nla, pt->validation_data); + err = validate_nla_bitfield32(nla, pt->validation_data); + if (err) + goto out_err; + break; case NLA_NUL_STRING: if (pt->len) @@ -115,13 +123,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype, else minlen = attrlen; - if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL) - return -EINVAL; + if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL) { + err = -EINVAL; + goto out_err; + } /* fall through */ case NLA_STRING: if (attrlen < 1) - return -ERANGE; + goto out_err; if (pt->len) { char *buf = nla_data(nla); @@ -130,13 +140,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype, attrlen--; if (attrlen > pt->len) - return -ERANGE; + goto out_err; } break; case NLA_BINARY: if (pt->len && attrlen > pt->len) - return -ERANGE; + goto out_err; break; case NLA_NESTED: @@ -152,10 +162,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype, minlen = nla_attr_minlen[pt->type]; if (attrlen < minlen) - return -ERANGE; + goto out_err; } return 0; +out_err: + NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation"); + return err; } /** @@ -180,12 +193,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype, int rem; nla_for_each_attr(nla, head, len, rem) { - int err = validate_nla(nla, maxtype, policy, NULL); + int err = validate_nla(nla, maxtype, policy, extack); - if (err < 0) { - NL_SET_BAD_ATTR(extack, nla); + if (err < 0) return err; - } } return 0; @@ -241,7 +252,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, struct netlink_ext_ack *extack) { const struct nlattr *nla; - int rem, err; + int rem; memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1)); @@ -249,17 +260,12 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, u16 type = nla_type(nla); if (type > 0 && type <= maxtype) { - static const char _msg[] = "Attribute failed policy validation"; - const char *msg = _msg; - if (policy) { - err = validate_nla(nla, maxtype, policy, &msg); - if (err < 0) { - NL_SET_BAD_ATTR(extack, nla); - if (extack) - extack->_msg = msg; - goto errout; - } + int err = validate_nla(nla, maxtype, policy, + extack); + + if (err < 0) + return err; } tb[type] = (struct nlattr *)nla; @@ -270,9 +276,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n", rem, current->comm); - err = 0; -errout: - return err; + return 0; } EXPORT_SYMBOL(nla_parse); -- cgit v1.2.3 From 9a659a35ba177cec30676e170fb6ed98157bcb0d Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 26 Sep 2018 11:15:33 +0200 Subject: netlink: allow NLA_NESTED to specify nested policy to validate Now that we have a validation_data pointer, and the len field in the policy is unused for NLA_NESTED, we can allow using them both to have nested validation. This can be nice in code, although we still have to use nla_parse_nested() or similar which would also take a policy; however, it also serves as documentation in the policy without requiring a look at the code. Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- include/net/netlink.h | 13 +++++++++++-- lib/nlattr.c | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index 0d698215d4d9..91907852da1c 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -200,8 +200,10 @@ enum { * NLA_NUL_STRING Maximum length of string (excluding NUL) * NLA_FLAG Unused * NLA_BINARY Maximum length of attribute payload - * NLA_NESTED Don't use `len' field -- length verification is - * done by checking len of nested header (or empty) + * NLA_NESTED 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 + * number in the nested policy. * NLA_U8, NLA_U16, * NLA_U32, NLA_U64, * NLA_S8, NLA_S16, @@ -224,6 +226,10 @@ enum { * NLA_REJECT This attribute is always rejected and validation data * 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. + * Note that nla_parse() will validate, but of course not + * parse, the nested sub-policies. * All other Unused * * Example: @@ -247,6 +253,9 @@ struct nla_policy { #define NLA_POLICY_ETH_ADDR NLA_POLICY_EXACT_LEN(ETH_ALEN) #define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN) +#define NLA_POLICY_NESTED(maxattr, policy) \ + { .type = NLA_NESTED, .validation_data = policy, .len = maxattr } + /** * struct nl_info - netlink source information * @nlh: Netlink message header of original request diff --git a/lib/nlattr.c b/lib/nlattr.c index 6e03d650bec4..04750f88477c 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -155,6 +155,20 @@ static int validate_nla(const struct nlattr *nla, int maxtype, */ if (attrlen == 0) break; + if (attrlen < NLA_HDRLEN) + goto out_err; + if (pt->validation_data) { + err = nla_validate(nla_data(nla), nla_len(nla), pt->len, + pt->validation_data, extack); + if (err < 0) { + /* + * return directly to preserve the inner + * error message/attribute pointer + */ + return err; + } + } + break; default: if (pt->len) minlen = pt->len; -- cgit v1.2.3 From 1501d13596b92d6d1f0ea5e104be838188b6e026 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 26 Sep 2018 11:15:34 +0200 Subject: netlink: add nested array policy validation Sometimes nested netlink attributes are just used as arrays, with the nla_type() of each not being used; we have this in nl80211 and e.g. NFTA_SET_ELEM_LIST_ELEMENTS. Add the ability to validate this type of message directly in the policy, by adding the type NLA_NESTED_ARRAY which does exactly this: require a first level of nesting but ignore the attribute type, and then inside each require a second level of nested and validate those attributes against a given policy (if present). Note that some nested array types actually require that all of the entries have the same index, this is possible to express in a nested policy already, apart from the validation that only the one allowed type is used. Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- include/net/netlink.h | 12 +++++++++++- lib/nlattr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index 91907852da1c..3698ca8ff92c 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -172,6 +172,7 @@ enum { NLA_FLAG, NLA_MSECS, NLA_NESTED, + NLA_NESTED_ARRAY, NLA_NUL_STRING, NLA_BINARY, NLA_S8, @@ -200,7 +201,8 @@ enum { * NLA_NUL_STRING Maximum length of string (excluding NUL) * NLA_FLAG Unused * NLA_BINARY Maximum length of attribute payload - * NLA_NESTED Length verification is done by checking len of + * 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 * number in the nested policy. @@ -230,6 +232,12 @@ enum { * `len' to the max attribute number. * 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 other Unused * * Example: @@ -255,6 +263,8 @@ struct nla_policy { #define NLA_POLICY_NESTED(maxattr, policy) \ { .type = NLA_NESTED, .validation_data = policy, .len = maxattr } +#define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \ + { .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr } /** * struct nl_info - netlink source information diff --git a/lib/nlattr.c b/lib/nlattr.c index 04750f88477c..2f8feff669a7 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -67,6 +67,34 @@ static int validate_nla_bitfield32(const struct nlattr *nla, return 0; } +static int nla_validate_array(const struct nlattr *head, int len, int maxtype, + const struct nla_policy *policy, + struct netlink_ext_ack *extack) +{ + const struct nlattr *entry; + int rem; + + nla_for_each_attr(entry, head, len, rem) { + int ret; + + if (nla_len(entry) == 0) + continue; + + if (nla_len(entry) < NLA_HDRLEN) { + NL_SET_ERR_MSG_ATTR(extack, entry, + "Array element too short"); + return -ERANGE; + } + + ret = nla_validate(nla_data(entry), nla_len(entry), + maxtype, policy, extack); + if (ret < 0) + return ret; + } + + return 0; +} + static int validate_nla(const struct nlattr *nla, int maxtype, const struct nla_policy *policy, struct netlink_ext_ack *extack) @@ -169,6 +197,29 @@ static int validate_nla(const struct nlattr *nla, int maxtype, } } break; + case NLA_NESTED_ARRAY: + /* a nested array attribute is allowed to be empty; if its not, + * it must have a size of at least NLA_HDRLEN. + */ + if (attrlen == 0) + break; + if (attrlen < NLA_HDRLEN) + goto out_err; + if (pt->validation_data) { + int err; + + err = nla_validate_array(nla_data(nla), nla_len(nla), + pt->len, pt->validation_data, + extack); + if (err < 0) { + /* + * return directly to preserve the inner + * error message/attribute pointer + */ + return err; + } + } + break; default: if (pt->len) minlen = pt->len; -- cgit v1.2.3 From 3e48be05f3c7eb6f6126939e9d957903c5cfeee5 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Thu, 27 Sep 2018 11:28:35 +0200 Subject: netlink: add attribute range validation to policy Without further bloating the policy structs, we can overload the `validation_data' pointer with a struct of s16 min, max and use those to validate ranges in NLA_{U,S}{8,16,32,64} attributes. It may sound strange to validate NLA_U32 with a s16 max, but in many cases NLA_U32 is used for enums etc. since there's no size benefit in using a smaller attribute width anyway, due to netlink attribute alignment; in cases like that it's still useful, particularly when the attribute really transports an enum value. Doing so lets us remove quite a bit of validation code, if we can be sure that these attributes aren't used by userspace in places where they're ignored today. To achieve all this, split the 'type' field and introduce a new 'validation_type' field which indicates what further validation (beyond the validation prescribed by the type of the attribute) is done. This currently allows for no further validation (the default), as well as min, max and range checks. Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- include/net/netlink.h | 67 ++++++++++++++++++++++++++++++++++++++++++++--- lib/nlattr.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index 3698ca8ff92c..d34ceeba82a8 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -188,9 +188,19 @@ enum { #define NLA_TYPE_MAX (__NLA_TYPE_MAX - 1) +enum nla_policy_validation { + NLA_VALIDATE_NONE, + NLA_VALIDATE_RANGE, + NLA_VALIDATE_MIN, + NLA_VALIDATE_MAX, +}; + /** * struct nla_policy - attribute validation policy * @type: Type of attribute or NLA_UNSPEC + * @validation_type: type of attribute validation done in addition to + * type-specific validation (e.g. range), see + * &enum nla_policy_validation * @len: Type specific length of payload * * Policies are defined as arrays of this struct, the array must be @@ -238,7 +248,26 @@ enum { * 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 other Unused + * All other Unused - 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_U8, + * NLA_U16, + * NLA_U32, + * NLA_U64, + * 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 + * 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 the code instead. + * All other Unused - but note that it's a union * * Example: * static const struct nla_policy my_policy[ATTR_MAX+1] = { @@ -249,9 +278,15 @@ enum { * }; */ struct nla_policy { - u16 type; + u8 type; + u8 validation_type; u16 len; - const void *validation_data; + union { + const void *validation_data; + struct { + s16 min, max; + }; + }; }; #define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_EXACT_LEN, .len = _len } @@ -266,6 +301,32 @@ struct nla_policy { #define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \ { .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr } +#define __NLA_ENSURE(condition) (sizeof(char[1 - 2*!(condition)]) - 1) +#define NLA_ENSURE_INT_TYPE(tp) \ + (__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) + +#define NLA_POLICY_RANGE(tp, _min, _max) { \ + .type = NLA_ENSURE_INT_TYPE(tp), \ + .validation_type = NLA_VALIDATE_RANGE, \ + .min = _min, \ + .max = _max \ +} + +#define NLA_POLICY_MIN(tp, _min) { \ + .type = NLA_ENSURE_INT_TYPE(tp), \ + .validation_type = NLA_VALIDATE_MIN, \ + .min = _min, \ +} + +#define NLA_POLICY_MAX(tp, _max) { \ + .type = NLA_ENSURE_INT_TYPE(tp), \ + .validation_type = NLA_VALIDATE_MAX, \ + .max = _max, \ +} + /** * struct nl_info - netlink source information * @nlh: Netlink message header of original request diff --git a/lib/nlattr.c b/lib/nlattr.c index 2f8feff669a7..5670e4b7dfef 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -95,6 +95,64 @@ 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) +{ + bool validate_min, validate_max; + s64 value; + + validate_min = pt->validation_type == NLA_VALIDATE_RANGE || + pt->validation_type == NLA_VALIDATE_MIN; + validate_max = pt->validation_type == NLA_VALIDATE_RANGE || + pt->validation_type == NLA_VALIDATE_MAX; + + switch (pt->type) { + case NLA_U8: + value = nla_get_u8(nla); + break; + case NLA_U16: + value = nla_get_u16(nla); + break; + case NLA_U32: + value = nla_get_u32(nla); + break; + case NLA_S8: + value = nla_get_s8(nla); + break; + case NLA_S16: + value = nla_get_s16(nla); + break; + case NLA_S32: + value = nla_get_s32(nla); + break; + case NLA_S64: + value = nla_get_s64(nla); + break; + case NLA_U64: + /* treat this one specially, since it may not fit into s64 */ + if ((validate_min && nla_get_u64(nla) < pt->min) || + (validate_max && nla_get_u64(nla) > pt->max)) { + NL_SET_ERR_MSG_ATTR(extack, nla, + "integer out of range"); + return -ERANGE; + } + return 0; + default: + WARN_ON(1); + return -EINVAL; + } + + if ((validate_min && value < pt->min) || + (validate_max && value > pt->max)) { + NL_SET_ERR_MSG_ATTR(extack, nla, + "integer out of range"); + return -ERANGE; + } + + return 0; +} + static int validate_nla(const struct nlattr *nla, int maxtype, const struct nla_policy *policy, struct netlink_ext_ack *extack) @@ -230,6 +288,20 @@ static int validate_nla(const struct nlattr *nla, int maxtype, goto out_err; } + /* further validation */ + switch (pt->validation_type) { + case NLA_VALIDATE_NONE: + /* nothing to do */ + break; + case NLA_VALIDATE_RANGE: + case NLA_VALIDATE_MIN: + case NLA_VALIDATE_MAX: + err = nla_validate_int_range(pt, nla, extack); + if (err) + return err; + break; + } + return 0; out_err: NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation"); -- cgit v1.2.3 From 33188bd6430ef06d206ae4fda2cc92f14f16fd20 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Thu, 27 Sep 2018 11:28:36 +0200 Subject: netlink: add validation function to policy Add the ability to have an arbitrary validation function attached to a netlink policy that doesn't already use the validation_data pointer in another way. This can be useful to validate for example the content of a binary attribute, like in nl80211 the "(information) elements", which must be valid streams of "u8 type, u8 length, u8 value[length]". Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- include/net/netlink.h | 24 +++++++++++++++++++++++- lib/nlattr.c | 7 +++++++ 2 files changed, 30 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index d34ceeba82a8..6a106ef5ca56 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -193,13 +193,14 @@ enum nla_policy_validation { NLA_VALIDATE_RANGE, NLA_VALIDATE_MIN, NLA_VALIDATE_MAX, + NLA_VALIDATE_FUNCTION, }; /** * struct nla_policy - attribute validation policy * @type: Type of attribute or NLA_UNSPEC * @validation_type: type of attribute validation done in addition to - * type-specific validation (e.g. range), see + * type-specific validation (e.g. range, function call), see * &enum nla_policy_validation * @len: Type specific length of payload * @@ -269,6 +270,13 @@ enum nla_policy_validation { * of s16 - do that as usual in the code instead. * All other Unused - but note that it's a union * + * Meaning of `validate' field, use via NLA_POLICY_VALIDATE_FN: + * NLA_BINARY Validation function called for the attribute, + * not compatible with use of the validation_data + * as in NLA_BITFIELD32, NLA_REJECT, NLA_NESTED and + * NLA_NESTED_ARRAY. + * All other Unused - but note that it's a union + * * Example: * static const struct nla_policy my_policy[ATTR_MAX+1] = { * [ATTR_FOO] = { .type = NLA_U16 }, @@ -286,6 +294,8 @@ struct nla_policy { struct { s16 min, max; }; + int (*validate)(const struct nlattr *attr, + struct netlink_ext_ack *extack); }; }; @@ -307,6 +317,11 @@ struct nla_policy { tp == NLA_S16 || tp == NLA_U16 || \ tp == NLA_S32 || tp == NLA_U32 || \ tp == NLA_S64 || tp == NLA_U64) + tp) +#define NLA_ENSURE_NO_VALIDATION_PTR(tp) \ + (__NLA_ENSURE(tp != NLA_BITFIELD32 && \ + tp != NLA_REJECT && \ + tp != NLA_NESTED && \ + tp != NLA_NESTED_ARRAY) + tp) #define NLA_POLICY_RANGE(tp, _min, _max) { \ .type = NLA_ENSURE_INT_TYPE(tp), \ @@ -327,6 +342,13 @@ struct nla_policy { .max = _max, \ } +#define NLA_POLICY_VALIDATE_FN(tp, fn, ...) { \ + .type = NLA_ENSURE_NO_VALIDATION_PTR(tp), \ + .validation_type = NLA_VALIDATE_FUNCTION, \ + .validate = fn, \ + .len = __VA_ARGS__ + 0, \ +} + /** * struct nl_info - netlink source information * @nlh: Netlink message header of original request diff --git a/lib/nlattr.c b/lib/nlattr.c index 5670e4b7dfef..1e900bb414ef 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -300,6 +300,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype, if (err) return err; break; + case NLA_VALIDATE_FUNCTION: + if (pt->validate) { + err = pt->validate(nla, extack); + if (err) + return err; + } + break; } return 0; -- cgit v1.2.3 From a5f6cba291654168e6ab73c3e7ff5b27371c4cb9 Mon Sep 17 00:00:00 2001 From: David Ahern Date: Sun, 7 Oct 2018 20:16:25 -0700 Subject: netlink: Add strict version of nlmsg_parse and nla_parse nla_parse is currently lenient on message parsing, allowing type to be 0 or greater than max expected and only logging a message "netlink: %d bytes leftover after parsing attributes in process `%s'." if the netlink message has unknown data at the end after parsing. What this could mean is that the header at the front of the attributes is actually wrong and the parsing is shifted from what is expected. Add a new strict version that actually fails with EINVAL if there are any bytes remaining after the parsing loop completes, if the atttrbitue type is 0 or greater than max expected. Signed-off-by: David Ahern Acked-by: Christian Brauner Signed-off-by: David S. Miller --- include/net/netlink.h | 17 +++++++++++++++++ lib/nlattr.c | 48 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 53 insertions(+), 12 deletions(-) (limited to 'lib') diff --git a/include/net/netlink.h b/include/net/netlink.h index 9522a0bf1f3a..f1db8e594847 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -373,6 +373,9 @@ int nla_validate(const struct nlattr *head, int len, int maxtype, int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, int len, const struct nla_policy *policy, struct netlink_ext_ack *extack); +int nla_parse_strict(struct nlattr **tb, int maxtype, const struct nlattr *head, + int len, const struct nla_policy *policy, + struct netlink_ext_ack *extack); int nla_policy_len(const struct nla_policy *, int); struct nlattr *nla_find(const struct nlattr *head, int len, int attrtype); size_t nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize); @@ -525,6 +528,20 @@ static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen, nlmsg_attrlen(nlh, hdrlen), policy, extack); } +static inline int nlmsg_parse_strict(const struct nlmsghdr *nlh, int hdrlen, + struct nlattr *tb[], int maxtype, + const struct nla_policy *policy, + struct netlink_ext_ack *extack) +{ + if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) { + NL_SET_ERR_MSG(extack, "Invalid header length"); + return -EINVAL; + } + + return nla_parse_strict(tb, maxtype, nlmsg_attrdata(nlh, hdrlen), + nlmsg_attrlen(nlh, hdrlen), policy, extack); +} + /** * nlmsg_find_attr - find a specific attribute in a netlink message * @nlh: netlink message header diff --git a/lib/nlattr.c b/lib/nlattr.c index 1e900bb414ef..d26de6156b97 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -391,9 +391,10 @@ EXPORT_SYMBOL(nla_policy_len); * * Returns 0 on success or a negative error code. */ -int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, - int len, const struct nla_policy *policy, - struct netlink_ext_ack *extack) +static int __nla_parse(struct nlattr **tb, int maxtype, + const struct nlattr *head, int len, + bool strict, const struct nla_policy *policy, + struct netlink_ext_ack *extack) { const struct nlattr *nla; int rem; @@ -403,27 +404,50 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head, nla_for_each_attr(nla, head, len, rem) { u16 type = nla_type(nla); - if (type > 0 && type <= maxtype) { - if (policy) { - int err = validate_nla(nla, maxtype, policy, - extack); - - if (err < 0) - return err; + if (type == 0 || type > maxtype) { + if (strict) { + NL_SET_ERR_MSG(extack, "Unknown attribute type"); + return -EINVAL; } + continue; + } + if (policy) { + int err = validate_nla(nla, maxtype, policy, extack); - tb[type] = (struct nlattr *)nla; + if (err < 0) + return err; } + + tb[type] = (struct nlattr *)nla; } - if (unlikely(rem > 0)) + if (unlikely(rem > 0)) { pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n", rem, current->comm); + NL_SET_ERR_MSG(extack, "bytes leftover after parsing attributes"); + if (strict) + return -EINVAL; + } return 0; } + +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, false, policy, extack); +} EXPORT_SYMBOL(nla_parse); +int nla_parse_strict(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, true, policy, extack); +} +EXPORT_SYMBOL(nla_parse_strict); + /** * nla_find - Find a specific attribute in a stream of attributes * @head: head of attribute stream -- cgit v1.2.3