Re: [PATCH v14 1/4] asm-generic,arm64: create task variant of access_ok

[Gregory Price <gregory.price@memverge.com>]

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