On Thu, Mar 30, 2023 at 03:05:07PM +0100, Catalin Marinas wrote: > > Ah, thanks for the pointer. > > For ptrace(), we live with this relaxation as there's no easy way to > check. Take __access_remote_vm() for example, it ends up in > get_user_pages_remote() -> ... -> __get_user_pages() which just untags > the address. Even if it would want to do this conditionally, the tag > pointer is enabled per thread (and inherited) but the GUP API only takes > the mm. > > While we could improve it as ptrace() can tell which thread it is > tracing, I don't think it's worth the effort. On arm64, top-byte-ignore > was enabled from the start for in-user accesses but not at the syscall > level. We wanted to avoid breaking some use-cases with untagging all > user pointers, hence the explicit opt-in to catch some issues (glibc did > have a problem with brk() ignoring the top byte - > https://bugzilla.redhat.com/show_bug.cgi?id=1797052). > > So yeah, this access_ok() in a few places is a best effort to catch some > potential ABI regressions like the one above and also as a way to force > the old ABI (mostly) via sysctl. But we do have places like GUP where we > don't have the thread information (only the mm), so we just > indiscriminately untag the pointer. > > Note that there is no security risk for the access itself. The Arm > architecture selects the user vs kernel address spaces based on bit 55 > rather than 63. Untagging a pointer sign-extends bit 55. > > > I did not have a sufficient answer for this so I went down this path. > > > > It does seem simpler to simply untag the address, however it didn't seem > > like a good solution to simply leave an identified bad edge case. > > > > with access_ok(untagged_addr(addr), ...) it breaks down like this: > > > > (tracer,tracee) : result > > > > tag,tag : untagged - (correct) > > tag,untag : untagged - incorrect as this would have been an impossible > > state to reach through the standard prctl interface. Will > > lead to a SIGSEGV in the tracee upon next syscall > > Well, even without untagging the pointer, the tracer can set a random > address that passes access_ok() but still faults in the tracee. It's the > tracer that should ensure the pointer is valid in the context of the > tracee. > > Now, even if the selector pointer is tagged, the accesses still work > fine (top-byte-ignore) unless MTE is enabled in the tracee and the tag > should match the region's colour. But, again, that's no different from a > debugger changing pointer variables in the debugged process, they should > be valid and it's not for the kernel to sanitise them. > > > untag,tag : untagged - (correct) > > untag,untag : no-op - (correct), tagged address will fail to set > > > > Basically if the tracer is a tagged process while the tracee is not, it > > would become possible to set the tracee's selector to a tagged pointer. > > Yes, but does it matter? You'd trust the tracer to work correctly. There > are multiple ways it can break the tracee here even if access_ok() > worked as intended. > > > It's beyond me to say whether or not this situation is "ok" and "the > > user's fault", but it does feel like an addressable problem. > > To me, the situation looks fine. While it's addressable, we have other > places where the tag is ignored on the ptrace() path, so I don't think > it's worth the effort. > > -- > Catalin Thank you for the extensive breakdown. Given this, it seems like I should just revert to untagging the pointer and drop the access_ok extensions. I'll add a comment at the untag site that discusses this interaction. Thanks! Gregory
