Date:   Tue, 21 Mar 2023 17:12:28 -0400
From:   Gregory Price <gregory.price@memverge.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Gregory Price <gourry.memverge@gmail.com>,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        oleg@redhat.com, avagin@gmail.com, peterz@infradead.org,
        luto@kernel.org, krisman@collabora.com, corbet@lwn.net,
        shuah@kernel.org, Mark Rutland <mark.rutland@arm.com>,
        Will Deacon <will@kernel.org>
Subject: Re: [PATCH v13 1/3] syscall_user_dispatch: helper function to
 operate on given task
Message-ID: <ZBodvECutssWNo6a@memverge.com>
References: <20230301205843.2164-1-gregory.price@memverge.com>
 <20230301205843.2164-2-gregory.price@memverge.com>
 <87cz529kni.ffs@tglx>
 <ZBnhmtvlenY15P32@memverge.com>
 <87a605anvx.ffs@tglx>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87a605anvx.ffs@tglx>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?D+LPnVGXFc75Va618gF11VRfbRGN5qaOuAytzi+ZaM1txDwUedzqEPwOl1g/?=
 =?us-ascii?Q?f19+4S3Ubc14gyW9bUcoV+olPvdN8I1UF0F1K4x+Wnud3YDz2XAIP85LWSTI?=
 =?us-ascii?Q?K+LRXBvf9Z4HrzDTfQiYzmYrVOC2lFiS/zioCx/7yKg2TY988ICZaVNo3InE?=
 =?us-ascii?Q?UV+H3TmicAvJ2hg4owP8XXLigyyQ685u3V+P0sTOYs8AwoL+fmyE4JQ+10i6?=
 =?us-ascii?Q?qWyKCjFVVcc3wu9XevNmE2MF/9lxDgxLhMYer1ohdN0k/6Do9nXyAIANAoLv?=
 =?us-ascii?Q?IIbfR2kK0xTeLZVUo+CsDhhP6CCJfo/kCa0v2rb8wy1wwr27ezkOaEP+72q7?=
 =?us-ascii?Q?wUO06aOEXnSyYxXWkE0wgEdJ7orJPu3hY8nMicNI9CUQ1SJBHo3+jdL7ET+D?=
 =?us-ascii?Q?AD8J8IWS0yC/Ybwduu/1vbuMk3gUCn0+mc0kdY9EPxwELNeTJz7rcC/v0Vo1?=
 =?us-ascii?Q?ekDWml/yOO+J4euNpxjHr1DtUuY68wTdqUliRSwfYYomH/BaPRXc88rchHmi?=
 =?us-ascii?Q?dPfWGzafcid1LAfNoBCkc3tx+YcEPPvKoMTk59BgT8i6AglOqYx5T4wZWnV0?=
 =?us-ascii?Q?GpH8nvlnFmOQL9ZL0d7DZhCoLBnC14a64SEduNiPkCzWBovkWejAZJqh4YLE?=
 =?us-ascii?Q?shEal/i7EQxoq7HoRCjlpl5ZknPPZ9YUHWjOBLgZPIFMj30Lmp3+RoQAOQvA?=
 =?us-ascii?Q?bPHc/Sv0eJm/qlEQpMdZRG2WaiDd5DBbebIZ6IQuDoXjytL/vyz7Gd6oU8Qg?=
 =?us-ascii?Q?4/nsIFSVkppBVAa2LqnRlHxPd+KRBudJu0KqrmPiFLbhkmj/w08eEK8VuQg1?=
 =?us-ascii?Q?JTRxaYbJb+sM6CS3vShCbskbO3vBxgBoKTXJPvjh2Nc/jFAW5MBSfWQPcdkc?=
 =?us-ascii?Q?0AYAq5EQIFew3b+dBx/8pgFQSXjLN7OuOaN66MS9sKQGyJvp0YT4UCigB+id?=
 =?us-ascii?Q?0ql47Qr41WKJYojn7ZegCIgk4KNp/0Y0H3BN2t5VPdgAwi/rxMW/CkTIaWCb?=
 =?us-ascii?Q?gUKLZD63UzixMwMD7zY3cr1c4//jtPfe7jJHsGjo8lKyTpHtWgl3TcVlP3ey?=
 =?us-ascii?Q?Rhv1Qowo+lCx0lsRJSb0Xs5DW6vDHvPtQNF5m29gGX73qICh/4peRO7w1qgI?=
 =?us-ascii?Q?1vP+9Ta2HMb1VcPA12R/nG2UvdpJI9rUkjjLufJKLamn/n09l5Tq9vpTayXu?=
 =?us-ascii?Q?j11I0Y9uZMf6qpYsaETIHTAN5bepvU8ZSigMPoNyIfSamFiwb3bFQ83cOj6V?=
 =?us-ascii?Q?90Kit4msej1kQMQPrQ5vPrKsCGGDV/VAM4BUbbtMsRoNCOnFrpsvAlKXd3pt?=
 =?us-ascii?Q?U/EmxyVUXPuZ7zT3CBO4P4ffUkG9iGsEESajPIWckHqgi94PhyoHXGl0CMbK?=
 =?us-ascii?Q?AXOZkioeyhzvHwF/v4JV+MeJgebhbhKFsuwx1wZdNvVDywO9VCTS+3aodXBK?=
 =?us-ascii?Q?WazMLtMMcgzJY+Lt9+9AqSofnkrhFDDHj2MqVl7NuDYYwrWw6IjMryaBiChg?=
 =?us-ascii?Q?/F7/WhVGQgf1lpxePXxDYyVwlNPTLGNV0yWOCODgKcHpvos7m8CwqOxgbfNX?=
 =?us-ascii?Q?d7ZwoJ6fP7yFU+w8Tv9hAbk0YXeQVFgc9NSWPrmLv7BNnId+d74k/k2HbEW4?=
 =?us-ascii?Q?xctkGZwudDd1U8twlgh37Vs=3D?=
X-OriginatorOrg: memverge.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0697758e-ad11-49b2-e16b-08db2a50ff5d
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR17MB5512.namprd17.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2023 21:12:37.5962
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5c90cb59-37e7-4c81-9c07-00473d5fb682
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: W1p9sCWhVoEuQQuy+r5wSdkJ3cyKcgJxzOI4QRMOq6PunWjoJIlP8Q61lZYwCJauIUjMO3aMwXE1fkX+MsmG/Jr37P1PBhqpXQ0N05SAuG8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR17MB4487
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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