On Sat, Aug 25, 2018 at 11:15 PM, Al Viro [off-list ref] wrote:

On Sat, Aug 25, 2018 at 10:58:01PM -0700, Kees Cook wrote:

quoted

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 <redacted>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <redacted>
---
This should go through -stable please, but I have left off the "Cc:
stable" as per netdev patch policy. Note that use of struct_size()
will need manual expansion in backports, such as:
      sel_size = sizeof(*s) + sizeof(*s->keys) * s->nkeys;

Saner approach would be sel_size = offsetof(struct tc_u32_sel, keys[s->nkeys])...

Either is fine by me.

quoted

+     sel_size = struct_size(s, keys, s->nkeys);
+     if (nla_len(tb[TCA_U32_SEL]) < sel_size) {
+             err = -EINVAL;
+             goto erridr;
+     }

-     n = kzalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
+     n = kzalloc(offsetof(typeof(*n), sel) + sel_size, GFP_KERNEL);

ITYM
        n = kzalloc(offsetof(struct tc_u_common, sel.keys[s->nkeys]), GFP_KERNEL);

I prefer to reuse sel_size and keep typeof() to keep things tied to
"n" more directly. *shrug*

-Kees

-- 
Kees Cook
Pixel Security