Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"

On Thu, Oct 22, 2020 at 9:05 PM Nick Desaulniers
[off-list ref] wrote:

On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann [off-list ref] wrote:

quoted

On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
[off-list ref] wrote:

quoted

On Thu, Oct 22, 2020 at 9:35 AM David Laight [off-list ref] wrote:

quoted

Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.

Why not x86-64? Wouldn't it be *any* LP64 ISA?

x86-64 is slightly special because most instructions on a 32-bit
argument clear the upper 32 bits, while on most architectures
the same instruction would leave the upper bits unchanged.

Oh interesting, depends on the operations too on x86_64 IIUC?

It seems this doesn't impact the calling conventions (see below),
it's just that there are more cases on x86 where the callee doesn't
have to explicitly clear the upper bits because the this is implied.

quoted

quoted

Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue .  Greg, would you mind running this
through the wringer?

I would not expect this to change anything for the bug that Greg
is chasing, unless there is also a bug in clang.

In the version before the patch, we get a 64-bit argument from
user space, which may consist of the intended value in the lower
bits plus garbage in the upper bits. However, vlen only gets
passed down  into import_iovec() without any other operations
on it, and since import_iovec takes a 32-bit argument, this is
where it finally gets narrowed.

Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).

Sorry I got it wrong, looked up the aarch64 AAPCS now, which
explains

 "Unlike in the 32-bit AAPCS, named integral values must be
  narrowed by the callee rather than the caller."

Also confirmed using https://godbolt.org/z/acPrjj, which
shows more combinations of compilers and architectures
in addition to your example. I had expected arm64 to be
like powerpc64 and arm32 in this case, but it's the reverse.

I also verified that SYSCALL_DEFINEx() is correct on arm64
and saw that as of v4.19 it passes the syscall arguments
through pt_regs, which will do the right thing here regardless
of the argument passing rules. The earlier version also seems
to be working as intended.

         Arnd