On Thu, Jul 1, 2021 at 10:50 AM Peter Collingbourne pcc@google.com wrote:
On Thu, Jul 1, 2021 at 8:51 AM Catalin Marinas catalin.marinas@arm.com wrote:
Hi Peter,
On Wed, Jun 30, 2021 at 04:29:31PM -0700, Peter Collingbourne wrote:
If a user program uses userfaultfd on ranges of heap memory, it may end up passing a tagged pointer to the kernel in the range.start field of the UFFDIO_REGISTER ioctl. This can happen when using an MTE-capable allocator, or on Android if using the Tagged Pointers feature for MTE readiness [1].
When we added the tagged addr ABI, we realised it's nearly impossible to sort out all ioctls, so we added a note to the documentation that any address other than pointer to user structures as arguments to ioctl() should be untagged. Arguably, userfaultfd is not a random device but if we place it in the same category as mmap/mremap/brk, those don't allow tagged pointers either. And we do expect some apps to break when they rely on malloc() to return untagged pointers.
Okay, so arguably another approach would be to make userfaultfd consistent with mmap/mremap/brk and let the UFFDIO_REGISTER fail if given a tagged address.
This approach also seems reasonable. The problem, as things stand today, is that UFFDIO_REGISTER doesn't complain when a tagged pointer is used to register a memory range. But eventually the returned fault address in messages are untagged. If UFFDIO_REGISTER were to fail on passing a tagged pointer, then the userspace can address the issue.
When a fault subsequently occurs, the tag is stripped from the fault address returned to the application in the fault.address field of struct uffd_msg. However, from the application's perspective, the tagged address *is* the memory address, so if the application is unaware of memory tags, it may get confused by receiving an address that is, from its point of view, outside of the bounds of the allocation. We observed this behavior in the kselftest for userfaultfd [2] but other applications could have the same problem.
Just curious, what's generating the tagged pointers in the kselftest? Is it posix_memalign()?
Yes, on Android that call goes into our allocator which returns the tagged pointer.
Fix this by remembering which tag was used to originally register the userfaultfd and passing that tag back in fault.address. In a future enhancement, we may want to pass back the original fault address, but like SA_EXPOSE_TAGBITS, this should be guarded by a flag.
I don't see exposing the tagged fault address vs making up a tag (from the original request) that different. I find the former cleaner from an ABI perspective, though it's a bit more intrusive to pass the tagged address via handle_mm_fault().
My preference is to fix this in user-space entirely, by explicit untagging of the malloc'ed pointer either before being passed to userfaultfd or when handling the userfaultfd message. How common is it for apps to register malloc'ed pointers with userfaultfd? I was hoping that's more of an (anonymous) mmap() play.
I think it is very unlikely for someone to use malloc'ed pointers with userfaultfd.
At least we haven't seen any apps do this so far, and the tagged pointers feature has been in Android since last year's Android 11 release. So maybe we can say this is uncommon enough that we can just let userspace handle this. So we would do:
- Forbid tagged pointers in the ioctl as mentioned above.
- Fix the kselftest (e.g. by untagging the pointer, or making it use
mmap). A fix would probably be needed here anyway because we noticed that the test is later passing a tagged heap pointer to mremap (and failing).
The plan looks good to me. Using mmap (instead of posix_memalign) seems like a cleaner fix to the kselftest as compared to untagging the pointer everywhere.
I'd be okay with this approach but I'd first like to hear from Alistair and/or Lokesh since I think they favored the approach in my patch.
Peter