public inbox for [email protected]
 help / color / mirror / Atom feed
From: Nick Desaulniers <[email protected]>
To: David Laight <[email protected]>
Cc: Christoph Hellwig <[email protected]>,
	David Hildenbrand <[email protected]>,
	Greg KH <[email protected]>,
	Al Viro <[email protected]>,
	"[email protected]" <[email protected]>,
	Andrew Morton <[email protected]>,
	Jens Axboe <[email protected]>, Arnd Bergmann <[email protected]>,
	David Howells <[email protected]>,
	"[email protected]" 
	<[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" <[email protected]>,
	"[email protected]" 
	<[email protected]>
Subject: Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
Date: Thu, 22 Oct 2020 10:54:06 -0700	[thread overview]
Message-ID: <CAKwvOdnix6YGFhsmT_mY8ORNPTOsN3HwS33Dr0Ykn-pyJ6e-Bw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

On Thu, Oct 22, 2020 at 9:35 AM David Laight <[email protected]> wrote:
>
> From: Christoph Hellwig
> > Sent: 22 October 2020 14:24
> >
> > On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> > > My thinking: if the compiler that calls import_iovec() has garbage in
> > > the upper 32 bit
> > >
> > > a) gcc will zero it out and not rely on it being zero.
> > > b) clang will not zero it out, assuming it is zero.
> > >
> > > But
> > >
> > > a) will zero it out when calling the !inlined variant
> > > b) clang will zero it out when calling the !inlined variant
> > >
> > > When inlining, b) strikes. We access garbage. That would mean that we
> > > have calling code that's not generated by clang/gcc IIUC.
> >
> > Most callchains of import_iovec start with the assembly syscall wrappers.
>
> Wait...
> readv(2) defines:
>         ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
>
> But the syscall is defined as:
>
> SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
>                 unsigned long, vlen)
> {
>         return do_readv(fd, vec, vlen, 0);
> }
>
> I'm guessing that nothing actually masks the high bits that come
> from an application that is compiled with clang?
>
> The vlen is 'unsigned long' through the first few calls.
> So unless there is a non-inlined function than takes vlen
> as 'int' the high garbage bits from userspace are kept.

Yeah, that's likely a bug: https://godbolt.org/z/KfsPKs

>
> Which makes it a bug in the kernel C syscall wrappers.
> They need to explicitly mask the high bits of 32bit
> arguments on arm64 but not x86-64.

Why not x86-64? Wouldn't it be *any* LP64 ISA?

Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue .  Greg, would you mind running this
through the wringer?

>
> What does the ARM EABI say about register parameters?

AAPCS is the ABI for 64b ARM, IIUC, which is the ISA GKH is reporting
the problem against. IIUC, EABI is one of the 32b ABIs.  aarch64 is
LP64 just like x86_64.

--
Thanks,
~Nick Desaulniers

[-- Attachment #2: 0001-fs-fix-up-type-confusion-in-readv-writev.patch --]
[-- Type: application/octet-stream, Size: 4630 bytes --]

From aae26b13ffb9e38bb46b8c85985761b5f196b6f6 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <[email protected]>
Date: Thu, 22 Oct 2020 10:23:47 -0700
Subject: [PATCH] fs: fix up type confusion in readv/writev

The syscall interface doesn't match up with the interface libc is using
or that's defined in the manual pages.

ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
ssize_t writev(int fd, const struct iovec *iov, int iovcnt);

The kernel was defining `iovcnt` as `unsigned long` which is a problem
when userspace understands this to be `int`.

(There's still likely a signedness bug here, but use the proper widths
that import_iovec() expects.)

Signed-off-by: Nick Desaulniers <[email protected]>
---
 fs/read_write.c    | 10 +++++-----
 fs/splice.c        |  2 +-
 include/linux/fs.h |  2 +-
 lib/iov_iter.c     |  4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 19f5c4bf75aa..b858f39a4475 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -890,7 +890,7 @@ ssize_t vfs_iter_write(struct file *file, struct iov_iter *iter, loff_t *ppos,
 EXPORT_SYMBOL(vfs_iter_write);
 
 ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
-		  unsigned long vlen, loff_t *pos, rwf_t flags)
+		  unsigned int vlen, loff_t *pos, rwf_t flags)
 {
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
@@ -907,7 +907,7 @@ ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
 }
 
 static ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
-		   unsigned long vlen, loff_t *pos, rwf_t flags)
+		   unsigned int vlen, loff_t *pos, rwf_t flags)
 {
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
@@ -925,7 +925,7 @@ static ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
 }
 
 static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec,
-			unsigned long vlen, rwf_t flags)
+			unsigned int vlen, rwf_t flags)
 {
 	struct fd f = fdget_pos(fd);
 	ssize_t ret = -EBADF;
@@ -1025,13 +1025,13 @@ static ssize_t do_pwritev(unsigned long fd, const struct iovec __user *vec,
 }
 
 SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen)
+		unsigned int, vlen)
 {
 	return do_readv(fd, vec, vlen, 0);
 }
 
 SYSCALL_DEFINE3(writev, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen)
+		unsigned int, vlen)
 {
 	return do_writev(fd, vec, vlen, 0);
 }
diff --git a/fs/splice.c b/fs/splice.c
index 70cc52af780b..7508eccfa143 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -342,7 +342,7 @@ const struct pipe_buf_operations nosteal_pipe_buf_ops = {
 EXPORT_SYMBOL(nosteal_pipe_buf_ops);
 
 static ssize_t kernel_readv(struct file *file, const struct kvec *vec,
-			    unsigned long vlen, loff_t offset)
+			    unsigned int vlen, loff_t offset)
 {
 	mm_segment_t old_fs;
 	loff_t pos = offset;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c4ae9cafbbba..211bce5e6e60 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1895,7 +1895,7 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
 extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
 extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
 extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
-		unsigned long, loff_t *, rwf_t);
+		unsigned int, loff_t *, rwf_t);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
 extern ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 1635111c5bd2..ded9d9c4eb28 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1734,7 +1734,7 @@ struct iovec *iovec_from_user(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,
+		 unsigned int nr_segs, unsigned int fast_segs, struct iovec **iovp,
 		 struct iov_iter *i, bool compat)
 {
 	ssize_t total_len = 0;
@@ -1803,7 +1803,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 int nr_segs, unsigned int fast_segs,
 		 struct iovec **iovp, struct iov_iter *i)
 {
 	return __import_iovec(type, uvec, nr_segs, fast_segs, iovp, i,
-- 
2.29.0.rc1.297.gfa9743e501-goog


  parent reply	other threads:[~2020-10-22 17:54 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-25  4:51 let import_iovec deal with compat_iovecs as well v4 Christoph Hellwig
2020-09-25  4:51 ` [PATCH 1/9] compat.h: fix a spelling error in <linux/compat.h> Christoph Hellwig
2020-09-25  4:51 ` [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c Christoph Hellwig
2020-10-21 16:13   ` Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c" Greg KH
2020-10-21 20:59     ` David Laight
2020-10-21 23:39     ` Al Viro
2020-10-22  8:26       ` Greg KH
2020-10-22  8:35         ` David Hildenbrand
2020-10-22  8:40           ` David Laight
2020-10-22  8:48             ` David Hildenbrand
2020-10-22  9:01               ` Greg KH
2020-10-22  9:06                 ` David Laight
2020-10-22  9:19                 ` David Hildenbrand
2020-10-22  9:25                   ` David Hildenbrand
2020-10-22  9:32                     ` David Laight
2020-10-22  9:36                       ` David Hildenbrand
2020-10-22 10:48                         ` Greg KH
2020-10-22 12:18                           ` Greg KH
2020-10-22 12:42                             ` David Hildenbrand
2020-10-22 12:57                               ` Greg KH
2020-10-22 13:50                                 ` Greg KH
     [not found]                                   ` <CAK8P3a1B7OVdyzW0-97JwzZiwp0D0fnSfyete16QTvPp_1m07A@mail.gmail.com>
2020-10-22 14:40                                     ` Greg KH
2020-10-22 16:15                                       ` David Laight
2020-10-23 12:46                                   ` David Laight
2020-10-23 13:09                                     ` David Hildenbrand
2020-10-23 14:33                                       ` David Hildenbrand
2020-10-23 14:39                                         ` David Laight
2020-10-23 14:47                                           ` 'Greg KH'
2020-10-23 16:33                                             ` David Hildenbrand
2020-11-02  9:06                                             ` David Laight
2020-11-02 13:52                                               ` 'Greg KH'
2020-11-02 18:23                                                 ` David Laight
2020-10-23 17:58                                       ` Al Viro
2020-10-23 18:27                                         ` Segher Boessenkool
2020-10-23 21:28                                           ` David Laight
2020-10-24 17:29                                             ` Segher Boessenkool
2020-10-24 21:12                                               ` David Laight
     [not found]                                     ` <CAK8P3a1n+b8hOMhNQSDzgic03dyXbmpccfTJ3C1bGKvzsgMXbg@mail.gmail.com>
2020-10-23 13:28                                       ` David Laight
2020-10-22 13:23                         ` Christoph Hellwig
2020-10-22 16:35                           ` David Laight
2020-10-22 16:40                             ` Matthew Wilcox
2020-10-22 16:50                               ` David Laight
2020-10-22 17:00                               ` Nick Desaulniers
2020-10-22 20:59                                 ` Eric Biggers
2020-10-22 21:28                                   ` Al Viro
2020-10-22 18:19                               ` Al Viro
2020-10-22 17:54                             ` Nick Desaulniers [this message]
     [not found]                               ` <CAK8P3a3LjG+ZvmQrkb9zpgov8xBkQQWrkHBPgjfYSqBKGrwT4w@mail.gmail.com>
2020-10-22 19:04                                 ` Nick Desaulniers
2020-10-22 19:24                                   ` Al Viro
2020-10-22 19:27                                     ` Al Viro
2020-10-22 20:06                                     ` Al Viro
2020-10-22 20:09                                       ` Al Viro
2020-10-22 20:11                                     ` Nick Desaulniers
2020-10-22 22:07                                     ` David Laight
2020-10-23 13:12                                     ` David Hildenbrand
2020-10-22 22:04                                   ` David Laight
2020-10-22  9:28                   ` David Laight
2020-10-22  9:02               ` David Laight
2020-09-25  4:51 ` [PATCH 3/9] iov_iter: refactor rw_copy_check_uvector and import_iovec Christoph Hellwig
2020-09-25  4:51 ` [PATCH 4/9] iov_iter: transparently handle compat iovecs in import_iovec Christoph Hellwig
2020-09-25  4:51 ` [PATCH 5/9] fs: remove various compat readv/writev helpers Christoph Hellwig
2020-09-25  4:51 ` [PATCH 6/9] fs: remove the compat readv/writev syscalls Christoph Hellwig
2020-09-25  4:51 ` [PATCH 7/9] fs: remove compat_sys_vmsplice Christoph Hellwig
2020-09-25  4:51 ` [PATCH 8/9] mm: remove compat_process_vm_{readv,writev} Christoph Hellwig
2020-09-25  4:51 ` [PATCH 9/9] security/keys: remove compat_keyctl_instantiate_key_iov Christoph Hellwig
2020-09-25 15:23 ` let import_iovec deal with compat_iovecs as well v4 Al Viro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKwvOdnix6YGFhsmT_mY8ORNPTOsN3HwS33Dr0Ykn-pyJ6e-Bw@mail.gmail.com \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox