Re: [git pull] vfs.git pile 11

[Kees Cook]

On Thu, Jul 6, 2017 at 2:12 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>         iov_iter/uaccess/hardening pile.  For one thing, it trims the
> inline part of copy_to_user/copy_from_user to the minimum that *does*
> need to be inlined - object size checks, basically.  For another,
> it sanitizes the checks for iov_iter primitives.  There are 4 groups
> of checks: access_ok(), might_fault(), object size and KASAN.
>         * access_ok() had been verified by whoever had set the iov_iter
> up.  However, that has happened in a function far away, so proving that
> there's no path to actual copying bypassing those checks is hard and
> proving that iov_iter has not been buggered in the meanwhile is also
> not pleasant.  So we want those redone in actual copyin/copyout.
>         * might_fault() is better off consolidated - we know whether
> it needs to be checked as soon as we enter iov_iter primitive and
> observe the iov_iter flavour.  No need to wait until the copyin/copyout.
> The call chains are short enough to make sure we won't miss anything -
> in fact, it's more robust that way, since there are cases where we do
> e.g. forced fault-in before getting to copyin/copyout.
>         * KASAN checks belong in copyin/copyout - at the same level
> where other iov_iter flavours would've hit them in memcpy().
>         * object size checks should apply to *all* iov_iter flavours,
> not just iovec-backed ones.
>         There are two groups of primitives - one gets the kernel object
> described as pointer + size (copy_to_iter(), etc.) while another gets
> it as page + offset + size (copy_page_to_iter(), etc.)
>         For the first group the checks are best done where we actually
> have a chance to find the object size.  In other words, those belong in
> inline wrappers in uio.h, before calling into iov_iter.c.  Same kind
> as we have for inlined part of copy_to_user().
>         For the second group there is no object to look at - offset in
> page is just a number, it bears no type information.  So we do them
> in the common helper called by iov_iter.c primitives of that kind.
> All it currently does is checking that we are not trying to access
> outside of the compound page; eventually we might want to add some
> sanity checks on the page involved.
>
>         So the things we need in copyin/copyout part of iov_iter.c
> do not quite match anything in uaccess.h (we want no zeroing, we *do*
> want access_ok() and KASAN and we want no might_fault() or object size
> checks done on that level).  OTOH, these needs are simple enough to
> provide a couple of helpers (static in iov_iter.c) doing just what
> we need...
>
> The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6:
>
>   Linux 4.12-rc1 (2017-05-13 13:19:49 -0700)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git uaccess-work.iov_iter
>
> for you to fetch changes up to ea93a426af164d346a0b4fe0836143bf32177330:
>
>   iov_iter: saner checks on copyin/copyout (2017-06-29 22:29:36 -0400)
>
> ----------------------------------------------------------------
> Al Viro (5):
>       copy_{from,to}_user(): move kasan checks and might_fault() out-of-line
>       copy_{to,from}_user(): consolidate object size checks

We still need to fix the missed-zeroing-on-overflow corner-case:
https://patchwork.kernel.org/patch/9826959/

>       iov_iter/hardening: move object size checks to inlined part

+   if (unlikely(!check_copy_size(addr, bytes, false)))
+       return false;
+   else
+       return _copy_from_iter_full(addr, bytes, i);

Can these be rewritten to avoid the double-negative?

>       iov_iter: sanity checks for copy to/from page primitives

Nice to see these!

>       iov_iter: saner checks on copyin/copyout

+   might_fault();

Should this be might_sleep()? Just from reading the patch it looked
like you were adding might_sleep()s in the other cases.

>
>  include/linux/thread_info.h | 27 +++++++++++++
>  include/linux/uaccess.h     | 44 +++++----------------
>  include/linux/uio.h         | 58 ++++++++++++++++++++++++---
>  lib/iov_iter.c              | 96 ++++++++++++++++++++++++++++++++-------------
>  lib/usercopy.c              | 10 ++++-
>  5 files changed, 167 insertions(+), 68 deletions(-)

-Kees

-- 
Kees Cook
Pixel Security