mbox series

[v2,0/4] usercopy: generic tests + arm64 fixes

Message ID 20230321122514.1743889-1-mark.rutland@arm.com
Headers
Series usercopy: generic tests + arm64 fixes |

Message

Mark Rutland March 21, 2023, 12:25 p.m. UTC 
  This series adds tests for the usercopy functions, which have found a
few issues on most architectures I've tested on. The last two patches
are fixes for arm64 (which pass all the tests, and are at least as good
performance-wise as the existing implementation).

Arch maintainers: if you are Cc'd on patches 1 or 2, I believe that your
architecture has an issue (which should be called out explicitly in the
commit message). I've only been able to test on some architectures, so
other architectures may be affected.

Al, Linus: could you double-check that I have the requirements right?
I'm going by Al's prior message at:

  https://lore.kernel.org/all/YNSyZaZtPTmTa5P8@zeniv-ca.linux.org.uk/

Which mentions the commentary in include/linux/uaccess.h:

 * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
 * at to must become equal to the bytes fetched from the corresponding area
 * starting at from.  All data past to + size - N must be left unmodified.
 *
 * If copying succeeds, the return value must be 0.  If some data cannot be
 * fetched, it is permitted to copy less than had been fetched; the only
 * hard requirement is that not storing anything at all (i.e. returning size)
 * should happen only when nothing could be copied.  In other words, you don't
 * have to squeeze as much as possible - it is allowed, but not necessary.

I've made two additional assumptions:

1) That a usercopy in a non-atomic context shouldn't have a partial
   failure if it's possible for it to succeed completely. i.e. callers
   don't need to call it in a loop if they only care about it entirely
   succeeding or failing.

   Code all over the place seems to rely upon that (e.g. signal code
   using copy_to_user() to place signal frames), so I assume this must
   be the case or we have bigger issues.

2) That clear_user() has the same requirements, given it has the same
   rough shape as raw_copy_{to,from}_user(), and I believe it's used in
   the same way in the iov_iter code (but I could be mistaken).

Since v1 [1]:
* Trivial rebase to v6.3-rc3
* Simplify the test harness
* Improve comments
* Improve commit messages
* Expand Cc list

[1] https://lore.kernel.org/lkml/20230314115030.347976-1-mark.rutland@arm.com/

Thanks,
Mark.

Mark Rutland (4):
  lib: test copy_{to,from}_user()
  lib: test clear_user()
  arm64: fix __raw_copy_to_user semantics
  arm64: fix clear_user() semantics

 arch/arm64/lib/clear_user.S   | 195 ++++++++++---
 arch/arm64/lib/copy_to_user.S | 203 +++++++++++---
 lib/Kconfig.debug             |   9 +
 lib/Makefile                  |   1 +
 lib/usercopy_kunit.c          | 506 ++++++++++++++++++++++++++++++++++
 5 files changed, 834 insertions(+), 80 deletions(-)
 create mode 100644 lib/usercopy_kunit.c

Comments

Kees Cook Nov. 15, 2023, 10:30 p.m. UTC | #1 
On Tue, Mar 21, 2023 at 12:25:10PM +0000, Mark Rutland wrote:
> This series adds tests for the usercopy functions, which have found a
> few issues on most architectures I've tested on. The last two patches
> are fixes for arm64 (which pass all the tests, and are at least as good
> performance-wise as the existing implementation).
> [...]
>  lib/usercopy_kunit.c          | 506 ++++++++++++++++++++++++++++++++++

You didn't like lib/test_user_copy.c ? :)

There was a prior attempt to move it to KUnit:

https://lore.kernel.org/lkml/20200721174654.72132-1-vitor@massaru.org/