Re: [git pull] vfs.git pile 11 - iov_iter/hardening

[Linus Torvalds]

On Fri, Jul 7, 2017 at 5:29 PM, Al Viro  wrote:
>
> Trivial conflicts with libnvdimm; this stuff will get some
> followups, but again, that's for another series.

Gaah. Yeah, I guess I could have done the trivial ugly merge that just
took the new copy_from_iter_flushcache() as-is, and didn't match it up
with all the new iov_iter hardening.

Except I decided I don't want that, and want to do a proper merge instead.

Which made the trivial merge something that actually changed that
copy_from_iter_flushcache() logic, and maybe I screwed it up in the
process.

It builds, and it looks right to me, but you and Dan should really
check out the end result.

Particularly so for the CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE logic,
which is where this differs from the other cases, and which I changed
to make the #ifdef less noticeable.

I did try both a i386 and a x86-64 build of the iov_iter code, since
that should test both of those ARCH_HAS cases, but that was purely a
build test.

Also, the new __must_check warnings do trigger. At least for lustre. I
couldn't be arsed to try to fix those, since .. lustre. But I'm also
adding a couple of lustre people to the cc, just to make them aware of
it.. I'd really like the allmodconfig build to be clean.

Linus

Re: [git pull] vfs.git pile 11 - iov_iter/hardening

[Linus Torvalds]

On Fri, Jul 7, 2017 at 5:29 PM, Al Viro  wrote:
>
> Trivial conflicts with libnvdimm; this stuff will get some
> followups, but again, that's for another series.

Gaah. Yeah, I guess I could have done the trivial ugly merge that just
took the new copy_from_iter_flushcache() as-is, and didn't match it up
with all the new iov_iter hardening.

Except I decided I don't want that, and want to do a proper merge instead.

Which made the trivial merge something that actually changed that
copy_from_iter_flushcache() logic, and maybe I screwed it up in the
process.

It builds, and it looks right to me, but you and Dan should really
check out the end result.

Particularly so for the CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE logic,
which is where this differs from the other cases, and which I changed
to make the #ifdef less noticeable.

I did try both a i386 and a x86-64 build of the iov_iter code, since
that should test both of those ARCH_HAS cases, but that was purely a
build test.

Also, the new __must_check warnings do trigger. At least for lustre. I
couldn't be arsed to try to fix those, since .. lustre. But I'm also
adding a couple of lustre people to the cc, just to make them aware of
it.. I'd really like the allmodconfig build to be clean.

Linus

[git pull] vfs.git pile 11 - iov_iter/hardening

[Al Viro]

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.  It's not quite
what we need to check (in particular, combination of iovec-backed
and set_fs(KERNEL_DS) is almost certainly a bug, not a cause to skip
checks), but that's for later series.  For now let's keep might_fault().
* 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...

Trivial conflicts with libnvdimm; this stuff will get some
followups, but again, that's for another series.

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 09fc68dc66f7597bdc8898c991609a48f061bed5:

  iov_iter: saner checks on copyin/copyout (2017-07-07 05:18:09 -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
  iov_iter/hardening: move object size checks to inlined part
  iov_iter: sanity checks for copy to/from page primitives
  iov_iter: saner checks on copyin/copyout

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

[git pull] vfs.git pile 11 - iov_iter/hardening

[Al Viro]

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.  It's not quite
what we need to check (in particular, combination of iovec-backed
and set_fs(KERNEL_DS) is almost certainly a bug, not a cause to skip
checks), but that's for later series.  For now let's keep might_fault().
* 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...

Trivial conflicts with libnvdimm; this stuff will get some
followups, but again, that's for another series.

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 09fc68dc66f7597bdc8898c991609a48f061bed5:

  iov_iter: saner checks on copyin/copyout (2017-07-07 05:18:09 -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
  iov_iter/hardening: move object size checks to inlined part
  iov_iter: sanity checks for copy to/from page primitives
  iov_iter: saner checks on copyin/copyout

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

Re: [git pull] vfs.git pile 11

[Linus Torvalds]

On Thu, Jul 6, 2017 at 2:20 PM, Al Viro  wrote:
>
> Linus, could you hold that one back until tomorrow?  I want to tweak the
> last commit in there a bit, but I want to give it a local beating first...

Ok, dropping this one. All your other branches are merged now.

Linus

Re: [git pull] vfs.git pile 11

[Linus Torvalds]

On Thu, Jul 6, 2017 at 2:20 PM, Al Viro  wrote:
>
> Linus, could you hold that one back until tomorrow?  I want to tweak the
> last commit in there a bit, but I want to give it a local beating first...

Ok, dropping this one. All your other branches are merged now.

Linus

Re: [git pull] vfs.git pile 11

[Al Viro]

On Thu, Jul 06, 2017 at 09:29:27PM +0100, Al Viro wrote:
> On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote:
> > On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote:
> > > 
> > > +   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?
> > 
> > Matter of taste - I've no strong preferences here.
> > 
> > > +   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.
> > 
> > D'oh - shouldn't have written that pull request message before the
> > first cup of coffee...  might_sleep() it is, of course.
> 
> Hrm...  Said that, might_sleep() doesn't check one thing might_fault()
> does - the
> #if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> if (current->mm)
> might_lock_read(>mm->mmap_sem);
> #endif
> thing.  Let me think a bit...

FWIW,
* with iovec-backed, any of those primitives under pagefault_disable()
is seriously wrong.  To the point where we probably want to complain when
called that way.  That, of course, needs to be checked at the outermost level -
the primitives might do pagefault_disable() internally; that's fine.  Outside
caller doing that under pagefault_disable() isn't.
* uaccess_kernel() (== set_fs(KERNEL_DS)) has nothing to do with it.
If anything, we should not do copyin/copyout on iovec-backed ones with that
present.
* telling lockdep that we might end up grabbing ->mm->mmap_sem, OTOH,
is the right thing to do.  In addition to might_sleep().

Linus, could you hold that one back until tomorrow?  I want to tweak the
last commit in there a bit, but I want to give it a local beating first...

Re: [git pull] vfs.git pile 11

[Al Viro]

On Thu, Jul 06, 2017 at 09:29:27PM +0100, Al Viro wrote:
> On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote:
> > On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote:
> > > 
> > > +   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?
> > 
> > Matter of taste - I've no strong preferences here.
> > 
> > > +   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.
> > 
> > D'oh - shouldn't have written that pull request message before the
> > first cup of coffee...  might_sleep() it is, of course.
> 
> Hrm...  Said that, might_sleep() doesn't check one thing might_fault()
> does - the
> #if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> if (current->mm)
> might_lock_read(>mm->mmap_sem);
> #endif
> thing.  Let me think a bit...

FWIW,
* with iovec-backed, any of those primitives under pagefault_disable()
is seriously wrong.  To the point where we probably want to complain when
called that way.  That, of course, needs to be checked at the outermost level -
the primitives might do pagefault_disable() internally; that's fine.  Outside
caller doing that under pagefault_disable() isn't.
* uaccess_kernel() (== set_fs(KERNEL_DS)) has nothing to do with it.
If anything, we should not do copyin/copyout on iovec-backed ones with that
present.
* telling lockdep that we might end up grabbing ->mm->mmap_sem, OTOH,
is the right thing to do.  In addition to might_sleep().

Linus, could you hold that one back until tomorrow?  I want to tweak the
last commit in there a bit, but I want to give it a local beating first...

Re: [git pull] vfs.git pile 11

[Al Viro]

On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote:
> On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote:
> > 
> > +   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?
> 
> Matter of taste - I've no strong preferences here.
> 
> > +   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.
> 
> D'oh - shouldn't have written that pull request message before the
> first cup of coffee...  might_sleep() it is, of course.

Hrm...  Said that, might_sleep() doesn't check one thing might_fault()
does - the
#if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
if (current->mm)
might_lock_read(>mm->mmap_sem);
#endif
thing.  Let me think a bit...

Re: [git pull] vfs.git pile 11

[Al Viro]

On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote:
> On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote:
> > 
> > +   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?
> 
> Matter of taste - I've no strong preferences here.
> 
> > +   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.
> 
> D'oh - shouldn't have written that pull request message before the
> first cup of coffee...  might_sleep() it is, of course.

Hrm...  Said that, might_sleep() doesn't check one thing might_fault()
does - the
#if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
if (current->mm)
might_lock_read(>mm->mmap_sem);
#endif
thing.  Let me think a bit...

Re: [git pull] vfs.git pile 11

[Al Viro]

On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote:
> 
> +   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?

Matter of taste - I've no strong preferences here.

> +   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.

D'oh - shouldn't have written that pull request message before the
first cup of coffee...  might_sleep() it is, of course.

Re: [git pull] vfs.git pile 11

[Al Viro]

On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote:
> 
> +   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?

Matter of taste - I've no strong preferences here.

> +   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.

D'oh - shouldn't have written that pull request message before the
first cup of coffee...  might_sleep() it is, of course.

Re: [git pull] vfs.git pile 11

[Kees Cook]

On Thu, Jul 6, 2017 at 2:12 AM, Al Viro  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

Re: [git pull] vfs.git pile 11

[Kees Cook]

On Thu, Jul 6, 2017 at 2:12 AM, Al Viro  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

[git pull] vfs.git pile 11

[Al Viro]

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
  iov_iter/hardening: move object size checks to inlined part
  iov_iter: sanity checks for copy to/from page primitives
  iov_iter: saner checks on copyin/copyout

 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(-)

[git pull] vfs.git pile 11

[Al Viro]

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
  iov_iter/hardening: move object size checks to inlined part
  iov_iter: sanity checks for copy to/from page primitives
  iov_iter: saner checks on copyin/copyout

 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(-)