[v4.4.y] net: sched: Fix memory exposure from short TCA_U32_SEL

[v4.4.y] net: sched: Fix memory exposure from short TCA_U32_SEL

[Zubin Mithra]

From: Kees Cook <keescook@chromium.org>

commit 98c8f125fd8a6240ea343c1aa50a1be9047791b8 upstream

Via u32_change(), TCA_U32_SEL has an unspecified type in the netlink
policy, so max length isn't enforced, only minimum. This means nkeys
(from userspace) was being trusted without checking the actual size of
nla_len(), which could lead to a memory over-read, and ultimately an
exposure via a call to u32_dump(). Reachability is CAP_NET_ADMIN within
a namespace.

Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Zubin Mithra <zsm@chromium.org>
---
Notes:
* Syzkaller triggered an OOB read in u32_change with the following
stacktrace:
 [<ffffffff81cb8911>] __dump_stack lib/dump_stack.c:15 [inline]
 [<ffffffff81cb8911>] dump_stack+0xc1/0x120 lib/dump_stack.c:51
 [<ffffffff81525640>] print_trailer+0x151/0x15a mm/slub.c:658
 [<ffffffff81525913>] object_err+0x35/0x3d mm/slub.c:665
 [<ffffffff8152619f>] print_address_description mm/kasan/report.c:188 [inline]
 [<ffffffff8152619f>] kasan_report_error mm/kasan/report.c:285 [inline]
 [<ffffffff8152619f>] kasan_report.part.0.cold+0x21a/0x4c1 mm/kasan/report.c:310
 [<ffffffff814ec0a5>] kasan_report+0x25/0x30 mm/kasan/report.c:297
 [<ffffffff814eb336>] check_memory_region_inline mm/kasan/kasan.c:292 [inline]
 [<ffffffff814eb336>] check_memory_region+0x116/0x190 mm/kasan/kasan.c:299
 [<ffffffff814eb7d4>] memcpy+0x24/0x50 mm/kasan/kasan.c:334
 [<ffffffff823b6d33>] u32_change+0xb03/0x1e60 net/sched/cls_u32.c:844
 [<ffffffff8232f1a0>] tc_ctl_tfilter+0xc20/0x15f0 net/sched/cls_api.c:335
 [<ffffffff822d82e5>] rtnetlink_rcv_msg+0x4f5/0x6d0 net/core/rtnetlink.c:3605
 [<ffffffff823c92ef>] netlink_rcv_skb+0xdf/0x2f0 net/netlink/af_netlink.c:2361
 [<ffffffff822d7de0>] rtnetlink_rcv+0x30/0x40 net/core/rtnetlink.c:3611
 [<ffffffff823c7f02>] netlink_unicast_kernel net/netlink/af_netlink.c:1277 [inline]
 [<ffffffff823c7f02>] netlink_unicast+0x462/0x650 net/netlink/af_netlink.c:1303
 [<ffffffff823c8891>] netlink_sendmsg+0x7a1/0xc50 net/netlink/af_netlink.c:1859
 [<ffffffff8223b165>] sock_sendmsg_nosec net/socket.c:627 [inline]
 [<ffffffff8223b165>] sock_sendmsg+0xd5/0x110 net/socket.c:637
 [<ffffffff8223ebe7>] ___sys_sendmsg+0x767/0x890 net/socket.c:1964
 [<ffffffff8224175b>] __sys_sendmsg+0xbb/0x150 net/socket.c:1998
 [<ffffffff82241822>] SYSC_sendmsg net/socket.c:2009 [inline]
 [<ffffffff82241822>] SyS_sendmsg+0x32/0x50 net/socket.c:2005
 [<ffffffff82a489e7>] entry_SYSCALL_64_fastpath+0x1e/0xa0

* The commit is present in linux-4.9.y.

* This backport is similar to the commit in linux-4.9.y and addresses
conflicts related to struct_size() not being present.

* Tests run: Chrome OS tryjobs, Syzkaller reproducer

 net/sched/cls_u32.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4fbb67430ce48..4d745a2efd201 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -734,6 +734,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	struct nlattr *opt = tca[TCA_OPTIONS];
 	struct nlattr *tb[TCA_U32_MAX + 1];
 	u32 htid;
+	size_t sel_size;
 	int err;
 #ifdef CONFIG_CLS_U32_PERF
 	size_t size;
@@ -827,8 +828,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		return -EINVAL;
 
 	s = nla_data(tb[TCA_U32_SEL]);
+	sel_size = sizeof(*s) + sizeof(*s->keys) * s->nkeys;
+	if (nla_len(tb[TCA_U32_SEL]) < sel_size)
+		return -EINVAL;
 
-	n = kzalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
+	n = kzalloc(offsetof(typeof(*n), sel) + sel_size, GFP_KERNEL);
 	if (n == NULL)
 		return -ENOBUFS;
 
@@ -841,7 +845,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	}
 #endif
 
-	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
+	memcpy(&n->sel, s, sel_size);
 	RCU_INIT_POINTER(n->ht_up, ht);
 	n->handle = handle;
 	n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
-- 
2.23.0.866.gb869b98d4c-goog

Re: [v4.4.y] net: sched: Fix memory exposure from short TCA_U32_SEL

[Greg KH]

On Fri, Oct 18, 2019 at 12:06:47PM -0700, Zubin Mithra wrote:
> From: Kees Cook <keescook@chromium.org>
> 
> commit 98c8f125fd8a6240ea343c1aa50a1be9047791b8 upstream
> 
> Via u32_change(), TCA_U32_SEL has an unspecified type in the netlink
> policy, so max length isn't enforced, only minimum. This means nkeys
> (from userspace) was being trusted without checking the actual size of
> nla_len(), which could lead to a memory over-read, and ultimately an
> exposure via a call to u32_dump(). Reachability is CAP_NET_ADMIN within
> a namespace.
> 
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Zubin Mithra <zsm@chromium.org>
> ---
> Notes:
> * Syzkaller triggered an OOB read in u32_change with the following

Now queued up, thanks.

greg k-h