diff mbox series

[v9,1/9] iov_iter: add copy_struct_from_iter()

Message ID 0e7270919b461c4249557b12c7dfce0ad35af300.1617258892.git.osandov@fb.com (mailing list archive)
State New
Headers show
Series fs: interface for directly reading/writing compressed data | expand

Commit Message

Omar Sandoval April 1, 2021, 6:51 a.m. UTC
From: Omar Sandoval <osandov@fb.com>

This is essentially copy_struct_from_user() but for an iov_iter.

Suggested-by: Aleksa Sarai <cyphar@cyphar.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 include/linux/uio.h |  1 +
 lib/iov_iter.c      | 91 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

Comments

Linus Torvalds April 1, 2021, 4:05 p.m. UTC | #1
On Wed, Mar 31, 2021 at 11:51 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> + *
> + * The recommended usage is something like the following:
> + *
> + *     if (usize > PAGE_SIZE)
> + *       return -E2BIG;

Maybe this should be more than a recommendation, and just be inside
copy_struct_from_iter(), because otherwise the "check_zeroed_user()"
call might be quite the timesink for somebody who does something
stupid.

But otherwise this new version (still) looks fine to me.

            Linus
Omar Sandoval April 2, 2021, 7:33 a.m. UTC | #2
On Thu, Apr 01, 2021 at 09:05:22AM -0700, Linus Torvalds wrote:
> On Wed, Mar 31, 2021 at 11:51 PM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > + *
> > + * The recommended usage is something like the following:
> > + *
> > + *     if (usize > PAGE_SIZE)
> > + *       return -E2BIG;
> 
> Maybe this should be more than a recommendation, and just be inside
> copy_struct_from_iter(), because otherwise the "check_zeroed_user()"
> call might be quite the timesink for somebody who does something
> stupid.

I did actually almost send this out with the check in
copy_struct_from_iter(), but decided not to for consistency with
copy_struct_from_user().

openat2() seems to be the only user of copy_struct_from_user() that
doesn't limit to PAGE_SIZE, which is odd given that Aleksa wrote both
openat2() and copy_struct_from_user(). Aleksa, was this intentional?

Thanks,
Omar
Christian Brauner April 2, 2021, 8:04 a.m. UTC | #3
On Fri, Apr 02, 2021 at 12:33:20AM -0700, Omar Sandoval wrote:
> On Thu, Apr 01, 2021 at 09:05:22AM -0700, Linus Torvalds wrote:
> > On Wed, Mar 31, 2021 at 11:51 PM Omar Sandoval <osandov@osandov.com> wrote:
> > >
> > > + *
> > > + * The recommended usage is something like the following:
> > > + *
> > > + *     if (usize > PAGE_SIZE)
> > > + *       return -E2BIG;
> > 
> > Maybe this should be more than a recommendation, and just be inside
> > copy_struct_from_iter(), because otherwise the "check_zeroed_user()"
> > call might be quite the timesink for somebody who does something
> > stupid.
> 
> I did actually almost send this out with the check in
> copy_struct_from_iter(), but decided not to for consistency with
> copy_struct_from_user().
> 
> openat2() seems to be the only user of copy_struct_from_user() that
> doesn't limit to PAGE_SIZE, which is odd given that Aleksa wrote both

Al said there's nothing wrong with copying large chunks of memory so we
shouldn't limit the helper but instead limit the callers which have
expectations about their size limit:
https://lore.kernel.org/lkml/20190905182801.GR1131@ZenIV.linux.org.uk/

Christian
Omar Sandoval April 2, 2021, 5:50 p.m. UTC | #4
On Fri, Apr 02, 2021 at 10:04:23AM +0200, Christian Brauner wrote:
> On Fri, Apr 02, 2021 at 12:33:20AM -0700, Omar Sandoval wrote:
> > On Thu, Apr 01, 2021 at 09:05:22AM -0700, Linus Torvalds wrote:
> > > On Wed, Mar 31, 2021 at 11:51 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > >
> > > > + *
> > > > + * The recommended usage is something like the following:
> > > > + *
> > > > + *     if (usize > PAGE_SIZE)
> > > > + *       return -E2BIG;
> > > 
> > > Maybe this should be more than a recommendation, and just be inside
> > > copy_struct_from_iter(), because otherwise the "check_zeroed_user()"
> > > call might be quite the timesink for somebody who does something
> > > stupid.
> > 
> > I did actually almost send this out with the check in
> > copy_struct_from_iter(), but decided not to for consistency with
> > copy_struct_from_user().
> > 
> > openat2() seems to be the only user of copy_struct_from_user() that
> > doesn't limit to PAGE_SIZE, which is odd given that Aleksa wrote both
> 
> Al said there's nothing wrong with copying large chunks of memory so we
> shouldn't limit the helper but instead limit the callers which have
> expectations about their size limit:
> https://lore.kernel.org/lkml/20190905182801.GR1131@ZenIV.linux.org.uk/

Thanks for the context. So I guess it makes sense to keep the check
"recommended" for both functions.
diff mbox series

Patch

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 27ff8eb786dc..cc94223d16d9 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -121,6 +121,7 @@  size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i);
 size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i);
+int copy_struct_from_iter(void *dst, size_t ksize, struct iov_iter *i);
 
 size_t _copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i);
 size_t _copy_from_iter(void *addr, size_t bytes, struct iov_iter *i);
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index f66c62aa7154..9642f651e27a 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -934,6 +934,97 @@  size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
 }
 EXPORT_SYMBOL(copy_page_from_iter);
 
+/**
+ * copy_struct_from_iter - copy a struct from an iov_iter
+ * @dst: Destination buffer.
+ * @ksize: Size of @dst struct.
+ * @i: Source iterator.
+ *
+ * Copies a struct from an iov_iter in a way that guarantees
+ * backwards-compatibility for struct arguments in an iovec (as long as the
+ * rules for copy_struct_from_user() are followed).
+ *
+ * The source struct is assumed to be stored in the current segment of the
+ * iov_iter, and its size is the size of the current segment. The iov_iter must
+ * be positioned at the beginning of the current segment.
+ *
+ * The recommended usage is something like the following:
+ *
+ *   int do_foo(struct iov_iter *i)
+ *   {
+ *     size_t usize = iov_iter_single_seg_count(i);
+ *     struct foo karg;
+ *     int err;
+ *
+ *     if (usize > PAGE_SIZE)
+ *       return -E2BIG;
+ *     if (usize < FOO_SIZE_VER0)
+ *       return -EINVAL;
+ *     err = copy_struct_from_iter(&karg, sizeof(karg), i);
+ *     if (err)
+ *       return err;
+ *
+ *     // ...
+ *   }
+ *
+ * Returns 0 on success or one of the following errors:
+ *  * -E2BIG:  (size of current segment > @ksize) and there are non-zero
+ *             trailing bytes in the current segment.
+ *  * -EFAULT: access to userspace failed.
+ *  * -EINVAL: the iterator is not at the beginning of the current segment.
+ *
+ * On success, the iterator is advanced to the next segment. On error, the
+ * iterator is not advanced.
+ */
+int copy_struct_from_iter(void *dst, size_t ksize, struct iov_iter *i)
+{
+	size_t usize;
+	int ret;
+
+	if (i->iov_offset != 0)
+		return -EINVAL;
+	if (iter_is_iovec(i)) {
+		usize = i->iov->iov_len;
+		might_fault();
+		if (copyin(dst, i->iov->iov_base, min(ksize, usize)))
+			return -EFAULT;
+		if (usize > ksize) {
+			ret = check_zeroed_user(i->iov->iov_base + ksize,
+						usize - ksize);
+			if (ret < 0)
+				return ret;
+			else if (ret == 0)
+				return -E2BIG;
+		}
+	} else if (iov_iter_is_kvec(i)) {
+		usize = i->kvec->iov_len;
+		memcpy(dst, i->kvec->iov_base, min(ksize, usize));
+		if (usize > ksize &&
+		    memchr_inv(i->kvec->iov_base + ksize, 0, usize - ksize))
+			return -E2BIG;
+	} else if (iov_iter_is_bvec(i)) {
+		char *p;
+
+		usize = i->bvec->bv_len;
+		p = kmap_local_page(i->bvec->bv_page);
+		memcpy(dst, p + i->bvec->bv_offset, min(ksize, usize));
+		if (usize > ksize &&
+		    memchr_inv(p + i->bvec->bv_offset + ksize, 0,
+			       usize - ksize)) {
+			kunmap_local(p);
+			return -E2BIG;
+		}
+		kunmap_local(p);
+	} else {
+		return -EFAULT;
+	}
+	if (usize < ksize)
+		memset(dst + usize, 0, ksize - usize);
+	iov_iter_advance(i, usize);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(copy_struct_from_iter);
+
 static size_t pipe_zero(size_t bytes, struct iov_iter *i)
 {
 	struct pipe_inode_info *pipe = i->pipe;