[PATCH] [iov_iter] use memmove() when copying to/from user page

From: Alexander Potapenko
Date: Tue May 16 2017 - 08:27:47 EST

It's possible that calling sendfile() to copy the data from a memfd to
itself may result in doing a memcpy() with overlapping arguments.
To avoid undefined behavior here, replace memcpy() with memmove() and
rename memcpy_to_page()/memcpy_from_page() accordingly.

This problem has been detected with KMSAN and syzkaller.

Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx>
---
For the record, the following program:
===================================
// autogenerated by syzkaller (http://github.com/google/syzkaller)

#ifndef __NR_memfd_create
#define __NR_memfd_create 319
#endif

#include <sys/mman.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/wait.h>

#include <pthread.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static uint64_t current_time_ms()
{
struct timespec ts;
if (clock_gettime(CLOCK_MONOTONIC, &ts)) _exit(1);
return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000;
}

int fd;
void *page;

void* thr(void* arg)
{
sendfile(fd, fd, page, 0xfec);
return 0;
}

void test()
{
int i;
pthread_t th[2];

page = mmap(0x20000000, 0x1000, 0x3, 0x32, -1, 0);
char buf[1] = "\0";
fd = syscall(__NR_memfd_create, buf, 0);
write(fd, buf, 1);

for (i = 0; i < 2; i++) {
pthread_create(&th[i], 0, thr, NULL);
usleep(10000);
}
usleep(100000);
}

int main()
{
int iter;
for (iter = 0; iter < 10; iter++) {
int pid = fork();
if (pid < 0)
_exit(1);
if (pid == 0) {
test();
_exit(0);
}
int status = 0;
uint64_t start = current_time_ms();
for (;;) {
int res;
if (current_time_ms() - start > 5 * 1000) {
kill(-pid, SIGKILL);
kill(pid, SIGKILL);
while (waitpid(-1, &status, __WALL) != pid) {
}
return 0;
}
usleep(1000);
}
}
return 0;
}
===================================

triggers calls to memcpy() with overlapping arguments:

==================================================================
BUG: memcpy-param-overlap in generic_perform_write+0x551/0xa20
__msan_memcpy(ffff88013c6e3001, ffff88013c6e3000, 105)
CPU: 0 PID: 1040 Comm: probe Not tainted 4.11.0-rc5+ #2562
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x143/0x1b0 lib/dump_stack.c:52
__msan_memcpy+0x91/0x1e0 mm/kmsan/kmsan_instr.c:359
memcpy_from_page lib/iov_iter.c:433
iov_iter_copy_from_user_atomic+0x98e/0x1320 lib/iov_iter.c:721
generic_perform_write+0x551/0xa20 mm/filemap.c:2843
__generic_file_write_iter+0x3fa/0x8f0 mm/filemap.c:2960
generic_file_write_iter+0x705/0xa80 mm/filemap.c:2988
call_write_iter ./include/linux/fs.h:1733
vfs_iter_write+0x481/0x580 fs/read_write.c:391
iter_file_splice_write+0xa87/0x12f0 fs/splice.c:771
do_splice_from fs/splice.c:873
direct_splice_actor+0x1ae/0x210 fs/splice.c:1040
splice_direct_to_actor+0x683/0xd40 fs/splice.c:995
do_splice_direct+0x327/0x430 fs/splice.c:1083
do_sendfile+0x8a3/0x12e0 fs/read_write.c:1379
SYSC_sendfile64+0x1ab/0x2b0 fs/read_write.c:1434
SyS_sendfile64+0x9a/0xc0 fs/read_write.c:1426
do_syscall_64+0x72/0xa0 arch/x86/entry/common.c:285
entry_SYSCALL64_slow_path+0x25/0x25 arch/x86/entry/entry_64.S:246
RIP: 0033:0x43f4da
RSP: 002b:00007f1f1bba4db8 EFLAGS: 00000206 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f4da
RDX: 0000000020000000 RSI: 0000000000000003 RDI: 0000000000000003
RBP: 00007f1f1bba4dd0 R08: 00007f1f1bba5700 R09: 00007f1f1bba5700
R10: 0000000000000fec R11: 0000000000000206 R12: 0000000000000000
R13: 0000000000000000 R14: 00007f1f1bba59c0 R15: 00007f1f1bba5700
==================================================================

---
lib/iov_iter.c | 36 +++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index f835964c9485..097a8820e930 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -427,17 +427,19 @@ void iov_iter_init(struct iov_iter *i, int direction,
}
EXPORT_SYMBOL(iov_iter_init);

-static void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
+static void memmove_from_page(char *to, struct page *page, size_t offset,
+ size_t len)
{
char *from = kmap_atomic(page);
- memcpy(to, from + offset, len);
+ memmove(to, from + offset, len);
kunmap_atomic(from);
}

-static void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
+static void memmove_to_page(struct page *page, size_t offset, const char *from,
+ size_t len)
{
char *to = kmap_atomic(page);
- memcpy(to + offset, from, len);
+ memmove(to + offset, from, len);
kunmap_atomic(to);
}

@@ -525,7 +527,7 @@ static size_t copy_pipe_to_iter(const void *addr, size_t bytes,
return 0;
for ( ; n; idx = next_idx(idx, pipe), off = 0) {
size_t chunk = min_t(size_t, n, PAGE_SIZE - off);
- memcpy_to_page(pipe->bufs[idx].page, off, addr, chunk);
+ memmove_to_page(pipe->bufs[idx].page, off, addr, chunk);
i->idx = idx;
i->iov_offset = off + chunk;
n -= chunk;
@@ -543,8 +545,8 @@ size_t copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
iterate_and_advance(i, bytes, v,
__copy_to_user(v.iov_base, (from += v.iov_len) - v.iov_len,
v.iov_len),
- memcpy_to_page(v.bv_page, v.bv_offset,
- (from += v.bv_len) - v.bv_len, v.bv_len),
+ memmove_to_page(v.bv_page, v.bv_offset,
+ (from += v.bv_len) - v.bv_len, v.bv_len),
memcpy(v.iov_base, (from += v.iov_len) - v.iov_len, v.iov_len)
)

@@ -562,8 +564,8 @@ size_t copy_from_iter(void *addr, size_t bytes, struct iov_iter *i)
iterate_and_advance(i, bytes, v,
__copy_from_user((to += v.iov_len) - v.iov_len, v.iov_base,
v.iov_len),
- memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
- v.bv_offset, v.bv_len),
+ memmove_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
+ v.bv_offset, v.bv_len),
memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
)

@@ -586,8 +588,8 @@ bool copy_from_iter_full(void *addr, size_t bytes, struct iov_iter *i)
v.iov_base, v.iov_len))
return false;
0;}),
- memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
- v.bv_offset, v.bv_len),
+ memmove_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
+ v.bv_offset, v.bv_len),
memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
)

@@ -606,8 +608,8 @@ size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i)
iterate_and_advance(i, bytes, v,
__copy_from_user_inatomic_nocache((to += v.iov_len) - v.iov_len,
v.iov_base, v.iov_len),
- memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
- v.bv_offset, v.bv_len),
+ memmove_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
+ v.bv_offset, v.bv_len),
memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
)

@@ -629,8 +631,8 @@ bool copy_from_iter_full_nocache(void *addr, size_t bytes, struct iov_iter *i)
v.iov_base, v.iov_len))
return false;
0;}),
- memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
- v.bv_offset, v.bv_len),
+ memmove_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
+ v.bv_offset, v.bv_len),
memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
)

@@ -721,8 +723,8 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
iterate_all_kinds(i, bytes, v,
__copy_from_user_inatomic((p += v.iov_len) - v.iov_len,
v.iov_base, v.iov_len),
- memcpy_from_page((p += v.bv_len) - v.bv_len, v.bv_page,
- v.bv_offset, v.bv_len),
+ memmove_from_page((p += v.bv_len) - v.bv_len, v.bv_page,
+ v.bv_offset, v.bv_len),
memcpy((p += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
)
kunmap_atomic(kaddr);
--
2.13.0.303.g4ebf302169-goog