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

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?

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).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.

After your patch, the SYSCALL_DEFINE3() does the narrowing
conversion with the same clearing of the upper bits.

If there is a problem somewhere leading up to import_iovec(),
it would have to in some code that expects to get a 32-bit
register argument but gets called with a register that has
garbage in the upper bits /without/ going through a correct
sanitizing function like SYSCALL_DEFINE3().

      Arnd

-- 
Thanks,
~Nick Desaulniers