netdev - Re: [PATCH net-next] tls: cap the output scatter list to something reasonable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 7 Feb 2022 21:20:04 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, borisp@...dia.com,
        john.fastabend@...il.com, daniel@...earbox.net,
        vfedorenko@...ek.ru, kernel-team@...com, axboe@...nel.dk
Subject: Re: [PATCH net-next] tls: cap the output scatter list to something
 reasonable

On Mon, Feb 07, 2022 at 09:26:19AM -0800, Jakub Kicinski wrote:
> On Mon, 7 Feb 2022 17:15:30 +0000 Al Viro wrote:
> > On Wed, Feb 02, 2022 at 02:20:31PM -0800, Jakub Kicinski wrote:
> > > TLS recvmsg() passes user pages as destination for decrypt.
> > > The decrypt operation is repeated record by record, each
> > > record being 16kB, max. TLS allocates an sg_table and uses
> > > iov_iter_get_pages() to populate it with enough pages to
> > > fit the decrypted record.
> > > 
> > > Even though we decrypt a single message at a time we size
> > > the sg_table based on the entire length of the iovec.
> > > This leads to unnecessarily large allocations, risking
> > > triggering OOM conditions.
> > > 
> > > Use iov_iter_truncate() / iov_iter_reexpand() to construct
> > > a "capped" version of iov_iter_npages(). Alternatively we
> > > could parametrize iov_iter_npages() to take the size as
> > > arg instead of using i->count, or do something else..  
> > 
> > Er...  Would simply passing 16384/PAGE_SIZE instead of MAX_INT work
> > for your purposes?
> 
> The last arg is maxpages, I want maxbytes, no?

What's the point of pass maxpages as argument to that, seeing that
you ignore the value you've got?  I'm just trying to understand what
semantics do you really intend for that thing.

Another thing: looking at that bunch now, for pipe-backed ones
that won't work.  It's a bug, strictly speaking, even though
the actual primitives that grab those pages *will* honour the
truncation/reexpand.

Frankly, I wonder if we would be better off with making iov_iter_npages()
a wrapper for that one, passing SIZE_MAX as maxbytes.  How does the following
(completely untested) look for you?

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 1198a2bfc9bfc..07e4ebae7c6fa 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -236,11 +236,16 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
 			size_t maxsize, unsigned maxpages, size_t *start);
 ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,
 			size_t maxsize, size_t *start);
-int iov_iter_npages(const struct iov_iter *i, int maxpages);
+int __iov_iter_npages(const struct iov_iter *i, size_t maxsize, int maxpages);
 void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state);
 
 const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags);
 
+static inline int iov_iter_npages(const struct iov_iter *i, int maxpages)
+{
+	return __iov_iter_npages(i, SIZE_MAX, maxpages);
+}
+
 static inline size_t iov_iter_count(const struct iov_iter *i)
 {
 	return i->count;
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index b0e0acdf96c15..35a86d68f7073 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1763,9 +1763,9 @@ size_t hash_and_copy_to_iter(const void *addr, size_t bytes, void *hashp,
 }
 EXPORT_SYMBOL(hash_and_copy_to_iter);
 
-static int iov_npages(const struct iov_iter *i, int maxpages)
+static int iov_npages(const struct iov_iter *i, size_t size, int maxpages)
 {
-	size_t skip = i->iov_offset, size = i->count;
+	size_t skip = i->iov_offset;
 	const struct iovec *p;
 	int npages = 0;
 
@@ -1783,9 +1783,9 @@ static int iov_npages(const struct iov_iter *i, int maxpages)
 	return npages;
 }
 
-static int bvec_npages(const struct iov_iter *i, int maxpages)
+static int bvec_npages(const struct iov_iter *i, size_t size, int maxpages)
 {
-	size_t skip = i->iov_offset, size = i->count;
+	size_t skip = i->iov_offset;
 	const struct bio_vec *p;
 	int npages = 0;
 
@@ -1801,36 +1801,43 @@ static int bvec_npages(const struct iov_iter *i, int maxpages)
 	return npages;
 }
 
-int iov_iter_npages(const struct iov_iter *i, int maxpages)
+int __iov_iter_npages(const struct iov_iter *i, size_t maxsize, int maxpages)
 {
-	if (unlikely(!i->count))
+	if (i->count < maxsize)
+		maxsize = i->count;
+	if (unlikely(!maxsize))
 		return 0;
 	/* iovec and kvec have identical layouts */
 	if (likely(iter_is_iovec(i) || iov_iter_is_kvec(i)))
-		return iov_npages(i, maxpages);
+		return iov_npages(i, maxsize, maxpages);
 	if (iov_iter_is_bvec(i))
-		return bvec_npages(i, maxpages);
+		return bvec_npages(i, maxsize, maxpages);
 	if (iov_iter_is_pipe(i)) {
 		unsigned int iter_head;
 		int npages;
 		size_t off;
+		size_t n;
 
 		if (!sanity(i))
 			return 0;
 
 		data_start(i, &iter_head, &off);
+		n = DIV_ROUND_UP(off + maxsize, PAGE_SIZE);
+		if (maxpages > n)
+			maxpages = n;
+
 		/* some of this one + all after this one */
 		npages = pipe_space_for_user(iter_head, i->pipe->tail, i->pipe);
 		return min(npages, maxpages);
 	}
 	if (iov_iter_is_xarray(i)) {
 		unsigned offset = (i->xarray_start + i->iov_offset) % PAGE_SIZE;
-		int npages = DIV_ROUND_UP(offset + i->count, PAGE_SIZE);
+		int npages = DIV_ROUND_UP(offset + maxsize, PAGE_SIZE);
 		return min(npages, maxpages);
 	}
 	return 0;
 }
-EXPORT_SYMBOL(iov_iter_npages);
+EXPORT_SYMBOL(__iov_iter_npages);
 
 const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags)
 {