On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann <arnd@xxxxxxxx> wrote: > > On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers > <ndesaulniers@xxxxxxxxxx> wrote: > > On Thu, Oct 22, 2020 at 9:35 AM David Laight <David.Laight@xxxxxxxxxx> 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? > > > 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
