Re: [PATCH v8 00/10] fs: interface for directly reading/writing compressed data

[Omar Sandoval <osandov@osandov.com>]

On Fri, Mar 19, 2021 at 02:47:03PM -0700, Linus Torvalds wrote:
> On Fri, Mar 19, 2021 at 2:12 PM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > After spending a few minutes trying to simplify copy_struct_from_iter(),
> > it's honestly easier to just use the iterate_all_kinds() craziness than
> > open coding it to only operate on iov[0]. But that's an implementation
> > detail, and we can trivially make the interface stricter:
> 
> This is an improvement, but talking about the iterate_all_kinds()
> craziness, I think your existing code is broken.
> 
> That third case (kernel pointer source):
> 
> +    copy = min(ksize - copied, v.iov_len);
> +    memcpy(dst + copied, v.iov_base, copy);
> +    if (memchr_inv(v.iov_base, 0, v.iov_len))
> +        return -E2BIG;
> 
> can't be right. Aren't you checking that it's *all* zero, even the
> part you copied?

Oops, that should of course be

	if (memchr_inv(v.iov_base + copy, 0, v.iov_len - copy))
		return -E2BIG;

like the other cases. Point taken, though.

> Our iov_iter stuff is really complicated already, this is part of why
> I'm not a huge fan of using it.
> 
> I still suspect you'd be better off not using the iterate_all_kinds()
> thing at all, and just explicitly checking ITER_BVEC/ITER_KVEC
> manually.
> 
> Because you can play games like fooling your "copy_struct_from_iter()"
> to not copy anything at all with ITER_DISCARD, can't you?
> 
> Which then sounds like it might end up being useful as a kernel data
> leak, because it will use some random uninitialized kernel memory for
> the structure.
> 
> Now, I don't think you can actually get that ITER_DISCARD case, so
> this is not *really* a problem, but it's another example of how that
> iterate_all_kinds() thing has these subtle cases embedded into it.

Right, that would probably be better off returning EFAULT or something
for ITER_DISCARD.

> The whole point of copy_struct_from_iter() is presumably to be the
> same kind of "obviously safe" interface as copy_struct_from_user() is
> meant to be, so these subtle cases just then make me go "Hmm".
> 
> I think just open-coding this when  you know there is no actual
> looping going on, and the data has to be at the *beginning*, should be
> fairly simple. What makes iterate_all_kinds() complicated is that
> iteration, the fact that there can be empty entries in there, but it's
> also that "iov_offset" thing etc.
> 
> For the case where you just (a) require that iov_offset is zero, and
> (b) everything has to fit into the very first iov entry (regardless of
> what type that iov entry is), I think you actually end up with a much
> simpler model.
> 
> I do realize that I am perhaps concentrating a bit too much on this
> one part of the patch series, but the iov_iter thing has bitten us
> before. And it has bitten really core developers and then Al has had
> to fix up mistakes.
> 
> In fact, it wasn't that long ago that I asked Al to verify code I
> wrote, because I was worried about having missed something subtle. So
> now when I see these iov_iter users, it just makes me go all nervous.

So here's what it looks like with these restrictions (chances are
there's a bug or two in here):

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_atomic(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_atomic(p);
			return -E2BIG;
		}
		kunmap_atomic(p);
	} else {
		return -EFAULT;
	}
	if (usize < ksize)
		memset(dst + usize, 0, ksize - usize);
	iov_iter_advance(i, usize);
	return 0;
}

Not much shorter, but it is easier to follow.