Re: [PATCH v13 1/3] syscall_user_dispatch: helper function to operate on given task

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

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