Re: copy_from_user_*() and buffer zeroing

["H. Peter Anvin" <hpa@zytor.com>]

On 11/26/2013 03:04 PM, NeilBrown wrote:
> On Tue, 26 Nov 2013 14:28:59 -0800 "H. Peter Anvin" <hpa@zytor.com>
> wrote:
> 
>> On 11/26/2013 01:54 PM, Andrew Morton wrote:
>>> 
>>> Nine years ago:
>>> 
>>> commit 7079f897164cb14f616c785d3d01629fd6a97719 Author: mingo
>>> <mingo> Date:   Fri Aug 27 17:33:18 2004 +0000
>>> 
>>> [PATCH] Add a few might_sleep() checks
>>> 
>>> Add a whole bunch more might_sleep() checks.  We also enable
>>> might_sleep() checking in copy_*_user().  This was non-trivial
>>> because of the "copy_*_user() in atomic regions" trick would
>>> generate false positives.  Fix that up by adding a new
>>> __copy_*_user_inatomic(), which avoids the might_sleep()
>>> check.
>>> 
>>> Only i386 is supported in this patch.
>>> 
>>> 
>>> I can't think of any reason why __copy_from_user_inatomic()
>>> should be non-zeroing.  But maybe I'm missing something - this
>>> would pretty easily permit uninitialised data to appear in
>>> pagecache and someone surely would have noticed..
>>> 
>> 
>> Yes, and the might_sleep() check is indeed bypassed.
>> 
>> However, the non-zeroing bit is currently motivated by the
>> following comment:
>> 
>> /** * __copy_from_user: - Copy a block of data from user space,
>> with less checking. * @to:   Destination address, in kernel
>> space. * @from: Source address, in user space. * @n:    Number of
>> bytes to copy. * * Context: User context only.  This function may
>> sleep. * * Copy data from user space to kernel space.  Caller
>> must check * the specified block with access_ok() before calling
>> this function. * * Returns number of bytes that could not be
>> copied. * On success, this will be zero. * * If some data could
>> not be copied, this function will pad the copied * data to the
>> requested size using zero bytes. * * An alternate version -
>> __copy_from_user_inatomic() - may be called from * atomic context
>> and will fail rather than sleep.  In this case the * uncopied
>> bytes will *NOT* be padded with zeros.  See fs/filemap.h * for
>> explanation of why this is needed. */
>> 
>> This comment is only present in the 32-bit code.  fs/filemap.h of
>> course no longer exists, however, the original commit seems to
>> be 01408c4939479ec46c15aa7ef6e2406be50eeeca which puts a comment
>> in the (now defunct mm/filemap.h).
>> 
>> I have to say I don't follow the explanation in that patch.  It
>> seems like if you're concurrently reading a buffer being written
>> you should expect to get any kind of mismash...
>> 
>> Neil, is this still an issue?
>> 
> 
> I can't be certain if this is "still" and issue as many things
> could have changed and I haven't been following them. I can try to
> explain the original issue though.
> 
> If a process tries to read a file while another process is writing
> to the same page of the same file, then it is quite reasonable for
> the reader to see almost any combination of the old and the new
> data.  However it is wrong for it so see something else.  In
> particular if the file actually contains no nuls, and the writer
> doesn't write any nuls, then the read should not see any nuls.
> 
> At the time of this patch, that could happen. If the page contains
> valid data it will not be locked, and a read can succeed at any
> time without further locking. When writing to a page,
> filemap_copy_from_user would first try an atomic copy and if that
> failed, it could write zeros into the page, which would then be 
> over-written by a subsequent non-atomic copy. This leaves a small
> window where zeros can be seen in the page by a read (or a
> memory-mapping).
> 
> A quick look at the code history shows that Nick Piggin removed the
> comment from mm/filemap.h in commit  4a9e5ef1f4f15205e477817a5 and
> it looks like the code was changed so it doesn't "try one way, then
> try another".  So it could well be that the failure mode that
> caused the problem before is no longer a possible failure mode. And
> if that failure mode is no longer possible, then maybe
> copy_from_user will never fail and so never has a need to fill with
> zeros??
> 

Nick, could you perhaps comment on this?

> The reason only i386 was changed it that it was the only arch were 
> copy_from_user_atomic might ever zero a tail.  Most arch just used
> memcpy or similar.  powerpc is the only other arch that defined a
> non-trivial copy_from_user_atomic and I confirmed at the time that
> it would never (need to) zero a tail.

Well, there are several that do now...

	-hpa