On Tue, Mar 21, 2023 at 08:46:26PM +0100, Thomas Gleixner wrote:
> On Tue, Mar 21 2023 at 12:55, Gregory Price wrote:
> > On Tue, Mar 21, 2023 at 04:41:37PM +0100, Thomas Gleixner wrote:
> >> On Wed, Mar 01 2023 at 15:58, Gregory Price wrote:
> >> > +static int task_set_syscall_user_dispatch(struct task_struct *task, unsigned long mode,
> >> > + unsigned long offset, unsigned long len,
> >> > + char __user *selector)
> >> > {
> >> > switch (mode) {
> >> > case PR_SYS_DISPATCH_OFF:
> >> ...
> >>
> >> case PR_SYS_DISPATCH_ON:
> >> if (selector && !access_ok(selector, sizeof(*selector)))
> >> return -EFAULT;
> >>
> >> I'm not seing how this can work on ARM64 when user pointer tagging is
> >> enabled in the tracee, but not in the tracer. In such a case, if the
> >> pointer is tagged, access_ok() will fail because access_ok() wont untag
> >> it.
> >
> > I see that untagged_addr(x) is available to clear tags, I don't see an
> > immediate issues with converting to:
> >
> > !access_ok(untagged_addr(selector), sizeof(*selector))
>
> If this would be correct, then access_ok() on arm64 would
> unconditionally untag the checked address, but it does not. Simply
> because untagging is only valid if the task enabled pointer tagging. If
> it didn't a tagged pointer is obviously invalid.
>
> Why would ptrace make this suddenly valid?
>
> Just because it's in the way of what you want to achieve is not a really
> sufficient justification.
>
> Thanks,
>
> tglx
Ah, I see, The issue stems from this code in arch/arm64/asm/uaccess.h
static inline int access_ok(const void __user *addr, unsigned long size)
{
/*
* Asynchronous I/O running in a kernel thread does not have the
* TIF_TAGGED_ADDR flag of the process owning the mm, so always untag
* the user address before checking.
*/
if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
(current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
addr = untagged_addr(addr);
return likely(__access_ok(addr, size));
}
The calling task clears the tags if the tagged flag is set.
The problem is that no task_access_ok equivalent exists to validate a
pointer based on another task's settings.
The "clean" way to fix this issue is with a task_access_ok, this keeps
things portable.
On ARM64, it looks like refactoring access_ok into the following:
static inline int task_access_ok(struct task_struct *task,
const void __user *addr,
unsigned long size)
{
/*
* Asynchronous I/O running in a kernel thread does not have the
* TIF_TAGGED_ADDR flag of the process owning the mm, so always untag
* the user address before checking.
*/
if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
(task->flags & PF_KTHREAD || test_ti_thread_flag(task, TIF_TAGGED_ADDR)))
addr = untagged_addr(addr);
return likely(__access_ok(addr, size));
}
static inline int access_ok(const void __user *addr, unsigned long size)
{
return task_access_ok(current, addr, size);
}
#define task_access_ok task_access_ok
#define access_ok access_ok
A similar change is made in include/asm-generic/access_ok.h
If this is an amenable solution, I will pull this into a patch ahead of
the changes in syscall user dispatch.
~Gregory
