[PATCH] iov_iter: take an unsigned long segment count in import_iovec()

[PATCH] iov_iter: take an unsigned long segment count in import_iovec()

[Andrea Parri]

readv(), writev() and their positional variants, process_vm_readv(),
process_vm_writev(), vmsplice(), process_madvise(), vectored aio and
the btrfs encoded I/O ioctls all take a full-width segment count, and
all of them hand it to import_iovec() or __import_iovec().  Those take
an unsigned int count, so on 64-bit kernels the upper 32 bits are
dropped before iovec_from_user() checks the count against UIO_MAXIOV.

A count of 0x100000001 is therefore imported as one segment: writev()
on a pipe writes a byte and readv() reads it back, instead of both
failing with EINVAL.  A count of 0x100000000 returns 0 as an empty
vector.

import_iovec() was added with an unsigned int count, but the helper it
wrapped, rw_copy_check_uvector(), took an unsigned long, as does
iovec_from_user() today.  Until v4.1, readv(), writev(), preadv(),
pwritev(), process_vm_readv(), process_vm_writev() and vectored aio
therefore failed these counts with EINVAL, as did vmsplice() until
v4.18.

Widen nr_segs in both import helpers so the UIO_MAXIOV check sees the
count the caller passed.  Callers with 32-bit counts are unaffected.

AI assistance was used to find the bug, develop the fix, and run the
reproducers.

Fixes: bc917be81059 ("saner iov_iter initialization primitives")
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
Tested on x86_64 with virtme-ng, base 734bef548a96e vs patched:
readv, writev, preadv, pwritev, preadv2, pwritev2, vectored aio,
process_madvise, process_vm_readv, process_vm_writev and vmsplice with
counts 0, 1, 1024, 1025, UINT_MAX, 2^32, 2^32+1, 2^32+1024, 2^32+1025
and ULONG_MAX (LONG_MAX for aio).  The base kernel accepts 2^32,
2^32+1 and 2^32+1024 on every path; the patched kernel rejects every
count above 1024 with EINVAL.  Also built every caller with
allmodconfig W=1, and lib/iov_iter.o and the readv, vmsplice and
process_vm callers with i386 defconfig W=1.

Reproducer (64-bit userspace):
  cc -Wall -Wextra -O2 repro.c -o repro
  ./repro

// SPDX-License-Identifier: GPL-2.0-only
#define _GNU_SOURCE
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/uio.h>
#include <unistd.h>

int main(void)
{
	char source = 'x', received = '-';
	struct iovec wiov = { .iov_base = &source, .iov_len = 1 };
	struct iovec riov = { .iov_base = &received, .iov_len = 1 };
	const unsigned long count = (1UL << 32) + 1;
	long wret, rret;
	int pipefd[2], werr;

	if (pipe(pipefd)) {
		perror("pipe");
		return EXIT_FAILURE;
	}
	errno = 0;
	wret = syscall(SYS_writev, pipefd[1], &wiov, count);
	werr = errno;
	close(pipefd[1]);
	errno = 0;
	rret = syscall(SYS_readv, pipefd[0], &riov, count);
	printf("writev=%ld (%s) readv=%ld (%s) byte=%c\n",
	       wret, strerror(werr), rret, strerror(errno), received);
	close(pipefd[0]);
	return EXIT_SUCCESS;
}

 include/linux/uio.h | 8 ++++----
 lib/iov_iter.c      | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index fe2e985d74d24..529c6cd53df7a 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -362,11 +362,11 @@ struct iovec *iovec_from_user(const struct iovec __user *uvector,
 		unsigned long nr_segs, unsigned long fast_segs,
 		struct iovec *fast_iov, bool compat);
 ssize_t import_iovec(int type, const struct iovec __user *uvec,
-		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
-		 struct iov_iter *i);
+		 unsigned long nr_segs, unsigned int fast_segs,
+		 struct iovec **iovp, struct iov_iter *i);
 ssize_t __import_iovec(int type, const struct iovec __user *uvec,
-		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
-		 struct iov_iter *i, bool compat);
+		 unsigned long nr_segs, unsigned int fast_segs,
+		 struct iovec **iovp, struct iov_iter *i, bool compat);
 int import_ubuf(int type, void __user *buf, size_t len, struct iov_iter *i);
 
 static inline void iov_iter_ubuf(struct iov_iter *i, unsigned int direction,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 2072c04e99d05..55cf2ff5bfdad 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1362,8 +1362,8 @@ static ssize_t __import_iovec_ubuf(int type, const struct iovec __user *uvec,
 }
 
 ssize_t __import_iovec(int type, const struct iovec __user *uvec,
-		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
-		 struct iov_iter *i, bool compat)
+		 unsigned long nr_segs, unsigned int fast_segs,
+		 struct iovec **iovp, struct iov_iter *i, bool compat)
 {
 	ssize_t total_len = 0;
 	unsigned long seg;
@@ -1434,7 +1434,7 @@ ssize_t __import_iovec(int type, const struct iovec __user *uvec,
  * Return: Negative error code on error, bytes imported on success
  */
 ssize_t import_iovec(int type, const struct iovec __user *uvec,
-		 unsigned nr_segs, unsigned fast_segs,
+		 unsigned long nr_segs, unsigned int fast_segs,
 		 struct iovec **iovp, struct iov_iter *i)
 {
 	return __import_iovec(type, uvec, nr_segs, fast_segs, iovp, i,

base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.53.0

Re: [PATCH] iov_iter: take an unsigned long segment count in import_iovec()

[Jan Kara]

On Mon 26-10-26 19:06:18, Andrea Parri wrote:
> readv(), writev() and their positional variants, process_vm_readv(),
> process_vm_writev(), vmsplice(), process_madvise(), vectored aio and
> the btrfs encoded I/O ioctls all take a full-width segment count, and
> all of them hand it to import_iovec() or __import_iovec().  Those take
> an unsigned int count, so on 64-bit kernels the upper 32 bits are
> dropped before iovec_from_user() checks the count against UIO_MAXIOV.
> 
> A count of 0x100000001 is therefore imported as one segment: writev()
> on a pipe writes a byte and readv() reads it back, instead of both
> failing with EINVAL.  A count of 0x100000000 returns 0 as an empty
> vector.
> 
> import_iovec() was added with an unsigned int count, but the helper it
> wrapped, rw_copy_check_uvector(), took an unsigned long, as does
> iovec_from_user() today.  Until v4.1, readv(), writev(), preadv(),
> pwritev(), process_vm_readv(), process_vm_writev() and vectored aio
> therefore failed these counts with EINVAL, as did vmsplice() until
> v4.18.
> 
> Widen nr_segs in both import helpers so the UIO_MAXIOV check sees the
> count the caller passed.  Callers with 32-bit counts are unaffected.
> 
> AI assistance was used to find the bug, develop the fix, and run the
> reproducers.
> 
> Fixes: bc917be81059 ("saner iov_iter initialization primitives")
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> Tested on x86_64 with virtme-ng, base 734bef548a96e vs patched:
> readv, writev, preadv, pwritev, preadv2, pwritev2, vectored aio,
> process_madvise, process_vm_readv, process_vm_writev and vmsplice with
> counts 0, 1, 1024, 1025, UINT_MAX, 2^32, 2^32+1, 2^32+1024, 2^32+1025
> and ULONG_MAX (LONG_MAX for aio).  The base kernel accepts 2^32,
> 2^32+1 and 2^32+1024 on every path; the patched kernel rejects every
> count above 1024 with EINVAL.  Also built every caller with
> allmodconfig W=1, and lib/iov_iter.o and the readv, vmsplice and
> process_vm callers with i386 defconfig W=1.
> 
> Reproducer (64-bit userspace):
>   cc -Wall -Wextra -O2 repro.c -o repro
>   ./repro
> 
> // SPDX-License-Identifier: GPL-2.0-only
> #define _GNU_SOURCE
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/syscall.h>
> #include <sys/uio.h>
> #include <unistd.h>
> 
> int main(void)
> {
> 	char source = 'x', received = '-';
> 	struct iovec wiov = { .iov_base = &source, .iov_len = 1 };
> 	struct iovec riov = { .iov_base = &received, .iov_len = 1 };
> 	const unsigned long count = (1UL << 32) + 1;
> 	long wret, rret;
> 	int pipefd[2], werr;
> 
> 	if (pipe(pipefd)) {
> 		perror("pipe");
> 		return EXIT_FAILURE;
> 	}
> 	errno = 0;
> 	wret = syscall(SYS_writev, pipefd[1], &wiov, count);
> 	werr = errno;
> 	close(pipefd[1]);
> 	errno = 0;
> 	rret = syscall(SYS_readv, pipefd[0], &riov, count);
> 	printf("writev=%ld (%s) readv=%ld (%s) byte=%c\n",
> 	       wret, strerror(werr), rret, strerror(errno), received);
> 	close(pipefd[0]);
> 	return EXIT_SUCCESS;
> }
> 
>  include/linux/uio.h | 8 ++++----
>  lib/iov_iter.c      | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index fe2e985d74d24..529c6cd53df7a 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -362,11 +362,11 @@ struct iovec *iovec_from_user(const struct iovec __user *uvector,
>  		unsigned long nr_segs, unsigned long fast_segs,
>  		struct iovec *fast_iov, bool compat);
>  ssize_t import_iovec(int type, const struct iovec __user *uvec,
> -		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
> -		 struct iov_iter *i);
> +		 unsigned long nr_segs, unsigned int fast_segs,
> +		 struct iovec **iovp, struct iov_iter *i);
>  ssize_t __import_iovec(int type, const struct iovec __user *uvec,
> -		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
> -		 struct iov_iter *i, bool compat);
> +		 unsigned long nr_segs, unsigned int fast_segs,
> +		 struct iovec **iovp, struct iov_iter *i, bool compat);
>  int import_ubuf(int type, void __user *buf, size_t len, struct iov_iter *i);
>  
>  static inline void iov_iter_ubuf(struct iov_iter *i, unsigned int direction,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 2072c04e99d05..55cf2ff5bfdad 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1362,8 +1362,8 @@ static ssize_t __import_iovec_ubuf(int type, const struct iovec __user *uvec,
>  }
>  
>  ssize_t __import_iovec(int type, const struct iovec __user *uvec,
> -		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
> -		 struct iov_iter *i, bool compat)
> +		 unsigned long nr_segs, unsigned int fast_segs,
> +		 struct iovec **iovp, struct iov_iter *i, bool compat)
>  {
>  	ssize_t total_len = 0;
>  	unsigned long seg;
> @@ -1434,7 +1434,7 @@ ssize_t __import_iovec(int type, const struct iovec __user *uvec,
>   * Return: Negative error code on error, bytes imported on success
>   */
>  ssize_t import_iovec(int type, const struct iovec __user *uvec,
> -		 unsigned nr_segs, unsigned fast_segs,
> +		 unsigned long nr_segs, unsigned int fast_segs,
>  		 struct iovec **iovp, struct iov_iter *i)
>  {
>  	return __import_iovec(type, uvec, nr_segs, fast_segs, iovp, i,
> 
> base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR