Does uaccess_kernel() work for detecting kernel thread?

Does uaccess_kernel() work for detecting kernel thread?

[Tetsuo Handa]

Commit db68ce10c4f0a27c ("new helper: uaccess_kernel()") replaced segment_eq(get_fs(), KERNEL_DS)
with uaccess_kernel(). But uaccess_kernel() became an unconditional "false" for some architectures
due to commit 5e6e9852d6f76e01 ("uaccess: add infrastructure for kernel builds with set_fs()") and
follow up changes in Linux 5.10. As a result, I guess that uaccess_kernel() can no longer be used
as a condition for checking whether current thread is a kernel thread or not.

For example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
isn't sg_check_file_access() failing to detect kernel context?

static int sg_check_file_access(struct file *filp, const char *caller)
{
	if (filp->f_cred != current_real_cred()) {
		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
			caller, task_tgid_vnr(current), current->comm);
		return -EPERM;
	}
	if (uaccess_kernel()) {
		pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
			caller, task_tgid_vnr(current), current->comm);
		return -EACCES;
	}
	return 0;
}

For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
isn't TOMOYO unexpectedly checking permissions for socket operations?

static bool tomoyo_kernel_service(void)
{
	/* Nothing to do if I am a kernel service. */
	return uaccess_kernel();
}

static u8 tomoyo_sock_family(struct sock *sk)
{
	u8 family;

	if (tomoyo_kernel_service())
		return 0;
	family = sk->sk_family;
	switch (family) {
	case PF_INET:
	case PF_INET6:
	case PF_UNIX:
		return family;
	default:
		return 0;
	}
}

Don't we need to replace such usage with something like (current->flags & PF_KTHREAD) ?
I don't know about io_uring, but according to
https://lkml.kernel.org/r/dacfb329-de66-d0cf-dcf9-f030ea1370de@schaufler-ca.com ,
should (current->flags & (PF_KTHREAD | PF_IO_WORKER)) == PF_KTHREAD be used instead?

Re: Does uaccess_kernel() work for detecting kernel thread?

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:

> Commit db68ce10c4f0a27c ("new helper: uaccess_kernel()") replaced segment_eq(get_fs(), KERNEL_DS)
> with uaccess_kernel(). But uaccess_kernel() became an unconditional "false" for some architectures
> due to commit 5e6e9852d6f76e01 ("uaccess: add infrastructure for kernel builds with set_fs()") and
> follow up changes in Linux 5.10. As a result, I guess that uaccess_kernel() can no longer be used
> as a condition for checking whether current thread is a kernel thread or not.
>
> For example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
> isn't sg_check_file_access() failing to detect kernel context?
>
> static int sg_check_file_access(struct file *filp, const char *caller)
> {
> 	if (filp->f_cred != current_real_cred()) {
> 		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
> 			caller, task_tgid_vnr(current), current->comm);
> 		return -EPERM;
> 	}
> 	if (uaccess_kernel()) {
> 		pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
> 			caller, task_tgid_vnr(current), current->comm);
> 		return -EACCES;
> 	}
> 	return 0;
> }
>
> For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
> isn't TOMOYO unexpectedly checking permissions for socket operations?
>
> static bool tomoyo_kernel_service(void)
> {
> 	/* Nothing to do if I am a kernel service. */
> 	return uaccess_kernel();
> }
>
> static u8 tomoyo_sock_family(struct sock *sk)
> {
> 	u8 family;
>
> 	if (tomoyo_kernel_service())
> 		return 0;
> 	family = sk->sk_family;
> 	switch (family) {
> 	case PF_INET:
> 	case PF_INET6:
> 	case PF_UNIX:
> 		return family;
> 	default:
> 		return 0;
> 	}
> }
>
> Don't we need to replace such usage with something like (current->flags & PF_KTHREAD) ?
> I don't know about io_uring, but according to
> https://lkml.kernel.org/r/dacfb329-de66-d0cf-dcf9-f030ea1370de@schaufler-ca.com ,
> should (current->flags & (PF_KTHREAD | PF_IO_WORKER)) == PF_KTHREAD be used instead?

I think you are reading the situation properly.

I skimmed the tomoyo code and it appears that you are excluding kernel
threads so as not to limit kernel threads such as nfsd.  For
PF_IO_WORKER kernel threads which are running code on behalf of a user
we want to perform the ordinary permission checks.  So you want
the idiom you pasted above.

I do wonder though if perhaps we should create a is_user_cred helper to
detect the difference between the creds of kernel threads and the thread
of ordinary userspace.   Which would handle io_uring that copy creds
around and check them at a later time more cleanly.

Eric

Re: Does uaccess_kernel() work for detecting kernel thread?

[Christoph Hellwig]

On Tue, Dec 22, 2020 at 11:39:08PM +0900, Tetsuo Handa wrote:
> For example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
> isn't sg_check_file_access() failing to detect kernel context?

sg_check_file_access does exactly the right thing - fail for all kernel
threads as those can't support the magic it does.

> For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
> isn't TOMOYO unexpectedly checking permissions for socket operations?

Can someone explain WTF TOMOYO is even doing there?  A security module
has absolutely no business checking what context it is called from, but
must check the process credentials instead.

Re: Does uaccess_kernel() work for detecting kernel thread?

[Tetsuo Handa]

On 2020/12/23 16:53, Christoph Hellwig wrote:
> On Tue, Dec 22, 2020 at 11:39:08PM +0900, Tetsuo Handa wrote:
>> For example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
>> isn't sg_check_file_access() failing to detect kernel context?
> 
> sg_check_file_access does exactly the right thing - fail for all kernel
> threads as those can't support the magic it does.

My question is, in Linux 5.10, sg_check_file_access() for x86 became

static int sg_check_file_access(struct file *filp, const char *caller)
{
	if (filp->f_cred != current_real_cred()) {
		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
			caller, task_tgid_vnr(current), current->comm);
		return -EPERM;
	}
	if (0) {
		pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
			caller, task_tgid_vnr(current), current->comm);
		return -EACCES;
	}
	return 0;
}

due to commit 5e6e9852d6f76e01 ("uaccess: add infrastructure for kernel
builds with set_fs()") and follow up changes. Don't we need to change this
"uaccess_kernel()" with "(current->flags & PF_KTHREAD)" ?

> 
>> For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
>> isn't TOMOYO unexpectedly checking permissions for socket operations?
> 
> Can someone explain WTF TOMOYO is even doing there?  A security module
> has absolutely no business checking what context it is called from, but
> must check the process credentials instead.
> 

TOMOYO distinguishes userspace processes and kernel threads, and grants
kernel threads implicit permissions to perform socket operations.
Since "uaccess_kernel()" became "0" for x86, TOMOYO is no longer able to
grant kernel threads implicit permissions to perform socket operations.
Since Eric says "For PF_IO_WORKER kernel threads which are running code on behalf
of a user we want to perform the ordinary permission checks.", I think that
TOMOYO wants to use "(current->flags & (PF_KTHREAD | PF_IO_WORKER)) == PF_KTHREAD"
instead.

Re: Does uaccess_kernel() work for detecting kernel thread?

[Christoph Hellwig]

On Tue, Dec 22, 2020 at 11:33:58AM -0600, Eric W. Biederman wrote:
> I do wonder though if perhaps we should create a is_user_cred helper to
> detect the difference between the creds of kernel threads and the thread
> of ordinary userspace.   Which would handle io_uring that copy creds
> around and check them at a later time more cleanly.

I don't think we should as no one has a business to check this difference.
If there is a case where the creds are not correct for all access decisions
we need to fix that rather than adding hacks.

Re: Does uaccess_kernel() work for detecting kernel thread?

[Christoph Hellwig]

On Wed, Dec 23, 2020 at 07:11:38PM +0900, Tetsuo Handa wrote:
> due to commit 5e6e9852d6f76e01 ("uaccess: add infrastructure for kernel
> builds with set_fs()") and follow up changes. Don't we need to change this
> "uaccess_kernel()" with "(current->flags & PF_KTHREAD)" ?

No.  The real problem here is that when a this funtion is called under
set_fs it allows kernel memory access for all user pointers, and due to
the indirection in the playload allows reading or changing kernel
memory.  A kthread does not have that issue.

> >> For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
> >> isn't TOMOYO unexpectedly checking permissions for socket operations?
> > 
> > Can someone explain WTF TOMOYO is even doing there?  A security module
> > has absolutely no business checking what context it is called from, but
> > must check the process credentials instead.
> > 
> 
> TOMOYO distinguishes userspace processes and kernel threads, and grants
> kernel threads implicit permissions to perform socket operations.

And this is the problem we need to fix.  A kernel thread can't just have
implicit permissions only because it is a kernel thread.  Think of e.g.
the io_uring service threads.

Re: Does uaccess_kernel() work for detecting kernel thread?

[Tetsuo Handa]

On 2021/01/05 16:59, Christoph Hellwig wrote:
> On Wed, Dec 23, 2020 at 07:11:38PM +0900, Tetsuo Handa wrote:
>> due to commit 5e6e9852d6f76e01 ("uaccess: add infrastructure for kernel
>> builds with set_fs()") and follow up changes. Don't we need to change this
>> "uaccess_kernel()" with "(current->flags & PF_KTHREAD)" ?
> 
> No.  The real problem here is that when a this funtion is called

Called by "who" ?
Called by "a userspace process" ?
Called by "a kernel thread" ?
Called by "an io_uring service thread" ?

>                                                                  under
> set_fs it allows kernel memory access for all user pointers, and due to
> the indirection in the playload allows reading or changing kernel
> memory.  A kthread does not have that issue.

If this uaccess_kernel() is intended to reject calling this function from
"a userspace process", uaccess_kernel() is failing to reject because
uaccess_kernel() is always "false" for x86.

If this uaccess_kernel() is intended to reject calling this function from
"a kernel thread", uaccess_kernel() is failing to reject because
uaccess_kernel() is always "false" for x86.

If this uaccess_kernel() is intended to reject calling this function from
"an io_uring service thread", uaccess_kernel() is failing to reject because
uaccess_kernel() is always "false" for x86.

What does uaccess_kernel() in sg_check_file_access() (and uhid_char_write(),
ib_safe_file_access(), bpfilter_process_sockopt() etc.) want to check?

> 
>>>> For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n,
>>>> isn't TOMOYO unexpectedly checking permissions for socket operations?
>>>
>>> Can someone explain WTF TOMOYO is even doing there?  A security module
>>> has absolutely no business checking what context it is called from, but
>>> must check the process credentials instead.
>>>
>>
>> TOMOYO distinguishes userspace processes and kernel threads, and grants
>> kernel threads implicit permissions to perform socket operations.
> 
> And this is the problem we need to fix.  A kernel thread can't just have
> implicit permissions only because it is a kernel thread.  Think of e.g.
> the io_uring service threads.

We can use (current->flags & (PF_KTHREAD | PF_IO_WORKER)) == PF_KTHREAD like
https://lkml.kernel.org/r/dacfb329-de66-d0cf-dcf9-f030ea1370de@schaufler-ca.com does
in order to exclude e.g. the io_uring service threads, can't we?