| Message ID | 20161203005853.GA117599@beast |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Dec 3, 2016 at 9:58 AM, Kees Cook <keescook@chromium.org> wrote: > - if (len > 0xFFFF) > + if (len > 0xFFFF || len < icmph_len) > return -EMSGSIZE; EMSGSIZE usually means the message is too long. Maybe use EINVAL? That's what the code will return if the passed-in ICMP header is invalid (e.g., is not an echo request).
From: Kees Cook <keescook@chromium.org> Date: Fri, 2 Dec 2016 16:58:53 -0800 > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > index 205e2000d395..8257be3f032c 100644 > --- a/net/ipv4/ping.c > +++ b/net/ipv4/ping.c > @@ -654,7 +654,7 @@ int ping_common_sendmsg(int family, struct msghdr *msg, size_t len, > void *user_icmph, size_t icmph_len) { > u8 type, code; > > - if (len > 0xFFFF) > + if (len > 0xFFFF || len < icmph_len) > return -EMSGSIZE; As suggested by Lorenzo, please use -EINVAL here. Thanks.
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index 205e2000d395..8257be3f032c 100644 --- a/net/ipv4/ping.c +++ b/net/ipv4/ping.c @@ -654,7 +654,7 @@ int ping_common_sendmsg(int family, struct msghdr *msg, size_t len, void *user_icmph, size_t icmph_len) { u8 type, code; - if (len > 0xFFFF) + if (len > 0xFFFF || len < icmph_len) return -EMSGSIZE; /*
Prior to commit c0371da6047a ("put iov_iter into msghdr") in v3.19, there was no check that the iovec contained enough bytes for a icmp header, and the read loop would walk across neighboring stack contents. Since the iov_iter conversion, bad arguments are noticed, but the returned error is EFAULT. Returning EMSGSIZE is a clearer fix and solves the problem prior to v3.19. This was found using trinity with KASAN on v3.18: BUG: KASAN: stack-out-of-bounds in memcpy_fromiovec+0x60/0x114 at addr ffffffc071077da0 Read of size 8 by task trinity-c2/9623 page:ffffffbe034b9a08 count:0 mapcount:0 mapping: (null) index:0x0 flags: 0x0() page dumped because: kasan: bad access detected CPU: 0 PID: 9623 Comm: trinity-c2 Tainted: G BU 3.18.0-dirty #15 Hardware name: Google Tegra210 Smaug Rev 1,3+ (DT) Call trace: [<ffffffc000209c98>] dump_backtrace+0x0/0x1ac arch/arm64/kernel/traps.c:90 [<ffffffc000209e54>] show_stack+0x10/0x1c arch/arm64/kernel/traps.c:171 [< inline >] __dump_stack lib/dump_stack.c:15 [<ffffffc000f18dc4>] dump_stack+0x7c/0xd0 lib/dump_stack.c:50 [< inline >] print_address_description mm/kasan/report.c:147 [< inline >] kasan_report_error mm/kasan/report.c:236 [<ffffffc000373dcc>] kasan_report+0x380/0x4b8 mm/kasan/report.c:259 [< inline >] check_memory_region mm/kasan/kasan.c:264 [<ffffffc00037352c>] __asan_load8+0x20/0x70 mm/kasan/kasan.c:507 [<ffffffc0005b9624>] memcpy_fromiovec+0x5c/0x114 lib/iovec.c:15 [< inline >] memcpy_from_msg include/linux/skbuff.h:2667 [<ffffffc000ddeba0>] ping_common_sendmsg+0x50/0x108 net/ipv4/ping.c:674 [<ffffffc000dded30>] ping_v4_sendmsg+0xd8/0x698 net/ipv4/ping.c:714 [<ffffffc000dc91dc>] inet_sendmsg+0xe0/0x12c net/ipv4/af_inet.c:749 [< inline >] __sock_sendmsg_nosec net/socket.c:624 [< inline >] __sock_sendmsg net/socket.c:632 [<ffffffc000cab61c>] sock_sendmsg+0x124/0x164 net/socket.c:643 [< inline >] SYSC_sendto net/socket.c:1797 [<ffffffc000cad270>] SyS_sendto+0x178/0x1d8 net/socket.c:1761 CVE-2016-8399 Reported-by: Qidan He <i@flanker017.me> Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- net/ipv4/ping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)