Re: [RFC] unifying write variants for filesystems

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Feb 04, 2014 at 09:27:23AM -0800, Zach Brown wrote:
> I think Kent is talking about what happens after the user addresses are
> consumed.  Turning dio into more of a bio mapping and redirection engine
> would use more of the bio machinery instead of the bits that dio has
> implemented itself with state in struct dio that hangs off the bios.  I
> imagine it'd still make sense to clean up the addresses/pages arguments
> that feed that engine.  (And give another entry point that already has
> bios for callers like loop, etc.)
> 
> > BTW, folks, any suggestions about the name of that "memory stream" thing?
> > struct iov_iter really implies iterator for iovec and more generic name
> > would probably be better...  struct mem_stream would probably do if nobody
> > comes up with better variant, but it's long and somewhat clumsy...
> 
> I don't like 'stream'.  To me that sounds more strictly advancing than I
> think this'd be capable of.  Maybe something dirt simple like 'mem_vec'?
> With 'mvec_' call prefixes?

Umm...  Frankly, I would rather discourage attempts to read the same data
twice, if only on the naming level...

Case in point: commit 1c1c87 (btrfs: sanitize BTRFS_IOC_FILE_EXTENT_SAME).
I really wonder how many places have similar holes.  What used to happen
was this: we have a userland structure, with a variable-sized array hanging
off its arse.  The size of array is determined by the field in fixed-sized
header.  We copy the header in, decide what size the whole thing should have,
and do memdup_user() to bring everything in.  Very convenient, since at that
point we have a pointer to that struct-with-array in the kernel space.
Attacker manages to increase the 'desc_count' field between two
copy_from_user()... and the sucker proceeds to loop over the array in
kernel-side copy, using the ->desc_count of that copy as the upper limit
of the loop.  Oops - in the best case, that is.

Double reads really ought to raise red flags on review.  I'm not saying that
they should be hard to do (after all, the fix in that commit *does* read the
same thing twice), but it's better if they are not used without thinking.
And no, I'm not suggesting to make ioctls use iov_iter/whatnot - it's just
an example of the class of bugs.  I wouldn't be surprised to find ->write()
instances in drivers suffering the same problem...