Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     Al Viro <viro@zeniv.linux.org.uk>, Jens Axboe <axboe@kernel.dk>,
        Christoph Hellwig <hch@lst.de>,
        Kees Cook <keescook@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <0bcc0c63-31a3-26fd-bccb-b28af0375c34@i-love.sakura.ne.jp>
Date:   Tue, 22 Dec 2020 11:33:58 -0600
In-Reply-To: <0bcc0c63-31a3-26fd-bccb-b28af0375c34@i-love.sakura.ne.jp>
        (Tetsuo Handa's message of "Tue, 22 Dec 2020 23:39:08 +0900")
Message-ID: <87a6u5iw3d.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: Does uaccess_kernel() work for detecting kernel thread?
Precedence: bulk

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