Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Nick Desaulniers <ndesaulniers@google.com>
Date: 2020-10-22 19:05:19
Also in:
io-uring, keyrings, linux-arch, linux-arm-kernel, linux-block, linux-fsdevel, linux-mips, linux-mm, linux-s390, linux-scsi, linux-security-module, lkml, netdev, sparclinux
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