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

[Arnd Bergmann <arnd@arndb.de>]

On Thu, Oct 22, 2020 at 9:05 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > On Thu, Oct 22, 2020 at 9:35 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > 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.

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